| Message ID | 5c2913d70556b03c9bb1893c6941e8ece04934b0.1693188390.git.andreyknvl@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] usb: gadget: clarify usage of USB_GADGET_DELAYED_STATUS | expand |
On Mon, Aug 28, 2023 at 04:10:30AM +0200, andrey.konovalov@linux.dev wrote: > From: Andrey Konovalov <andreyknvl@gmail.com> > > USB_GADGET_DELAYED_STATUS was introduced in commit 1b9ba000177e ("usb: > gadget: composite: Allow function drivers to pause control transfers"). > It was initially intended for the composite framework to allow delaying > completing the status stage of a SET_CONFIGURATION request until all > functions are ready. > > Unfortunately, that commit had an unintended side-effect of returning > USB_GADGET_DELAYED_STATUS from the ->setup() call of the composite > framework gadget driver. > > As a result of this and the incomplete documentation, some UDC drivers > started relying on USB_GADGET_DELAYED_STATUS to decide when to avoid > autocompleting the status stage for 0-length control transfers. dwc3 was > the first in commit 5bdb1dcc6330 ("usb: dwc3: ep0: handle delayed_status > again"). And a number of other UDC drivers followed later, probably > relying on the dwc3 behavior as a reference. > > Unfortunately, this violated the interface between the UDC and the > gadget driver for 0-length control transfers: the UDC driver must only > proceed with the status stage for a 0-length control transfer once the > gadget driver queued a response to EP0. > > As a result, a few gadget drivers are partially broken when used with > a UDC that only delays the status stage for 0-length transfers when > USB_GADGET_DELAYED_STATUS is returned from the setup() callback. > > This includes Raw Gadget and GadgetFS. For FunctionFS, a workaround was > added in commit 946ef68ad4e4 ("usb: gadget: ffs: Let setup() return > USB_GADGET_DELAYED_STATUS") and commit 4d644abf2569 ("usb: gadget: f_fs: > Only return delayed status when len is 0"). > > The proper solution to this issue would be to contain > USB_GADGET_DELAYED_STATUS within the composite framework and make all > UDC drivers to not complete the status stage for 0-length requests on > their own. > > Unfortunately, there is quite a few UDC drivers that need to get fixed > and the required changes for some of them are not trivial. > > For now, update the comments to clarify that USB_GADGET_DELAYED_STATUS > must not be used by the UDC drivers. > > The following two commits also add workarounds to Raw Gadget and GadgetFS > to make them compatible with the broken UDC drivers until they are fixed. > > Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com> > --- Acked-by: Alan Stern <stern@rowland.harvard.edu>
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h index 07531c4f4350..1d2cf6a070ac 100644 --- a/include/linux/usb/composite.h +++ b/include/linux/usb/composite.h @@ -35,6 +35,14 @@ * are ready. The control transfer will then be kept from completing till * all the function drivers that requested for USB_GADGET_DELAYED_STAUS * invoke usb_composite_setup_continue(). + * + * NOTE: USB_GADGET_DELAYED_STATUS must not be used in UDC drivers: they + * must delay completing the status stage for 0-length control transfers + * regardless of the whether USB_GADGET_DELAYED_STATUS is returned from + * the gadget driver's setup() callback. + * Currently, a number of UDC drivers rely on USB_GADGET_DELAYED_STATUS, + * which is a bug. These drivers must be fixed and USB_GADGET_DELAYED_STATUS + * must be contained within the composite framework. */ #define USB_GADGET_DELAYED_STATUS 0x7fff /* Impossibly large value */ diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 75bda0783395..6532beb587b1 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -711,6 +711,15 @@ static inline int usb_gadget_check_config(struct usb_gadget *gadget) * get_interface. Setting a configuration (or interface) is where * endpoints should be activated or (config 0) shut down. * + * The gadget driver's setup() callback does not have to queue a response to + * ep0 within the setup() call, the driver can do it after setup() returns. + * The UDC driver must wait until such a response is queued before proceeding + * with the data/status stages of the control transfer. + * + * NOTE: Currently, a number of UDC drivers rely on USB_GADGET_DELAYED_STATUS + * being returned from the setup() callback, which is a bug. See the comment + * next to USB_GADGET_DELAYED_STATUS for details. + * * (Note that only the default control endpoint is supported. Neither * hosts nor devices generally support control traffic except to ep0.) *