ESP32: raw SD IO

[bettio]

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

[petermm]

PR Review — ESP32 raw SD card block device NIFs

Reviewed commits:

• a11ecaeb — storage_nif: Extract shared SD card config parser (pure refactor)
• 9a308ac8 — esp32: Add low level SD card block device NIFs (esp:sdcard_open/2, sdcard_read/2, sdcard_write/3, sdcard_info/1, sdcard_close/1)

Files: src/platforms/esp32/components/avm_builtins/storage_nif.c, libs/avm_esp32/src/esp.erl,
doc/src/programmers-guide.md, src/platforms/esp32/test/main/test_erl_sources/test_sdcard.erl,
plus Kconfig / esp32_sys.h / test wiring.

Verdict: changes requested. One blocker (data corruption on large cards in the IDF≥6 path),
plus a resource-leak edge case, a docs rendering break, an SMP concurrency concern, and a test
robustness fix.

---

Summary table

#	Severity	Area	Issue
1	🔴 Blocker	IDF≥6 blockdev	64-bit byte address truncated to size_t → wrong sector on cards ≥ 4 GiB
2	🟠 Should-fix	sdcard_open	Resource leak if error-tuple allocation raises before enif_release_resource
3	🟠 Should-fix	docs	{important} admonition swallows the whole new section; nested fences break MyST
4	🟠 Should-fix	SMP	Busy spinlock held across blocking SD transaction
5	🟠 Should-fix	test	Last-sector roundtrip leaves card corrupted if an assertion fails mid-test

Confirmed clean: the a11ecaeb refactor is behavior-preserving; the IDF≥6 ops->read/ops->write
argument shapes match the esp_blockdev contract; free(dev->card) after ops->release is not
a double-free (sdmmc_release_blockdev only frees the wrapper, not the card).

---

1. 🔴 Blocker — IDF≥6 blockdev path truncates the sector byte address to 32 bits

sdcard_blockdev_read /
sdcard_blockdev_write (lines 532–551)
compute a 64-bit byte address and hand it to the ESP-IDF block-device wrapper:

return dev->blockdev->ops->read(dev->blockdev, dst, dev->sector_size,
    (uint64_t) sector * dev->sector_size, dev->sector_size);

The address math is correct here, but inside ESP-IDF v6
(components/sdmmc/sdmmc_blockdev.c) the helper that converts the byte address back to a
sector number casts the 64-bit address to size_t before dividing:

size_t start_sector_num = (size_t) addr / sector_size;
size_t last_byte_addr   = (size_t) (addr + data_len - 1);

On 32-bit ESP32 targets size_t is 32-bit, so any byte address ≥ 4 GiB wraps modulo 2³². With
512-byte sectors that is any sector ≥ 8,388,608 — i.e. roughly the top half of any card larger
than ~4 GiB. A read/write to a high sector silently lands on a low sector (e.g. byte address
0x1_0000_0000 → sector 0). For a "raw imaging / custom filesystem" feature this is silent data
corruption. (ESP-IDF commit a959111 only widened the disk_size calculation; the read/write
truncation is still present.)

The blockdev wrapper adds no value here — it just forwards to sdmmc_read_sectors /
sdmmc_write_sectors, which the IDF<6 branch already calls directly and which take a size_t
sector number (no byte-address overflow). Simplest correct fix: drop the blockdev path entirely
and call the sector APIs on all IDF versions. This also removes the ops->release lifecycle
ambiguity.

--- a/src/platforms/esp32/components/avm_builtins/storage_nif.c
+++ b/src/platforms/esp32/components/avm_builtins/storage_nif.c
@@
-#include <soc/soc_caps.h>
-#if ESP_IDF_VERSION_MAJOR >= 6 && defined(CONFIG_AVM_ENABLE_RAW_SDCARD_NIFS)
-#include <esp_blockdev.h>
-#endif
+#include <soc/soc_caps.h>
@@ struct SDCardBlockDevice
     sdmmc_card_t *card;
     sdspi_dev_handle_t spi_handle;
-#if ESP_IDF_VERSION_MAJOR >= 6
-    esp_blockdev_handle_t blockdev;
-#endif
 };
@@ static esp_err_t sdcard_blockdev_read(struct SDCardBlockDevice *dev, void *dst, size_t sector)
 {
-#if ESP_IDF_VERSION_MAJOR >= 6
-    return dev->blockdev->ops->read(dev->blockdev, dst, dev->sector_size,
-        (uint64_t) sector * dev->sector_size, dev->sector_size);
-#else
     return sdmmc_read_sectors(dev->card, dst, sector, 1);
-#endif
 }
