usb: device_next: wait for device to resume after remote wakeup signal #73362
Conversation
After the remote wake-up signal, wait a short time for the device to resume. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
If device is suspended, try to signal remote wakeup before buffer enqueue. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
This flag is not used anywhere, remove it. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
If enqueue of a notification failed, do not take semaphore and unref the buffer. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
jfischer-no
added
area: USB
| @@ -165,7 +166,19 @@ int usbd_wakeup_request(struct usbd_contex *const uds_ctx) | |||
| goto wakeup_request_error; | |||
| } | |||
|
|
|||
| k_sem_reset(&uds_ctx->rwup_sem); | |||
There was a problem hiding this comment.
Isn't this prone to race conditions where the host resumes the device before usbd_wakeup_request() obtains the usbd lock?
Using k_event that would have a bit that signifies not suspended would be immune to such race. Then the event would be set and cleared in usbd_status_suspended() with k_event_set_masked(&uds_ctx->events, value ? 0 : UDS_NOT_SUSPENDED_BIT, UDS_NOT_SUSPENDED_BIT); and the wait here would be changed to k_event_wait(&uds_ctx->events, UDS_NOT_SUSPENDED_BIT, false, resume_timeout);.
The name not suspended might not be the best, an alternative could be active but it might not be that good either. It would be important to make it not suspended because k_event_wait only allows to wait until an event bit gets set (there is no wait function for event bit to become cleared).
There was a problem hiding this comment.
Isn't this prone to race conditions where the host resumes the device before
usbd_wakeup_request()obtains the usbd lock?
Yes, e.g. if the suspend time is 0.
Using
k_eventthat would have a bit that signifiesnot suspendedwould be immune to such race. Then the event would be set and cleared inusbd_status_suspended()withk_event_set_masked(&uds_ctx->events, value ? 0 : UDS_NOT_SUSPENDED_BIT, UDS_NOT_SUSPENDED_BIT);and the wait here would be changed tok_event_wait(&uds_ctx->events, UDS_NOT_SUSPENDED_BIT, false, resume_timeout);.
I guessed you would suggest it, I will try it out.
The name
not suspendedmight not be the best, an alternative could beactivebut it might not be that good either. It would be important to make itnot suspendedbecausek_event_waitonly allows to wait until an event bit gets set (there is no wait function for event bit to become cleared).
|
|
||
| ret = usbd_wakeup_request(uds_ctx); | ||
| if (ret != 0) { | ||
| return ret; |
There was a problem hiding this comment.
Should we really be ending enqueue with an error if device is suspended (and wakeup fails or feature is cleared by the host)?
There was a problem hiding this comment.
Good question, if it is okay to enqueue in suspended state, then should we call remote wakeup signaling here or from the driver?
There was a problem hiding this comment.
Linux dwc2 gadget driver seems to allow queuing transfers only in active state (L0). If the device is in any other state (suspended), then it fails the queue with -EAGAIN. But it really depends on the gadget driver - some do fail when suspended, and some do not care.
|
@jfischer-no do you plan to come back to this PR? |
Yes. |
After the remote wake-up signal, wait a short time for the device to resume.
If device is suspended, try to signal remote wakeup before buffer enqueue.