ESP32: Add low level SD card raw IO NIFs#2343
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
c385dc0 to
544e190
Compare
PR review: low-level ESP32 SD-card block-device NIFsRange reviewed: The shared SD-card option parser appears to preserve the existing FAT mount behavior, and the new API has useful happy-path coverage. However, the raw-device implementation currently has three release-blocking correctness issues: it can tear down host state belonging to another card, it can truncate oversized Erlang sector integers, and it can dereference an invalid binary term after allocation failure. Findings1. [P1] Closing or failing to open one raw card can tear down unrelated cards
This is unsafe when another raw card or FAT mount is active. ESP-IDF documents Use the host descriptor's per-slot callback when Illustrative minimal direction (with version guards as required by the supported IDF matrix): diff --git a/src/platforms/esp32/components/avm_builtins/storage_nif.c b/src/platforms/esp32/components/avm_builtins/storage_nif.c
@@
+static esp_err_t sdcard_host_deinit(const sdmmc_host_t *host)
+{
+ if (host->flags & SDMMC_HOST_FLAG_DEINIT_ARG) {
+ return host->deinit_p(host->slot);
+ }
+ return host->deinit();
+}
+
static esp_err_t sdcard_blockdev_close(struct SDCardBlockDevice *dev)
{
- esp_err_t ret = ESP_OK;
-
- if (dev->interface == SDCardSDSPI) {
- sdspi_host_remove_device(dev->spi_handle);
- ret = sdspi_host_deinit();
-#ifdef SDMMC_SLOT_CONFIG_DEFAULT
- } else {
- ret = sdmmc_host_deinit();
-#endif
- }
+ esp_err_t ret = sdcard_host_deinit(&dev->card->host);
@@
err = sdspi_host_init_device(&cfg->slot.spi_dev, &dev->spi_handle);
if (UNLIKELY(err != ESP_OK)) {
- sdspi_host_deinit();
free(dev->card);
@@
err = sdmmc_card_init(&cfg->host, dev->card);
if (UNLIKELY(err != ESP_OK)) {
- sdspi_host_remove_device(dev->spi_handle);
- sdspi_host_deinit();
+ sdcard_host_deinit(&cfg->host);
@@
err = sdmmc_host_init_slot(cfg->host.slot, &cfg->slot.mmc_slot);
if (UNLIKELY(err != ESP_OK)) {
- sdmmc_host_deinit();
free(dev->card);
@@
err = sdmmc_card_init(&cfg->host, dev->card);
if (UNLIKELY(err != ESP_OK)) {
- sdmmc_host_deinit();
+ sdcard_host_deinit(&cfg->host);Add a regression that keeps one card/mount usable after a second open fails, plus close/reopen and resource-GC coverage. This needs hardware coverage for both SDMMC and SDSPI; the current QEMU happy path cannot establish ownership correctness. 2. [P1] Oversized sector integers can wrap to another sector, including sector 0Both read and write accept any AtomVM integer with The card capacity is stored as diff --git a/src/platforms/esp32/components/avm_builtins/storage_nif.c b/src/platforms/esp32/components/avm_builtins/storage_nif.c
@@ static term nif_esp_sdcard_read(Context *ctx, int argc, term argv[])
- if (UNLIKELY(!term_is_any_integer(argv[1]))) {
+ if (UNLIKELY(!term_is_uint32(argv[1]))) {
RAISE_ERROR(BADARG_ATOM);
}
- avm_int64_t sector = term_maybe_unbox_int64(argv[1]);
+ uint32_t sector = term_to_uint32(argv[1]);
@@
- if (UNLIKELY(sector < 0 || (uint64_t) sector >= dev->sector_count)) {
+ if (UNLIKELY(sector >= dev->sector_count)) {
@@ static term nif_esp_sdcard_write(Context *ctx, int argc, term argv[])
- if (UNLIKELY(!term_is_any_integer(argv[1]))) {
+ if (UNLIKELY(!term_is_uint32(argv[1]))) {
RAISE_ERROR(BADARG_ATOM);
}
- avm_int64_t sector = term_maybe_unbox_int64(argv[1]);
+ uint32_t sector = term_to_uint32(argv[1]);
@@
- if (UNLIKELY(sector < 0 || (uint64_t) sector >= dev->sector_count)) {
+ if (UNLIKELY(sector >= dev->sector_count)) {diff --git a/src/platforms/esp32/test/main/test_erl_sources/test_sdcard.erl b/src/platforms/esp32/test/main/test_erl_sources/test_sdcard.erl
@@ test_bad_args(SDCard) ->
{ok, #{sector_size := SectorSize, sector_count := SectorCount}} = esp:sdcard_info(SDCard),
+ ok = expect_badarg(fun() -> esp:sdcard_read(SDCard, -1) end),
+ ok = expect_badarg(fun() -> esp:sdcard_read(SDCard, 1 bsl 32) end),
+ ok = expect_badarg(fun() -> esp:sdcard_read(SDCard, 1 bsl 64) end),
ok = expect_badarg(fun() -> esp:sdcard_write(SDCard, 0, <<0>>) end),3. [P1] Read allocation failure can pass an invalid term to
|
term_is_uint32 was returning true on 32-bit builds for big integers whose low 64 bits fit uint32_t, so term_to_uint32 truncated them. Signed-off-by: Davide Bettio <davide@uninstall.it>
Check the term_is_* integer predicates and term_to_* conversions against per-case expected flags, and run the new test-term binary in CI. Signed-off-by: Davide Bettio <davide@uninstall.it>
Move the "sdmmc" / "sdspi" host and slot configuration out of nif_esp_mount into sdcard_config_from_source so the upcoming raw block device NIFs can reuse it instead of duplicating the option parsing. Pure refactor: the FAT mount path is unchanged. Signed-off-by: Davide Bettio <davide@uninstall.it>
Add esp:sdcard_open/2, esp:sdcard_read/2, esp:sdcard_write/3, esp:sdcard_info/1 and esp:sdcard_close/1 to read and write raw SD card sectors without mounting a filesystem, for custom filesystems, partition inspection and raw imaging. The card is opened with the same source and options as esp:mount/4 but initialized via sdmmc_card_init with no VFS/FAT layer. A new AVM_ENABLE_RAW_SDCARD_NIFS option enables them independently of the mount NIFs, so a raw-only build links no filesystem code. Signed-off-by: Davide Bettio <davide@uninstall.it>
Add esp:sdcard_open/2, esp:sdcard_read/2, esp:sdcard_write/3,
esp:sdcard_info/1 and esp:sdcard_close/1 to read and write raw SD card
sectors without mounting a filesystem, for custom filesystems, partition
inspection and raw imaging. Cards are opened with the same source and
options as esp:mount/4 ("sdmmc" or "sdspi"), but are initialized with no
VFS/FAT layer.
The series is two commits, reviewable bottom-up:
the FAT mount path and the new NIFs share the "sdmmc"/"sdspi" option
parsing; mount behavior is unchanged.
new AVM_ENABLE_RAW_SDCARD_NIFS option that is independent of the mount
NIFs, so a raw-only build links no filesystem code (~30 KB less
flash). Includes documentation and a QEMU-run test.
Known constraints, documented in the programmers guide: the same card
must not be driven through the raw API and esp:mount/4 at once; the NIFs
run the card transaction inline and block the scheduler, like
esp:mount/4; an SDSPI card must be closed before spi:close/1 releases
the bus.
The out-of-range sector checks rely on the term_is_uint32 fix (#2380):
without it, 32-bit builds truncate arguments like 1 bsl 64 instead of
rejecting them, and the new badarg tests fail.
Merge after #2380
These changes are made under both the "Apache 2.0" and the "GNU Lesser
General Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later