Skip to content

usbh: class transfer completions are delivered during unplug teardown when an unmount callback pumps tuh_task() #4502

Open

Description

@hathach

Deferred from #4009, which covers only the MSC Reset Recovery completion. Found by Codex review of 7173d6069 and confirmed by source inspection; the same paths read on master 53c1d1473. Not reproduced on hardware or in a unit test. Line numbers below are at that revision, except removing, which exists on the #4009 head only.

usbh_task_run() (src/host/usbh.c:861-968) has no nesting guard, and usbh_driver_set_config_complete() (usbh.c:2397, #4425) accepts callbacks that run tuh_task(). When tuh_umount_cb() (or another class unmount callback) pumps tuh_task() during unplug teardown, queued class-driver transfer completions are still delivered to the class driver. The XFER_COMPLETE dispatch (usbh.c:780-783) drops a completion only when !dev->connected. It does not test the removing flag that #4009 adds (usbh.c:127), which is set at the start of remove_device_tree() before any callback.

Consequence 1: mounted device

msch_xfer_cb() (src/class/msc/msc_host.c:463-519) has no device-state guard. A queued bulk data or status completion pumped from tuh_umount_cb() submits the next stage (:478, :480, :488, :490) or calls the application's command callback (:500) during teardown. Minor impact: the CSW is real.

Consequence 2: composite CDC+MSC unplugged during MSC Read Capacity, before mount

  1. remove_device_tree() skips tuh_umount_cb() for an unmounted device (usbh.c:1799).
  2. Drivers close in order CDC, then MSC (usbh.c:256-278). cdch_close() calls tuh_cdc_umount_cb() unconditionally (src/class/cdc/cdc_host.c:655-660).
  3. If that callback pumps tuh_task(), the queued Read Capacity CSW runs config_read_capacity_complete() (msc_host.c:675-695): it sets mounted, calls tuh_msc_mount_cb(), then usbh_driver_set_config_complete().
  4. The guard there (usbh.c:2398, dev->configured && dev_addr == enumerating_daddr) still passes, because usbh_device_close() runs only after the driver close loop (:1812).
  5. If MSC is the last interface, dev->mounted is set and tuh_mount_cb() fires (:2421-2422) during teardown, with no matching tuh_umount_cb(). The application's view of the device is wrong until the address is reused.

The trigger needs an application callback that drives tuh_task(), which #4425 treats as supported. How common that is for unmount callbacks is unknown.

Proposed fix

In usbh.c, treat a removing device as gone:

  • At the XFER_COMPLETE dispatch test dev && dev->connected && !dev->removing and drop the event (continue the task loop, do not return false).
  • Add && !dev->removing to usbh_driver_set_config_complete()'s guard so tuh_mount_cb() cannot fire during teardown.
  • Add a composite CDC+MSC unit test for consequence 2.

This depends on #4009's subtree-wide removing marking.

Deferred because it is generic usbh class-dispatch behaviour for every driver and pre-existing, outside #4009's Reset Recovery scope.

Related: #4433 — same family (class work after a callback that can run tuh_task()), but covers the mount callback, stale class state and same-address replacement, not completion dispatch during teardown.
Related: #4425 — closed fix of the mount-callback case in usbh_driver_set_config_complete(); its guard is what consequence 2 passes through.

Originating PR: #4009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Prio 📌follow-upDeferred work from a PR or investigation

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions