| Message ID | 20190109070856.27460-5-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: add mechanism to asynchronously validate data stage of ctrl out request | expand |
On Wed, 9 Jan 2019, Paul Elder wrote: > A usb gadget function driver may or may not want to delay the status > stage of a control OUT request. An instance where it might want to is to > asynchronously validate the data of a class-specific request. > > A function driver that wants an explicit status stage should set the > newly added explicit_status flag of the usb_request corresponding to the > data stage. Later on, the function driver can explicitly complete the > status stage by enqueueing a usb_request for ACK, or calling > usb_ep_set_halt() for STALL. > > To support both explicit and implicit status stages, a UDC driver must > call the newly added usb_gadget_control_complete function right before > calling usb_gadget_giveback_request. To support the explicit status > stage, it might then check what stage the usb_request was queued in, and > for control IN ACK the host's zero-length data packet, or for control > OUT send a zero-length DATA1 ACK packet. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> This looks good and has passed my tests so far. Can you check your uvc changes using dummy_hcd with the patch below? Alan Stern Index: usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c =================================================================== --- usb-4.x.orig/drivers/usb/gadget/udc/dummy_hcd.c +++ usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c @@ -88,6 +88,7 @@ struct dummy_ep { unsigned wedged:1; unsigned already_seen:1; unsigned setup_stage:1; + unsigned status_stage:1; unsigned stream_en:1; }; @@ -1037,7 +1038,7 @@ static void init_dummy_udc_hw(struct dum ep->ep.ops = &dummy_ep_ops; list_add_tail(&ep->ep.ep_list, &dum->gadget.ep_list); ep->halted = ep->wedged = ep->already_seen = - ep->setup_stage = 0; + ep->setup_stage = ep->status_stage = 0; usb_ep_set_maxpacket_limit(&ep->ep, ~0); ep->ep.max_streams = 16; ep->last_io = jiffies; @@ -1386,6 +1387,7 @@ static int transfer(struct dummy_hcd *du struct dummy *dum = dum_hcd->dum; struct dummy_request *req; int sent = 0; + bool is_ep0 = (ep == &dum->ep[0]); top: /* if there's no request queued, the device is NAKing; return */ @@ -1407,6 +1409,11 @@ top: * terminate reads. */ host_len = urb->transfer_buffer_length - urb->actual_length; + if (ep->status_stage) + host_len = 0; + else if (is_ep0 && host_len == 0) + ep->status_stage = 1; + dev_len = req->req.length - req->req.actual; len = min(host_len, dev_len); @@ -1471,6 +1478,12 @@ top: req->req.status = 0; } + /* Truncated control-IN => start the status stage */ + if (*status == 0 && is_ep0 && !ep->status_stage) { + ep->status_stage = 1; + *status = -EINPROGRESS; + } + /* * many requests terminate without a short packet. * send a zlp if demanded by flags. @@ -1486,6 +1499,8 @@ top: if (urb->transfer_flags & URB_ZERO_PACKET && !to_host) rescan = 1; + else if (is_ep0 && !ep->status_stage) + ep->status_stage = rescan = 1; else *status = 0; } @@ -1496,6 +1511,9 @@ top: list_del_init(&req->queue); spin_unlock(&dum->lock); + if (is_ep0) + usb_gadget_control_complete(&dum->gadget, + &req->req); usb_gadget_giveback_request(&ep->ep, &req->req); spin_lock(&dum->lock); @@ -1849,6 +1867,7 @@ restart: ep->already_seen = 1; if (ep == &dum->ep[0] && urb->error_count) { ep->setup_stage = 1; /* a new urb */ + ep->status_stage = 0; urb->error_count = 0; } if (ep->halted && !ep->setup_stage) {
On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > On Wed, 9 Jan 2019, Paul Elder wrote: > > > A usb gadget function driver may or may not want to delay the status > > stage of a control OUT request. An instance where it might want to is to > > asynchronously validate the data of a class-specific request. > > > > A function driver that wants an explicit status stage should set the > > newly added explicit_status flag of the usb_request corresponding to the > > data stage. Later on, the function driver can explicitly complete the > > status stage by enqueueing a usb_request for ACK, or calling > > usb_ep_set_halt() for STALL. > > > > To support both explicit and implicit status stages, a UDC driver must > > call the newly added usb_gadget_control_complete function right before > > calling usb_gadget_giveback_request. To support the explicit status > > stage, it might then check what stage the usb_request was queued in, and > > for control IN ACK the host's zero-length data packet, or for control > > OUT send a zero-length DATA1 ACK packet. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > This looks good and has passed my tests so far. Good! Thank you :) > Can you check your uvc > changes using dummy_hcd with the patch below? I'm not sure what to make of the test results. I get the same results with or without the patch. Which I guess makes sense... in dummy_queue, this is getting hit when the uvc function driver tries to complete the delayed status: req = usb_request_to_dummy_request(_req); if (!_req || !list_empty(&req->queue) || !_req->complete) return -EINVAL; So the delayed/explicit status stage is never completed, afaict. Paul
On Fri, 11 Jan 2019, Paul Elder wrote: > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > A usb gadget function driver may or may not want to delay the status > > > stage of a control OUT request. An instance where it might want to is to > > > asynchronously validate the data of a class-specific request. > > > > > > A function driver that wants an explicit status stage should set the > > > newly added explicit_status flag of the usb_request corresponding to the > > > data stage. Later on, the function driver can explicitly complete the > > > status stage by enqueueing a usb_request for ACK, or calling > > > usb_ep_set_halt() for STALL. > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > call the newly added usb_gadget_control_complete function right before > > > calling usb_gadget_giveback_request. To support the explicit status > > > stage, it might then check what stage the usb_request was queued in, and > > > for control IN ACK the host's zero-length data packet, or for control > > > OUT send a zero-length DATA1 ACK packet. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > This looks good and has passed my tests so far. > > Good! Thank you :) > > > Can you check your uvc > > changes using dummy_hcd with the patch below? > > I'm not sure what to make of the test results. I get the same results > with or without the patch. Which I guess makes sense... in dummy_queue, > this is getting hit when the uvc function driver tries to complete the > delayed status: > > req = usb_request_to_dummy_request(_req); > if (!_req || !list_empty(&req->queue) || !_req->complete) > return -EINVAL; > > So the delayed/explicit status stage is never completed, afaict. I presume you are hitting the !list_empty(&req->queue) test, yes? The other two tests are trivial. Triggering the !list_empty() test means the request has already been submitted and not yet completed. This probably indicates there is a bug in the uvc function driver code. Alan Stern
On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > On Fri, 11 Jan 2019, Paul Elder wrote: > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > > > A usb gadget function driver may or may not want to delay the status > > > > stage of a control OUT request. An instance where it might want to is to > > > > asynchronously validate the data of a class-specific request. > > > > > > > > A function driver that wants an explicit status stage should set the > > > > newly added explicit_status flag of the usb_request corresponding to the > > > > data stage. Later on, the function driver can explicitly complete the > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > usb_ep_set_halt() for STALL. > > > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > > call the newly added usb_gadget_control_complete function right before > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > stage, it might then check what stage the usb_request was queued in, and > > > > for control IN ACK the host's zero-length data packet, or for control > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > This looks good and has passed my tests so far. > > > > Good! Thank you :) > > > > > Can you check your uvc > > > changes using dummy_hcd with the patch below? > > > > I'm not sure what to make of the test results. I get the same results > > with or without the patch. Which I guess makes sense... in dummy_queue, > > this is getting hit when the uvc function driver tries to complete the > > delayed status: > > > > req = usb_request_to_dummy_request(_req); > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > return -EINVAL; > > > > So the delayed/explicit status stage is never completed, afaict. > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > other two tests are trivial. Yes, that is what's happening. > Triggering the !list_empty() test means the request has already been > submitted and not yet completed. This probably indicates there is a > bug in the uvc function driver code. The uvc function driver works with musb, though :/ I compared the sequence of calls to the uvc setup, completion handler, and status stage sending, and for some reason dummy_hcd, after an OUT setup-completion-status sequence, calls a completion-status-completion sequence, and then goes on the the next request. musb simply goes on to the next request after the setup-completion-status sequence. I commented out the paranoia block in dummy_timer, and dummy_hcd still does the extra completion, but it doesn't error out anymore. I doubt that's the/a solution though, especially since I get: [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). [ 22.624481] uvcvideo: Failed to initialize the device (-5). Not sure if that's a result of dummy_hcd not supporting isochronous transfers or not. I'm not sure where to continue investigating :/ Thanks, Paul
On Mon, 14 Jan 2019, Paul Elder wrote: > > > > Can you check your uvc > > > > changes using dummy_hcd with the patch below? > > > > > > I'm not sure what to make of the test results. I get the same results > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > this is getting hit when the uvc function driver tries to complete the > > > delayed status: > > > > > > req = usb_request_to_dummy_request(_req); > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > return -EINVAL; > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > other two tests are trivial. > > Yes, that is what's happening. > > > Triggering the !list_empty() test means the request has already been > > submitted and not yet completed. This probably indicates there is a > > bug in the uvc function driver code. > > The uvc function driver works with musb, though :/ > > I compared the sequence of calls to the uvc setup, completion handler, > and status stage sending, and for some reason dummy_hcd, after an OUT > setup-completion-status sequence, calls a completion-status-completion > sequence, and then goes on the the next request. musb simply goes on to > the next request after the setup-completion-status sequence. I don't quite understand. There's a control-OUT transfer, the setup, data, and status transactions all complete normally, and then what happens? What do you mean by "a completion-status-completion sequence"? A more detailed description would help. > I commented out the paranoia block in dummy_timer, and dummy_hcd still > does the extra completion, but it doesn't error out anymore. I doubt > that's the/a solution though, especially since I get: > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > Not sure if that's a result of dummy_hcd not supporting isochronous > transfers or not. > > I'm not sure where to continue investigating :/ Perhaps removing the "#if 0" protecting the dev_dbg line in dummy_queue() would provide some helpful output. Another thing to check would be if the "implement an emulated single-request FIFO" in dummy_queue() is causing trouble. There's no harm in replacing the long "if" condition with "if (0)". Alan Stern
On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote: > On Mon, 14 Jan 2019, Paul Elder wrote: > > > > > > Can you check your uvc > > > > > changes using dummy_hcd with the patch below? > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > this is getting hit when the uvc function driver tries to complete the > > > > delayed status: > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > return -EINVAL; > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > other two tests are trivial. > > > > Yes, that is what's happening. > > > > > Triggering the !list_empty() test means the request has already been > > > submitted and not yet completed. This probably indicates there is a > > > bug in the uvc function driver code. > > > > The uvc function driver works with musb, though :/ > > > > I compared the sequence of calls to the uvc setup, completion handler, > > and status stage sending, and for some reason dummy_hcd, after an OUT > > setup-completion-status sequence, calls a completion-status-completion > > sequence, and then goes on the the next request. musb simply goes on to > > the next request after the setup-completion-status sequence. > > I don't quite understand. There's a control-OUT transfer, the setup, > data, and status transactions all complete normally, and then what > happens? What do you mean by "a completion-status-completion > sequence"? A more detailed description would help. > I meant the functions (procedures) in the function driver, so the setup handler (uvc_function_setup), the completion handler (uvc_function_ep0_complete), and the status sender (uvc_send_response), although the last one actually sends the data stage for control IN. So after the status is sent on the uvc gadget driver's end, its completion handler is called again without the setup handler being called beforehand and I cant figure out why. > > I commented out the paranoia block in dummy_timer, and dummy_hcd still > > does the extra completion, but it doesn't error out anymore. I doubt > > that's the/a solution though, especially since I get: > > > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). > > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > > > Not sure if that's a result of dummy_hcd not supporting isochronous > > transfers or not. > > > > I'm not sure where to continue investigating :/ > > Perhaps removing the "#if 0" protecting the dev_dbg line in > dummy_queue() would provide some helpful output. It did, but didn't get me much farther :/ > Another thing to check would be if the "implement an emulated > single-request FIFO" in dummy_queue() is causing trouble. There's no > harm in replacing the long "if" condition with "if (0)". That didn't change anything. Although I did notice that the dummy_queue that calls the completion handler without the preceeding setup handler says that it's in the status stage (ep->status_stage == 1). Thanks, Paul
On Wed, 16 Jan 2019, Paul Elder wrote: > On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote: > > On Mon, 14 Jan 2019, Paul Elder wrote: > > > > > > > > Can you check your uvc > > > > > > changes using dummy_hcd with the patch below? > > > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > > this is getting hit when the uvc function driver tries to complete the > > > > > delayed status: > > > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > > return -EINVAL; > > > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > > other two tests are trivial. > > > > > > Yes, that is what's happening. > > > > > > > Triggering the !list_empty() test means the request has already been > > > > submitted and not yet completed. This probably indicates there is a > > > > bug in the uvc function driver code. > > > > > > The uvc function driver works with musb, though :/ > > > > > > I compared the sequence of calls to the uvc setup, completion handler, > > > and status stage sending, and for some reason dummy_hcd, after an OUT > > > setup-completion-status sequence, calls a completion-status-completion > > > sequence, and then goes on the the next request. musb simply goes on to > > > the next request after the setup-completion-status sequence. > > > > I don't quite understand. There's a control-OUT transfer, the setup, > > data, and status transactions all complete normally, and then what > > happens? What do you mean by "a completion-status-completion > > sequence"? A more detailed description would help. > > > > I meant the functions (procedures) in the function driver, so the setup > handler (uvc_function_setup), the completion handler > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > although the last one actually sends the data stage for control IN. > So after the status is sent on the uvc gadget driver's end, its > completion handler is called again without the setup handler being > called beforehand and I cant figure out why. Isn't this what you should expect? Every usb_request, if it is queued successfully, eventually gets a completion callback. That promise is made by every UDC driver; it's part of the gadget API. So for a control transfer with a data stage, you expect to have: Setup handler called Data-stage request submitted Data-stage request completion callback Status-stage request submitted Status-stage request completion callback Thus, two completion callbacks but only one setup callback. > > > I commented out the paranoia block in dummy_timer, and dummy_hcd still > > > does the extra completion, but it doesn't error out anymore. I doubt > > > that's the/a solution though, especially since I get: > > > > > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). > > > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > > > > > Not sure if that's a result of dummy_hcd not supporting isochronous > > > transfers or not. > > > > > > I'm not sure where to continue investigating :/ > > > > Perhaps removing the "#if 0" protecting the dev_dbg line in > > dummy_queue() would provide some helpful output. > > It did, but didn't get me much farther :/ > > > Another thing to check would be if the "implement an emulated > > single-request FIFO" in dummy_queue() is causing trouble. There's no > > harm in replacing the long "if" condition with "if (0)". > > That didn't change anything. > > Although I did notice that the dummy_queue that calls the completion > handler without the preceeding setup handler says that it's in the > status stage (ep->status_stage == 1). That is consistent with the events outlined above. Alan Stern
On Wed, Jan 16, 2019 at 10:06:53AM -0500, Alan Stern wrote: > On Wed, 16 Jan 2019, Paul Elder wrote: > > > On Mon, Jan 14, 2019 at 10:24:44AM -0500, Alan Stern wrote: > > > On Mon, 14 Jan 2019, Paul Elder wrote: > > > > > > > > > > Can you check your uvc > > > > > > > changes using dummy_hcd with the patch below? > > > > > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > > > this is getting hit when the uvc function driver tries to complete the > > > > > > delayed status: > > > > > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > > > return -EINVAL; > > > > > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > > > other two tests are trivial. > > > > > > > > Yes, that is what's happening. > > > > > > > > > Triggering the !list_empty() test means the request has already been > > > > > submitted and not yet completed. This probably indicates there is a > > > > > bug in the uvc function driver code. > > > > > > > > The uvc function driver works with musb, though :/ > > > > > > > > I compared the sequence of calls to the uvc setup, completion handler, > > > > and status stage sending, and for some reason dummy_hcd, after an OUT > > > > setup-completion-status sequence, calls a completion-status-completion > > > > sequence, and then goes on the the next request. musb simply goes on to > > > > the next request after the setup-completion-status sequence. > > > > > > I don't quite understand. There's a control-OUT transfer, the setup, > > > data, and status transactions all complete normally, and then what > > > happens? What do you mean by "a completion-status-completion > > > sequence"? A more detailed description would help. > > > > > > > I meant the functions (procedures) in the function driver, so the setup > > handler (uvc_function_setup), the completion handler > > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > > although the last one actually sends the data stage for control IN. > > So after the status is sent on the uvc gadget driver's end, its > > completion handler is called again without the setup handler being > > called beforehand and I cant figure out why. > > Isn't this what you should expect? Every usb_request, if it is queued > successfully, eventually gets a completion callback. That promise is > made by every UDC driver; it's part of the gadget API. So for a > control transfer with a data stage, you expect to have: > > Setup handler called > Data-stage request submitted > Data-stage request completion callback > Status-stage request submitted > Status-stage request completion callback > > Thus, two completion callbacks but only one setup callback. omg how did I not notice this :/ I guess I have to fix the uvc function driver so it works with that. musb doesn't call the status stage completion callback though; not that it does anything so it seems fine to me, but indeed the function driver has to be ready for it if it is called. > > > > I commented out the paranoia block in dummy_timer, and dummy_hcd still > > > > does the extra completion, but it doesn't error out anymore. I doubt > > > > that's the/a solution though, especially since I get: > > > > > > > > [ 22.616577] uvcvideo: Failed to query (129) UVC probe control : -75 (exp. 26). > > > > [ 22.624481] uvcvideo: Failed to initialize the device (-5). > > > > > > > > Not sure if that's a result of dummy_hcd not supporting isochronous > > > > transfers or not. > > > > > > > > I'm not sure where to continue investigating :/ > > > > > > Perhaps removing the "#if 0" protecting the dev_dbg line in > > > dummy_queue() would provide some helpful output. > > > > It did, but didn't get me much farther :/ > > > > > Another thing to check would be if the "implement an emulated > > > single-request FIFO" in dummy_queue() is causing trouble. There's no > > > harm in replacing the long "if" condition with "if (0)". > > > > That didn't change anything. > > > > Although I did notice that the dummy_queue that calls the completion > > handler without the preceeding setup handler says that it's in the > > status stage (ep->status_stage == 1). > > That is consistent with the events outlined above. Thanks, Paul
On Fri, 18 Jan 2019, Paul Elder wrote: > > > I meant the functions (procedures) in the function driver, so the setup > > > handler (uvc_function_setup), the completion handler > > > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > > > although the last one actually sends the data stage for control IN. > > > So after the status is sent on the uvc gadget driver's end, its > > > completion handler is called again without the setup handler being > > > called beforehand and I cant figure out why. > > > > Isn't this what you should expect? Every usb_request, if it is queued > > successfully, eventually gets a completion callback. That promise is > > made by every UDC driver; it's part of the gadget API. So for a > > control transfer with a data stage, you expect to have: > > > > Setup handler called > > Data-stage request submitted > > Data-stage request completion callback > > Status-stage request submitted > > Status-stage request completion callback > > > > Thus, two completion callbacks but only one setup callback. > > omg how did I not notice this :/ > > I guess I have to fix the uvc function driver so it works with that. > musb doesn't call the status stage completion callback though; not that > it does anything so it seems fine to me, but indeed the function driver > has to be ready for it if it is called. musb _has_ to call the status-stage completion callback. As just one reason, if the explicit_status flag isn't set then that callback is responsible for deallocating the status request. Without it, the status request will leak. Alan Stern
On Fri, Jan 18, 2019 at 11:52:57AM -0500, Alan Stern wrote: > On Fri, 18 Jan 2019, Paul Elder wrote: > > > > > I meant the functions (procedures) in the function driver, so the setup > > > > handler (uvc_function_setup), the completion handler > > > > (uvc_function_ep0_complete), and the status sender (uvc_send_response), > > > > although the last one actually sends the data stage for control IN. > > > > So after the status is sent on the uvc gadget driver's end, its > > > > completion handler is called again without the setup handler being > > > > called beforehand and I cant figure out why. > > > > > > Isn't this what you should expect? Every usb_request, if it is queued > > > successfully, eventually gets a completion callback. That promise is > > > made by every UDC driver; it's part of the gadget API. So for a > > > control transfer with a data stage, you expect to have: > > > > > > Setup handler called > > > Data-stage request submitted > > > Data-stage request completion callback > > > Status-stage request submitted > > > Status-stage request completion callback > > > > > > Thus, two completion callbacks but only one setup callback. > > > > omg how did I not notice this :/ > > > > I guess I have to fix the uvc function driver so it works with that. > > musb doesn't call the status stage completion callback though; not that > > it does anything so it seems fine to me, but indeed the function driver > > has to be ready for it if it is called. > > musb _has_ to call the status-stage completion callback. As just one > reason, if the explicit_status flag isn't set then that callback is > responsible for deallocating the status request. Without it, the > status request will leak. Ah, I see what you mean. I forgot about that because we reuse the request in uvc gadget. I'll have to add the status stage completion callback to musb too, then. Thanks, Paul
On Mon, 14 Jan 2019, Paul Elder wrote: > On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > > On Fri, 11 Jan 2019, Paul Elder wrote: > > > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > > > > > A usb gadget function driver may or may not want to delay the status > > > > > stage of a control OUT request. An instance where it might want to is to > > > > > asynchronously validate the data of a class-specific request. > > > > > > > > > > A function driver that wants an explicit status stage should set the > > > > > newly added explicit_status flag of the usb_request corresponding to the > > > > > data stage. Later on, the function driver can explicitly complete the > > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > > usb_ep_set_halt() for STALL. > > > > > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > > > call the newly added usb_gadget_control_complete function right before > > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > > stage, it might then check what stage the usb_request was queued in, and > > > > > for control IN ACK the host's zero-length data packet, or for control > > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > This looks good and has passed my tests so far. > > > > > > Good! Thank you :) > > > > > > > Can you check your uvc > > > > changes using dummy_hcd with the patch below? > > > > > > I'm not sure what to make of the test results. I get the same results > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > this is getting hit when the uvc function driver tries to complete the > > > delayed status: > > > > > > req = usb_request_to_dummy_request(_req); > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > return -EINVAL; > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > other two tests are trivial. > > Yes, that is what's happening. > > > Triggering the !list_empty() test means the request has already been > > submitted and not yet completed. This probably indicates there is a > > bug in the uvc function driver code. > > The uvc function driver works with musb, though :/ Did you ever figure out the reason for the "!list_empty(&req->queue)" error with dummy_hcd? Was it related to the confusion about completion callbacks for status requests? Interesting new question: How does your code in musb tell the difference between a 0-length data-stage reply to a control-IN transfer, and a status-stage request? Both would appear to the UDC driver as 0-length request submissions for ep0. Do you explicitly keep track of whether the data stage is pending? Alan Stern
On Wed, Jan 23, 2019 at 04:10:12PM -0500, Alan Stern wrote: > On Mon, 14 Jan 2019, Paul Elder wrote: > > > On Fri, Jan 11, 2019 at 10:50:11AM -0500, Alan Stern wrote: > > > On Fri, 11 Jan 2019, Paul Elder wrote: > > > > > > > On Wed, Jan 09, 2019 at 02:06:31PM -0500, Alan Stern wrote: > > > > > On Wed, 9 Jan 2019, Paul Elder wrote: > > > > > > > > > > > A usb gadget function driver may or may not want to delay the status > > > > > > stage of a control OUT request. An instance where it might want to is to > > > > > > asynchronously validate the data of a class-specific request. > > > > > > > > > > > > A function driver that wants an explicit status stage should set the > > > > > > newly added explicit_status flag of the usb_request corresponding to the > > > > > > data stage. Later on, the function driver can explicitly complete the > > > > > > status stage by enqueueing a usb_request for ACK, or calling > > > > > > usb_ep_set_halt() for STALL. > > > > > > > > > > > > To support both explicit and implicit status stages, a UDC driver must > > > > > > call the newly added usb_gadget_control_complete function right before > > > > > > calling usb_gadget_giveback_request. To support the explicit status > > > > > > stage, it might then check what stage the usb_request was queued in, and > > > > > > for control IN ACK the host's zero-length data packet, or for control > > > > > > OUT send a zero-length DATA1 ACK packet. > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> > > > > > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > > This looks good and has passed my tests so far. > > > > > > > > Good! Thank you :) > > > > > > > > > Can you check your uvc > > > > > changes using dummy_hcd with the patch below? > > > > > > > > I'm not sure what to make of the test results. I get the same results > > > > with or without the patch. Which I guess makes sense... in dummy_queue, > > > > this is getting hit when the uvc function driver tries to complete the > > > > delayed status: > > > > > > > > req = usb_request_to_dummy_request(_req); > > > > if (!_req || !list_empty(&req->queue) || !_req->complete) > > > > return -EINVAL; > > > > > > > > So the delayed/explicit status stage is never completed, afaict. > > > > > > I presume you are hitting the !list_empty(&req->queue) test, yes? The > > > other two tests are trivial. > > > > Yes, that is what's happening. > > > > > Triggering the !list_empty() test means the request has already been > > > submitted and not yet completed. This probably indicates there is a > > > bug in the uvc function driver code. > > > > The uvc function driver works with musb, though :/ > > Did you ever figure out the reason for the "!list_empty(&req->queue)" > error with dummy_hcd? Was it related to the confusion about completion > callbacks for status requests? Yeah, I'm pretty sure that's what was happening. With what I previously had the uvc function driver wasn't expecting a completion callback for the status stage but the OUT request flag was set so it just kept sending the data stage data to userspace and userspace kept calling the ioctl to send the status stage which kept calling the completion callback and so on, until the dummy_hcd timer triggered and the next request came in. > Interesting new question: How does your code in musb tell the > difference between a 0-length data-stage reply to a control-IN > transfer, and a status-stage request? Both would appear to the UDC > driver as 0-length request submissions for ep0. > Do you explicitly keep track of whether the data stage is pending? musb has a state machine to keep track of which stage it's in, so I just count whatever is queued during the status-IN stage as a status-stage request for control OUT requests. Now that you mention it, I don't actually check that the queued request's length is zero there... gotta fix that. Paul
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index af88b48c1cea..57b2c2550537 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -894,6 +894,40 @@ EXPORT_SYMBOL_GPL(usb_gadget_giveback_request); /* ------------------------------------------------------------------------- */ +/** + * usb_gadget_control_complete - complete the status stage of a control + * request, or delay it + * Context: in_interrupt() + * + * @gadget: gadget whose control request's status stage should be completed + * @request: usb request whose status stage should be completed + * + * This is called by device controller drivers before returning the completed + * request back to the gadget layer, to either complete or delay the status + * stage. It exits without doing anything if the request has a non-zero status, + * if it has zero length, or if its explicit_status flag is set. + */ +void usb_gadget_control_complete(struct usb_gadget *gadget, + struct usb_request *request) +{ + struct usb_request *req; + + if (request->explicit_status || request->status || !request->length) + return; + + /* Send an implicit status-stage request for ep0 */ + req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC); + if (req) { + req->length = 0; + req->explicit_status = 1; + req->complete = usb_ep_free_request; + usb_ep_queue(gadget->ep0, req, GFP_ATOMIC); + } +} +EXPORT_SYMBOL_GPL(usb_gadget_control_complete); + +/* ------------------------------------------------------------------------- */ + /** * gadget_find_ep_by_name - returns ep whose name is the same as sting passed * in second parameter or NULL if searched endpoint not found diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index e5cd84a0f84a..bf4f021ce139 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -73,6 +73,7 @@ struct usb_ep; * Note that for writes (IN transfers) some data bytes may still * reside in a device-side FIFO when the request is reported as * complete. + * @explicit_status: If true, delays the status stage * * These are allocated/freed through the endpoint they're used with. The * hardware's driver can add extra per-request data to the memory it returns, @@ -114,6 +115,8 @@ struct usb_request { int status; unsigned actual; + + bool explicit_status; }; /*-------------------------------------------------------------------------*/ @@ -850,6 +853,13 @@ extern void usb_gadget_giveback_request(struct usb_ep *ep, /*-------------------------------------------------------------------------*/ +/* utility to complete or delay status stage */ + +void usb_gadget_control_complete(struct usb_gadget *gadget, + struct usb_request *request); + +/*-------------------------------------------------------------------------*/ + /* utility to find endpoint by name */ extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g,
A usb gadget function driver may or may not want to delay the status stage of a control OUT request. An instance where it might want to is to asynchronously validate the data of a class-specific request. A function driver that wants an explicit status stage should set the newly added explicit_status flag of the usb_request corresponding to the data stage. Later on, the function driver can explicitly complete the status stage by enqueueing a usb_request for ACK, or calling usb_ep_set_halt() for STALL. To support both explicit and implicit status stages, a UDC driver must call the newly added usb_gadget_control_complete function right before calling usb_gadget_giveback_request. To support the explicit status stage, it might then check what stage the usb_request was queued in, and for control IN ACK the host's zero-length data packet, or for control OUT send a zero-length DATA1 ACK packet. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> v4 Acked-by: Alan Stern <stern@rowland.harvard.edu> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes from v4: - Change parameter of usb_gadget_control_complete to simply take a usb_request - Make usb_gadget_control_complete do nothing if the request has no length (ie. no data stage) Changes from v3: - More specific in commit message about what to do for udc driver acking - Set explicit_status in usb_gadget_control_complete - Make explicit_status type bool Changes from v2: Add status parameter to usb_gadget_control_complete, so that a usb_request is not queued if the status of the just given back request is nonzero. Changes from v1: Complete change of API. Now we use a flag that should be set in the usb_request that is queued for the data stage to signal to the UDC that we want to delay the status stage (as opposed to setting a flag in the UDC itself, that persists across all requests). We now also provide a function for UDC drivers to very easily allow implicit status stages, to mitigate the need to convert all function drivers to this new API at once, and to make it easier for UDC drivers to convert. drivers/usb/gadget/udc/core.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/usb/gadget.h | 10 ++++++++++ 2 files changed, 44 insertions(+)