#714 usbStop does not resume threads suspended in synchronous calls to usbTransmit or usbReceive

[Austin Morton]

as per title, if you have a thread suspended in synchronous usb call and another thread invokes usbStop, the thread which is suspended will never be woken.

The following code works as a workaround in application code, but I believe this should be incorporated into usbStop itself.

    usbStop(usbp);

    osalSysLock();
    for (unsigned i = 1; i <= (unsigned)USB_MAX_ENDPOINTS; i++) {
        if (usbp->epc[i] != NULL) {
            if (usbp->epc[i]->in_state != NULL) {
                osalThreadResumeS(&usbp->epc[i]->in_state->thread, MSG_RESET);
            }
            if (usbp->epc[i]->out_state != NULL) {
                osalThreadResumeS(&usbp->epc[i]->out_state->thread, MSG_RESET);
            }
        }
    }
    osalOsRescheduleS();
    osalSysUnlock();

[Austin Morton]

It doesn't look like I can edit above, so I will fix the issue here:

EP0 should be included in the loop, as even though the default implementation in chibios does not use synchronous transfers on ep0, it is possible (and I actually do it) to override the ep0 config and implement synchronous transfers on ep0

    usbStop(usbp);

    osalSysLock();
    for (unsigned i = 0; i <= (unsigned)USB_MAX_ENDPOINTS; i++) {
        if (usbp->epc[i] != NULL) {
            if (usbp->epc[i]->in_state != NULL) {
                osalThreadResumeS(&usbp->epc[i]->in_state->thread, MSG_RESET);
            }
            if (usbp->epc[i]->out_state != NULL) {
                osalThreadResumeS(&usbp->epc[i]->out_state->thread, MSG_RESET);
            }
        }
    }
    osalOsRescheduleS();
    osalSysUnlock();

[Giovanni Di Sirio]

Hi,

Thanks for the patch, applied with a small change in order to wakeup threads atomically.

Giovanni

[Austin Morton]

Perfect, thanks. I have applied this patch to my project and can confirm it works as expected. Consider this issue closed.

Many thanks for your hard work on this wonderful project.

[Austin Morton]

however, I just realized that this will break builds with USB_USE_WAIT disabled.

Here is my proposed change.

I included one extra line to set epc to NULL, similar to usbDisableEndpointsI, just for sanity sake.

diff --git a/os/hal/src/usb.c b/os/hal/src/usb.c
index 8e557d1..d5ada42 100644
--- a/os/hal/src/usb.c
+++ b/os/hal/src/usb.c
@@ -323,6 +323,7 @@ void usbStop(USBDriver *usbp) {

   /* Resetting all ongoing synchronous operations.*/
   for (i = 0; i <= (unsigned)USB_MAX_ENDPOINTS; i++) {
+#if USB_USE_WAIT == TRUE
     if (usbp->epc[i] != NULL) {
       if (usbp->epc[i]->in_state != NULL) {
         osalThreadResumeI(&usbp->epc[i]->in_state->thread, MSG_RESET);
@@ -331,6 +332,8 @@ void usbStop(USBDriver *usbp) {
         osalThreadResumeI(&usbp->epc[i]->out_state->thread, MSG_RESET);
       }
     }
+#endif

+    usbp->epc[i] = NULL;
   }
   osalOsRescheduleS();
   osalSysUnlock();

[Giovanni Di Sirio]

I totally missed that too. Fixed in repo.

Giovanni