@@ static esp_err_t sdcard_blockdev_write(
 {
-#if ESP_IDF_VERSION_MAJOR >= 6
-    return dev->blockdev->ops->write(
-        dev->blockdev, src, (uint64_t) sector * dev->sector_size, dev->sector_size);
-#else
     return sdmmc_write_sectors(dev->card, src, sector, 1);
-#endif
 }
@@ static esp_err_t sdcard_blockdev_close(struct SDCardBlockDevice *dev)
     esp_err_t ret = ESP_OK;
-
-#if ESP_IDF_VERSION_MAJOR >= 6
-    if (dev->blockdev) {
-        dev->blockdev->ops->release(dev->blockdev);
-        dev->blockdev = NULL;
-    }
-#endif
 
     if (dev->interface == SDCardSDSPI) {
@@ static esp_err_t sdcard_blockdev_init(struct SDCardConfig *cfg, struct SDCardBlockDevice *dev)
     dev->sector_size = dev->card->csd.sector_size;
     dev->sector_count = dev->card->csd.capacity;
-
-#if ESP_IDF_VERSION_MAJOR >= 6
-    err = sdmmc_get_blockdev(dev->card, &dev->blockdev);
-    if (UNLIKELY(err != ESP_OK)) {
-        dev->blockdev = NULL;
-        sdcard_blockdev_close(dev);
-        return err;
-    }
-#endif
 
     return ESP_OK;
 }

…and drop the now-unused initializer in nif_esp_sdcard_open:

@@ nif_esp_sdcard_open
     dev->sector_count = 0;
-#if ESP_IDF_VERSION_MAJOR >= 6
-    dev->blockdev = NULL;
-#endif

---

2. 🟠 Should-fix — resource leak in nif_esp_sdcard_open if the error tuple raises

nif_esp_sdcard_open (lines 697–716)
builds the error tuple before releasing the resource on the init-failure path:

esp_err_t err = sdcard_blockdev_init(&cfg, dev);

term return_term;
if (UNLIKELY(err != ESP_OK)) {
    return_term = make_esp_error_tuple(err, ctx);   // can RAISE_ERROR(OUT_OF_MEMORY) ...
} else {
    ...
}
enif_release_resource(dev);                          // ... so this never runs

make_esp_error_tuple calls memory_ensure_free and RAISE_ERROR(OUT_OF_MEMORY_ATOM) on failure
(line 99–103). If that fires, the enif_alloc_resource reference is leaked. (nif_esp_mount
deliberately frees before raising for the same reason.) Release before allocating the tuple:

@@ nif_esp_sdcard_open
     esp_err_t err = sdcard_blockdev_init(&cfg, dev);
 
-    term return_term;
     if (UNLIKELY(err != ESP_OK)) {
-        return_term = make_esp_error_tuple(err, ctx);
-    } else {
-        dev->open = true;
-        if (UNLIKELY(memory_ensure_free(ctx, TUPLE_SIZE(2) + TERM_BOXED_RESOURCE_SIZE)
-                != MEMORY_GC_OK)) {
-            enif_release_resource(dev);
-            RAISE_ERROR(OUT_OF_MEMORY_ATOM);
-        }
-        term dev_term = term_from_resource(dev, &ctx->heap);
-        return_term = term_alloc_tuple(2, &ctx->heap);
-        term_put_tuple_element(return_term, 0, OK_ATOM);
-        term_put_tuple_element(return_term, 1, dev_term);
+        enif_release_resource(dev);
+        return make_esp_error_tuple(err, ctx);
     }
-    enif_release_resource(dev); // decrement refcount after enif_alloc_resource
+
+    dev->open = true;
+    if (UNLIKELY(memory_ensure_free(ctx, TUPLE_SIZE(2) + TERM_BOXED_RESOURCE_SIZE)
+            != MEMORY_GC_OK)) {
+        enif_release_resource(dev); // dtor closes the now-open card
+        RAISE_ERROR(OUT_OF_MEMORY_ATOM);
+    }
+    term dev_term = term_from_resource(dev, &ctx->heap);
+    term return_term = term_alloc_tuple(2, &ctx->heap);
+    term_put_tuple_element(return_term, 0, OK_ATOM);
+    term_put_tuple_element(return_term, 1, dev_term);
+    enif_release_resource(dev); // decrement refcount after enif_alloc_resource
 
     return return_term;

The post-open=true OOM path stays correct: releasing the resource invokes the dtor, which closes
the card.

---

3. 🟠 Should-fix — docs: the new section is trapped inside an {important} admonition

In doc/src/programmers-guide.md (~line 1382) the {important}
block opens before "Remember to properly unmount…" and the closing ``` only appears after
the new "Raw SD card block access" section's {warning}. So the entire new section — including the
```erlang and ```{warning} blocks, which use the same 3-backtick fence length — is nested
inside the admonition and will break MyST/Sphinx rendering. Close the admonition right after its one
sentence and remove the trailing extra fence:

 ```{important}
 Remember to properly unmount any mounted filesystems before powering off or resetting the device to prevent data corruption.
+```
 
 #### Raw SD card block access
@@
 filesystem you intend to keep using.

-```

Restart and Sleep

---

## 4. 🟠 Should-fix — busy spinlock held across the blocking SD transaction

`SMP_LOCK(dev)` is held across `sdcard_blockdev_read` (line 740), `sdcard_blockdev_write`
(line 796) and the host/card teardown in `sdcard_blockdev_close` (lines 834/879). `SMP_LOCK` is an
atomic **busy-wait** spinlock (`src/libAtomVM/smp.h`). The author already flagged this in a comment
("a mutex would be preferable for larger transfers").

It's not a data race, but if two schedulers ever touch the same `sdcard()` resource the contending
core busy-spins for the full, unbounded duration of an SD transaction (program/erase latency can be
many ms). Since NIFs already block their scheduler thread synchronously, a blocking FreeRTOS mutex
is the right primitive for this resource. Suggested approach (apply to read/write/info/close only;
leave the mounted-FS spinlock alone):

```diff
+#include <freertos/FreeRTOS.h>
+#include <freertos/semphr.h>
+
+#ifndef AVM_NO_SMP
+#define SDCARD_LOCK_INIT(dev) ((dev)->lock = xSemaphoreCreateMutex())
+#define SDCARD_LOCK_OK(dev)   ((dev)->lock != NULL)
+#define SDCARD_LOCK(dev)      xSemaphoreTake((dev)->lock, portMAX_DELAY)
+#define SDCARD_UNLOCK(dev)    xSemaphoreGive((dev)->lock)
+#define SDCARD_LOCK_DESTROY(dev) do { if ((dev)->lock) vSemaphoreDelete((dev)->lock); } while (0)
+#else
+#define SDCARD_LOCK_INIT(dev)
+#define SDCARD_LOCK_OK(dev) true
+#define SDCARD_LOCK(dev)
+#define SDCARD_UNLOCK(dev)
+#define SDCARD_LOCK_DESTROY(dev)
+#endif

 struct SDCardBlockDevice
 {
 #ifndef AVM_NO_SMP
-    SpinLock lock;
+    SemaphoreHandle_t lock;
 #endif

…with SDCARD_LOCK_INIT(dev) + a SDCARD_LOCK_OK check in nif_esp_sdcard_open,
SDCARD_LOCK/UNLOCK replacing SMP_LOCK/UNLOCK in read/write/info/close, and
SDCARD_LOCK_DESTROY(dev) in the dtor. This is a larger change than the others; acceptable to defer
if multi-scheduler sharing of a single SD resource is considered out of scope, but it should be
called out explicitly rather than left as a silent spin.

---

5. 🟠 Should-fix — test can corrupt the real card on a mid-test failure

test_write_roundtrip writes a
pattern to the last sector and only restores the original after the read-back assertion
passes. If the read-back (or anything before the restore) fails, the last sector is left
overwritten — and the last sector can hold meaningful data (e.g. backup GPT). Wrap the mutation in
try ... after so the original is always restored:

     LastSector = SectorCount - 1,
     {ok, Original} = esp:sdcard_read(SDCard, LastSector),
     Pattern = make_pattern(SectorSize),
-    ok = esp:sdcard_write(SDCard, LastSector, Pattern),
-    {ok, Pattern} = esp:sdcard_read(SDCard, LastSector),
-    ok = esp:sdcard_write(SDCard, LastSector, Original),
+    try
+        ok = esp:sdcard_write(SDCard, LastSector, Pattern),
+        {ok, Pattern} = esp:sdcard_read(SDCard, LastSector),
+        ok
+    after
+        _ = esp:sdcard_write(SDCard, LastSector, Original)
+    end,
     {ok, Original} = esp:sdcard_read(SDCard, LastSector),
     ok.

---

Confirmed correct / non-issues

• a11ecaeb refactor — sdcard_config_from_source centralizes the exact "sdmmc"/"sdspi"
  parsing that previously lived inline in nif_esp_mount, with identical badarg behavior. FAT mount
  path unchanged.
• blockdev op signatures — ops->read(dev,dst,dst_size,src_addr,len) (5 args) and
  ops->write(dev,src,dst_addr,len) (4 args) match the esp_blockdev_ops_t contract; the asymmetry
  is correct (not a bug). (Mooted by fix 1.)
• card lifecycle — sdmmc_get_blockdev stores the caller-owned sdmmc_card_t* in ctx;
  sdmmc_release_blockdev frees only the wrapper. So free(dev->card) after release is correct, not
  a double-free.
• nif_esp_sdcard_read GC safety — the destination binary is created after a sufficient
  memory_ensure_free; on read error the unreferenced binary may be collected by the
  make_esp_error_tuple GC but is no longer needed, and on success no GC occurs between creation and
  return. Safe.
• sdcard_blockdev_init cleanup — partial host/device/card init failures unwind locally; the
  only leak is the OOM-before-release case in finding 2.
• open flag gating — read/write/info/close all re-check dev->open under the lock, so
  use-after-close and double-close are handled (return badarg / are idempotent).