supervisor/filesystem: let alternative writers (BLE-FT, web workflow) bypass STA_PROTECT while they hold the lock#11048
Conversation
nordic boards build with CIRCUITPY_HASHLIB_MBEDTLS_ONLY=1 which pulls sha1.c / sha256.c / sha512.c / platform_util.c from lib/mbedtls but ci_fetch_deps.py never fetched the submodule, breaking custom builds. Drive-by; not strictly part of the adafruit#10972 fix, but needed for the custom build-board workflow to produce a CLUE UF2 for verification.
… bypass STA_PROTECT while they hold the lock Since adafruit#10659, filesystem_is_writable_by_python() returns false on any USB-device-capable board after main.c boot (CONCURRENT_WRITE_PROTECTED and USB_WRITABLE are both set). vfs_fat_diskio.c:disk_ioctl(IOCTL_STATUS) calls this function and reports STA_PROTECT when it returns false, so f_open(FA_WRITE) returns FR_WRITE_PROTECTED. This affects every non-USB-MSC writer that calls f_open directly through FatFS: - BLE File Transfer (supervisor/shared/bluetooth/file_transfer.c): all WRITE/MOVE/MKDIR/DELETE commands fail with STATUS_ERROR_READONLY even when the board has no host on the bus. This is the original adafruit#10972 repro on CLUE running from battery. - Web workflow PUT/POST/MOVE/DELETE (supervisor/shared/web_workflow): same path, web-editor adafruit#460/adafruit#506 surfaces this as a read-only error. - storage.remount(readonly=False): can take the lock but then can't actually write while the lock is held. Each of these already calls filesystem_lock() to claim the blockdev LOCKED flag before writing. USB MSC does NOT go through filesystem_lock; it grabs LOCKED directly via blockdev_lock() inside tud_msc_is_writable_cb. So filesystem_lock() is exactly the right place to grant temporary write permission via IGNORE_WRITE_PROTECTION, mirroring the pattern main.c uses around boot.py. After this change: - filesystem_lock() is the single source of truth for 'I'm the local writer right now'. Holders can f_open(FA_WRITE) and operate normally. - USB MSC continues to be mutually excluded via LOCKED; it never sets IGNORE_WRITE_PROTECTION because it doesn't go through filesystem_lock. - _is_writable_by_python and _is_writable_by_usb are unchanged; the symmetric mutex from adafruit#10659 is preserved. - Python direct open(FA_WRITE) without a lock still requires storage.disable_usb_drive() or storage.remount(readonly=False) — same as current behavior. Fixes adafruit#10972
|
I am not sure about this fix. I am looking at the current semantics related to remounting in boot.py. |
|
Thanks for taking a look. Here's my analysis of the boot.py remount interaction — please tell me which case is bothering you so I can think about it more concretely.
I verified this on hardware: The IGNORE flag is only set inside A few specific interactions I checked:
If you'd prefer a different shape entirely — e.g. dropping IGNORE and instead having (About to do an empirical BLE-FT WRITE test on a battery-powered CLUE — happy to defer if you want me to wait on a shape change first. The change works in principle; whether it's the right shape is your call.) |
|
I didn't mean for you to check something. I was just trying to understand the current semantics myself. Currently (as of 10.0.0, not after the regression), are web workflow and BLE workflow supposed to be able to write files if the host can also write files? web and BLE workflow are transactional, so we know when they are done. USB MSC is not transactional, so it's unsafe to use it simultaneously with other workflows. Are Is there a race condition on startup? Maybe wifi or BLE workflow would connect before the USB enumeration finished. |
|
Thanks for the framing — those are the right questions. Auditing the tree to answer all three concretely: Q2: Are
|
dhalbert
left a comment
There was a problem hiding this comment.
I tested this with BLE:
- on a Feather nRF52840. With FileGlider, I was able to edit files, when the device was not also connected as a USB device. code.circuitpython.org over BLE was flakier: I got REPL output but not input.
- on a Metro S3. Again, I was able to use FileGlider to edit files. I could not pair or connect with a Linux desktop.
I tested with WiFi on a Metro ESP32-S3. I was able to edit files when the USB connection was power-only.
So I think this fixes the read-only problem we were seeing. BLE workflows are still quite flaky via code.circuitpython.org, but that is a different problem.
@melissa thanks for setting piclaw to work on this.
|
No worries. It was blocking on CP Code Editor work. |
Fixes #10972.
Summary
After PR #10659, alternative filesystem writers (BLE File Transfer, web workflow,
supervisor_workflow_*) can take theMP_BLOCKDEV_FLAG_LOCKEDblockdev mutex but cannot actually write through FatFS, becausedisk_ioctl(IOCTL_STATUS)callsfilesystem_is_writable_by_python()which returns false on any USB-device-capable board aftermain.cboot — sof_open(FA_WRITE)returnsFR_WRITE_PROTECTED.This 11-line patch makes
filesystem_lock()setMP_BLOCKDEV_FLAG_IGNORE_WRITE_PROTECTIONwhile the lock is held, andfilesystem_unlock()clear it. This mirrors the patternmain.calready uses aroundboot.pyexecution. USB MSC continues to mutex viaLOCKED(it takes the lock throughblockdev_lock()directly, not throughfilesystem_lock(), soIGNOREis never set on its behalf).Why this shape
I prototyped an alternative first: adding
(LOCKED == 0)as a fourth clause infilesystem_is_writable_by_python(). That worked for the "Pythonopen(W)on battery" case I tested, but:filesystem_lock()(setting LOCKED=1) before callingf_open(FA_WRITE). Once LOCKED=1, the new(LOCKED == 0)clause is also false, andf_openstill fails withFR_WRITE_PROTECTED. So that variant fixes a different case from what the issue reports.open(W)succeeds without taking any lock, so a concurrent BLE-FT or web workflowfilesystem_lock()would also succeed (LOCKED was 0), and two writers operate on FatFS at the same time.This patch avoids both problems:
filesystem_is_writable_by_python()is byte-identical to its pre-BLE File Transfer writes blocked as STATUS_ERROR_READONLY when no USB host attached #10972 form (verified viaobjdump— sameand #0xb0 / sub #0x30codegen on Cortex-M4). No regression to writability semantics.filesystem_lock()are explicitly granted write permission for the duration of the critical section.IGNORE_WRITE_PROTECTIONis a flag specifically intended for this — already used bymain.cto allow boot.py'sboot_out.txtwrite.LOCKEDmutex (viablockdev_lock(), notfilesystem_lock()), so the symmetric host-vs-local mutex from Fix filesystem writability from USB #10659 is preserved.open(W)without a lock remains blocked in the default state, so users still needstorage.disable_usb_drive()orstorage.remount(readonly=False)for direct Python writes. Same behavior as today; this PR is not changing that escape hatch.Verification
Built v2 firmware via the
Build board (custom)workflow on my fork:Hardware-verified on CLUE nRF52840 (10.3.0-alpha.2-25-ga8924a8e):
mount.readonlyTrueopen(W)OSError(30)storage.disable_usb_drive()in boot.py —mount.readonlyFalsestorage.disable_usb_drive()in boot.py — Pythonopen(W)+os.rename+os.sync+ readback after resetBinary inspection of
firmware.elfconfirms the patch is in the build:filesystem_is_writable_by_pythonis byte-identical to its pre-#10972 form (3-clause check folded intoand #0xb0 / cmp #0x30).What this patch does NOT do
I'm intentionally keeping this PR minimal. Two adjacent improvements suggested in #10972 are not in scope here:
The issue suggests distinguishing lock-contention from read-only by introducing
STATUS_ERROR_BUSY = 0x06(or similar) in BLE-FT protocol. Reasonable UX improvement, but orthogonal — the lock-busy case will be rare in practice oncefilesystem_lock()is the right mutex. Happy to do this as a follow-up if you'd like.I have not exercised the actual BLE-FT write path end-to-end on hardware from this PR's environment (Pi USB-host setup suppresses BLE-FT advertising when CIRCUITPY_USB_DEVICE is connected; getting into discovery mode reliably needs a double-tap reset I don't have remote-control access for). The change is in shared code with a small, well-typed surface, and I've verified the binary matches the source and the regression-free path empirically. If a reviewer has a CLUE-on-battery rig and
@adafruit/ble-file-transfer-js(web-editor over BLE) handy, that's the one path I couldn't directly cover.Test plan suggestion for reviewer
On a BLE-FT-capable board (CLUE, Feather Sense, etc.) running on battery (no USB host):
0x05(STATUS_ERROR_READONLY)For web workflow (any WiFi board with CIRCUITPY_WEB_WORKFLOW=1) on USB power adapter (no host):
curlPUT /fs/path/to/file.txtcc @dhalbert — this is a direct follow-up on the regression introduced by #10659; would appreciate your eye on whether the IGNORE pattern is the right shape here, or if you'd prefer a different approach (e.g. explicit per-writer flag).