| Message ID | 20181219092647.12397-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, 19 Dec 2018, 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 it might want to is to ---------------------------------------------^ Typo: missing "where" > 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 after > calling usb_gadget_giveback_request. The status of the request that was > just given back must be fed into usb_gadget_control_complete. To support > the explicit status stage, it might then check what stage the > usb_request was queued in, and send an ACK. I don't really understand that last sentence. Perhaps what you mean is that depending on the direction of the control transfer, the driver should either ACK the host's 0-length data packet (control-IN) or send a 0-length DATA1 packet (control-OUT)? > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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(+) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index af88b48c1cea..0a0ccd2b7639 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 > + * @explicit_status: true to delay status stage, false to complete here > + * @status: completion code of previously completed request > + * > + * This is called by device controller drivers after returning the completed > + * request back to the gadget layer, to either complete or delay the status > + * stage. > + */ > +void usb_gadget_control_complete(struct usb_gadget *gadget, > + unsigned int explicit_status, int status) > +{ > + struct usb_request *req; > + > + if (explicit_status || status) > + 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 = 0; Oops! I should have spotted this in the previous version, sorry. The implicit status-stage request should have its ->explicit_status set, so that we don't try to submit another status request when this one completes. Also, would it look better if the type of explicit_status was bool instead of unsigned int (both here and in the structure)? Either way, once this change is made: Acked-by: Alan Stern <stern@rowland.harvard.edu> > + 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..1e3a23637468 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; > + > + unsigned explicit_status:1; > }; > > /*-------------------------------------------------------------------------*/ > @@ -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, > + unsigned int explicit_status, int status); > + > +/*-------------------------------------------------------------------------*/ > + > /* utility to find endpoint by name */ > > extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g, >
On Wed, Dec 19, 2018 at 11:01:52AM -0500, Alan Stern wrote: > On Wed, 19 Dec 2018, 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 it might want to is to > ---------------------------------------------^ > Typo: missing "where" ack > > > 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 after > > calling usb_gadget_giveback_request. The status of the request that was > > just given back must be fed into usb_gadget_control_complete. To support > > the explicit status stage, it might then check what stage the > > usb_request was queued in, and send an ACK. > > I don't really understand that last sentence. Perhaps what you mean is > that depending on the direction of the control transfer, the driver > should either ACK the host's 0-length data packet (control-IN) or send > a 0-length DATA1 packet (control-OUT)? Yes, that is what I meant, though for MUSB I've only implemented and tested the control OUT case. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > 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(+) > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index af88b48c1cea..0a0ccd2b7639 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 > > + * @explicit_status: true to delay status stage, false to complete here > > + * @status: completion code of previously completed request > > + * > > + * This is called by device controller drivers after returning the completed > > + * request back to the gadget layer, to either complete or delay the status > > + * stage. > > + */ > > +void usb_gadget_control_complete(struct usb_gadget *gadget, > > + unsigned int explicit_status, int status) > > +{ > > + struct usb_request *req; > > + > > + if (explicit_status || status) > > + 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 = 0; > > Oops! I should have spotted this in the previous version, sorry. The > implicit status-stage request should have its ->explicit_status set, so > that we don't try to submit another status request when this one > completes. No, it's fine; the previous version had it set to 1 :) I didn't think it mattered, but the way you describe it makes it seem like it would matter depending on the UDC driver implementation, so I'll put it back in. > Also, would it look better if the type of explicit_status was bool > instead of unsigned int (both here and in the structure)? Yes, it would; I'll make the change. > Either way, once this change is made: > > Acked-by: Alan Stern <stern@rowland.harvard.edu> Thank you :) > > + 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..1e3a23637468 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; > > + > > + unsigned explicit_status:1; > > }; > > > > /*-------------------------------------------------------------------------*/ > > @@ -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, > > + unsigned int explicit_status, int status); > > + > > +/*-------------------------------------------------------------------------*/ > > + > > /* utility to find endpoint by name */ > > > > extern struct usb_ep *gadget_find_ep_by_name(struct usb_gadget *g, > > > Paul Elder
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index af88b48c1cea..0a0ccd2b7639 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 + * @explicit_status: true to delay status stage, false to complete here + * @status: completion code of previously completed request + * + * This is called by device controller drivers after returning the completed + * request back to the gadget layer, to either complete or delay the status + * stage. + */ +void usb_gadget_control_complete(struct usb_gadget *gadget, + unsigned int explicit_status, int status) +{ + struct usb_request *req; + + if (explicit_status || status) + 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 = 0; + 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..1e3a23637468 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; + + unsigned explicit_status:1; }; /*-------------------------------------------------------------------------*/ @@ -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, + unsigned int explicit_status, int status); + +/*-------------------------------------------------------------------------*/ + /* 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 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 after calling usb_gadget_giveback_request. The status of the request that was just given back must be fed into usb_gadget_control_complete. To support the explicit status stage, it might then check what stage the usb_request was queued in, and send an ACK. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> v1 Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- 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(+)