From 1d3c722b7d3b4ada25f248166cf056bc9155278f Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 12 Jun 2024 15:42:18 +1000 Subject: [PATCH 1/3] usb: Fix race if transfers are submitted by a thread. The USB pending transfer flag was cleared before calling the completion callback, to allow the callback code to call submit_xfer() again. Unfortunately this isn't safe in a multi-threaded environment, as another thread may see the endpoint is available before the callback is done executing and submit a new transfer. Rather than adding extra locking, specifically treat the transfer as still pending if checked from another thread while the callback is executing. Closes #874 Signed-off-by: Angus Gratton --- micropython/usb/usb-device/manifest.py | 2 +- micropython/usb/usb-device/usb/device/core.py | 36 ++++++++++++++++--- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/micropython/usb/usb-device/manifest.py b/micropython/usb/usb-device/manifest.py index 0dfab932f..27c9aa88a 100644 --- a/micropython/usb/usb-device/manifest.py +++ b/micropython/usb/usb-device/manifest.py @@ -1,2 +1,2 @@ -metadata(version="0.1.0") +metadata(version="0.1.1") package("usb") diff --git a/micropython/usb/usb-device/usb/device/core.py b/micropython/usb/usb-device/usb/device/core.py index 08277b1f4..06f0f33ce 100644 --- a/micropython/usb/usb-device/usb/device/core.py +++ b/micropython/usb/usb-device/usb/device/core.py @@ -8,6 +8,14 @@ import machine import struct +try: + from _thread import get_ident +except ImportError: + + def get_ident(): + return 0 # Placeholder, for no threading support + + _EP_IN_FLAG = const(1 << 7) # USB descriptor types @@ -76,6 +84,8 @@ def __init__(self): self._itfs = {} # Mapping from interface number to interface object, set by init() self._eps = {} # Mapping from endpoint address to interface object, set by _open_cb() self._ep_cbs = {} # Mapping from endpoint address to Optional[xfer callback] + self._cb_thread = None # Thread currently running endpoint callback + self._cb_ep = None # Endpoint number currently running callback self._usbd = machine.USBDevice() # low-level API def init(self, *itfs, **kwargs): @@ -298,7 +308,7 @@ def _submit_xfer(self, ep_addr, data, done_cb=None): # that function for documentation about the possible parameter values. if ep_addr not in self._eps: raise ValueError("ep_addr") - if self._ep_cbs[ep_addr]: + if self._xfer_pending(ep_addr): raise RuntimeError("xfer_pending") # USBDevice callback may be called immediately, before Python execution @@ -308,12 +318,25 @@ def _submit_xfer(self, ep_addr, data, done_cb=None): self._ep_cbs[ep_addr] = done_cb or True return self._usbd.submit_xfer(ep_addr, data) + def _xfer_pending(self, ep_addr): + # Singleton function to return True if transfer is pending on this endpoint. + # + # Generally, drivers should call Interface.xfer_pending() instead. See that + # function for more documentation. + return self._ep_cbs[ep_addr] or (self._cb_ep == ep_addr and self._cb_thread != get_ident()) + def _xfer_cb(self, ep_addr, result, xferred_bytes): # Singleton callback from TinyUSB custom class driver when a transfer completes. cb = self._ep_cbs.get(ep_addr, None) + self._cb_thread = get_ident() + self._cb_ep = ep_addr # Track while callback is running self._ep_cbs[ep_addr] = None - if callable(cb): - cb(ep_addr, result, xferred_bytes) + try: + # For a pending xfer, 'cb' should either a callback function or True (if no callback) + if callable(cb): + cb(ep_addr, result, xferred_bytes) + finally: + self._cb_ep = None def _control_xfer_cb(self, stage, request): # Singleton callback from TinyUSB custom class driver when a control @@ -528,7 +551,12 @@ def xfer_pending(self, ep_addr): # Return True if a transfer is already pending on ep_addr. # # Only one transfer can be submitted at a time. - return _dev and bool(_dev._ep_cbs[ep_addr]) + # + # The transfer is marked pending while a completion callback is running + # for that endpoint, unless this function is called from the callback + # itself. This makes it simple to submit a new transfer from the + # completion callback. + return _dev and _dev._xfer_pending(ep_addr) def submit_xfer(self, ep_addr, data, done_cb=None): # Submit a USB transfer (of any type except control) From 01f45c118f39610d8fcb2064d237b89ec5b81269 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 11 Jul 2024 11:23:23 +1000 Subject: [PATCH 2/3] usb: Add a note about buffer thread safety. This is to replace a commit which added locking here but caused some other problems. The idea behind the Buffer class is that a single producer can call pend_write() more than once and it's idempotent, however this is very complex to extend across multiple threads. Signed-off-by: Angus Gratton --- micropython/usb/README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/micropython/usb/README.md b/micropython/usb/README.md index 342a0a7e0..d4b975d12 100644 --- a/micropython/usb/README.md +++ b/micropython/usb/README.md @@ -134,3 +134,15 @@ USB MIDI devices in MicroPython. The example [midi_example.py](examples/device/midi_example.py) demonstrates how to create a simple MIDI device to send MIDI data to and from the USB host. + +### Limitations + +#### Buffer thread safety + +The internal Buffer class that's used by most of the USB device classes expects data +to be written to it (i.e. sent to the host) by only one thread. Bytes may be +lost from the USB transfers if more than one thread (or a thread and a callback) +try to write to the buffer simultaneously. + +If writing USB data from multiple sources, your code may need to add +synchronisation (i.e. locks). From c61ca51c6743a95fe8dd5b3345dca41355238271 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 11 Sep 2024 17:18:57 +1000 Subject: [PATCH 3/3] usb: Tidy up the description of TinyUSB callbacks. Signed-off-by: Angus Gratton --- micropython/usb/usb-device/usb/device/core.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/micropython/usb/usb-device/usb/device/core.py b/micropython/usb/usb-device/usb/device/core.py index 06f0f33ce..2d6790798 100644 --- a/micropython/usb/usb-device/usb/device/core.py +++ b/micropython/usb/usb-device/usb/device/core.py @@ -252,8 +252,8 @@ def active(self, *optional_value): return self._usbd.active(*optional_value) def _open_itf_cb(self, desc): - # Singleton callback from TinyUSB custom class driver, when USB host does - # Set Configuration. Called once per interface or IAD. + # Callback from TinyUSB lower layer, when USB host does Set + # Configuration. Called once per interface or IAD. # Note that even if the configuration descriptor contains an IAD, 'desc' # starts from the first interface descriptor in the IAD and not the IAD @@ -291,7 +291,7 @@ def _open_itf_cb(self, desc): itf.on_open() def _reset_cb(self): - # Callback when the USB device is reset by the host + # TinyUSB lower layer callback when the USB device is reset by the host # Allow interfaces to respond to the reset for itf in self._itfs.values(): @@ -302,7 +302,7 @@ def _reset_cb(self): self._ep_cbs = {} def _submit_xfer(self, ep_addr, data, done_cb=None): - # Singleton function to submit a USB transfer (of any type except control). + # Submit a USB transfer (of any type except control) to TinyUSB lower layer. # # Generally, drivers should call Interface.submit_xfer() instead. See # that function for documentation about the possible parameter values. @@ -319,27 +319,31 @@ def _submit_xfer(self, ep_addr, data, done_cb=None): return self._usbd.submit_xfer(ep_addr, data) def _xfer_pending(self, ep_addr): - # Singleton function to return True if transfer is pending on this endpoint. + # Returns True if a transfer is pending on this endpoint. # # Generally, drivers should call Interface.xfer_pending() instead. See that # function for more documentation. return self._ep_cbs[ep_addr] or (self._cb_ep == ep_addr and self._cb_thread != get_ident()) def _xfer_cb(self, ep_addr, result, xferred_bytes): - # Singleton callback from TinyUSB custom class driver when a transfer completes. + # Callback from TinyUSB lower layer when a transfer completes. cb = self._ep_cbs.get(ep_addr, None) self._cb_thread = get_ident() self._cb_ep = ep_addr # Track while callback is running self._ep_cbs[ep_addr] = None + + # In most cases, 'cb' is a callback function for the transfer. Can also be: + # - True (for a transfer with no callback) + # - None (TinyUSB callback arrived for invalid endpoint, or no transfer. + # Generally unlikely, but may happen in transient states.) try: - # For a pending xfer, 'cb' should either a callback function or True (if no callback) if callable(cb): cb(ep_addr, result, xferred_bytes) finally: self._cb_ep = None def _control_xfer_cb(self, stage, request): - # Singleton callback from TinyUSB custom class driver when a control + # Callback from TinyUSB lower layer when a control # transfer is in progress. # # stage determines appropriate responses (possible values