[PATCH 1/3] usb: gadget: clarify usage of USB_GADGET_DELAYED_STATUS

[PATCH 1/3] usb: gadget: clarify usage of USB_GADGET_DELAYED_STATUS

[andrey.konovalov]

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>
---
 include/linux/usb/composite.h | 8 ++++++++
 include/linux/usb/gadget.h    | 9 +++++++++
 2 files changed, 17 insertions(+)

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.)
  *
-- 
2.25.1

[PATCH 2/3] usb: raw-gadget: return USB_GADGET_DELAYED_STATUS from setup()

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@gmail.com>

Return USB_GADGET_DELAYED_STATUS from the setup() callback for 0-length
transfers as a workaround to stop some UDC drivers (e.g. dwc3) from
automatically proceeding with the status stage.

This workaround should be removed once all UDC drivers are fixed to
always delay the status stage until a response is queued to EP0.

Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
---
 drivers/usb/gadget/legacy/raw_gadget.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index e549022642e5..b9ecc55a2ce2 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -25,6 +25,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/ch11.h>
 #include <linux/usb/gadget.h>
+#include <linux/usb/composite.h>
 
 #include <uapi/linux/usb/raw_gadget.h>
 
@@ -363,6 +364,16 @@ static int gadget_setup(struct usb_gadget *gadget,
 out_unlock:
 	spin_unlock_irqrestore(&dev->lock, flags);
 out:
+	if (ret == 0 && ctrl->wLength == 0) {
+		/*
+		 * Return USB_GADGET_DELAYED_STATUS as a workaround to stop
+		 * some UDC drivers (e.g. dwc3) from automatically proceeding
+		 * with the status stage for 0-length transfers.
+		 * Should be removed once all UDC drivers are fixed to always
+		 * delay the status stage until a response is queued to EP0.
+		 */
+		return USB_GADGET_DELAYED_STATUS;
+	}
 	return ret;
 }
 
-- 
2.25.1

[PATCH 3/3] usb: gadgetfs: return USB_GADGET_DELAYED_STATUS from setup()

[andrey.konovalov]

From: Andrey Konovalov <andreyknvl@gmail.com>

Return USB_GADGET_DELAYED_STATUS from the setup() callback for 0-length
transfers as a workaround to stop some UDC drivers (e.g. dwc3) from
automatically proceeding with the status stage.

This workaround should be removed once all UDC drivers are fixed to
always delay the status stage until a response is queued to EP0.

Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
---
 drivers/usb/gadget/legacy/inode.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 28249d0bf062..154bbf578ba2 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -31,6 +31,7 @@
 
 #include <linux/usb/gadgetfs.h>
 #include <linux/usb/gadget.h>
+#include <linux/usb/composite.h>
 
 
 /*
@@ -241,6 +242,7 @@ static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
 #define xprintk(d,level,fmt,args...) \
 	printk(level "%s: " fmt , shortname , ## args)
 
+#undef DBG
 #ifdef DEBUG
 #define DBG(dev,fmt,args...) \
 	xprintk(dev , KERN_DEBUG , fmt , ## args)
@@ -256,8 +258,10 @@ static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
 	do { } while (0)
 #endif /* DEBUG */
 
+#undef ERROR
 #define ERROR(dev,fmt,args...) \
 	xprintk(dev , KERN_ERR , fmt , ## args)
+#undef INFO
 #define INFO(dev,fmt,args...) \
 	xprintk(dev , KERN_INFO , fmt , ## args)
 
@@ -1511,7 +1515,16 @@ gadgetfs_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 			event->u.setup = *ctrl;
 			ep0_readable (dev);
 			spin_unlock (&dev->lock);
-			return 0;
+			/*
+			 * Return USB_GADGET_DELAYED_STATUS as a workaround to
+			 * stop some UDC drivers (e.g. dwc3) from automatically
+			 * proceeding with the status stage for 0-length
+			 * transfers.
+			 * Should be removed once all UDC drivers are fixed to
+			 * always delay the status stage until a response is
+			 * queued to EP0.
+			 */
+			return w_length == 0 ? USB_GADGET_DELAYED_STATUS : 0;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH 1/3] usb: gadget: clarify usage of USB_GADGET_DELAYED_STATUS

[Alan Stern]

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>

Re: [PATCH 3/3] usb: gadgetfs: return USB_GADGET_DELAYED_STATUS from setup()

[Alan Stern]

On Mon, Aug 28, 2023 at 04:10:32AM +0200, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@gmail.com>
> 
> Return USB_GADGET_DELAYED_STATUS from the setup() callback for 0-length
> transfers as a workaround to stop some UDC drivers (e.g. dwc3) from
> automatically proceeding with the status stage.
> 
> This workaround should be removed once all UDC drivers are fixed to
> always delay the status stage until a response is queued to EP0.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
> ---
>  drivers/usb/gadget/legacy/inode.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 28249d0bf062..154bbf578ba2 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
> @@ -31,6 +31,7 @@
>  
>  #include <linux/usb/gadgetfs.h>
>  #include <linux/usb/gadget.h>
> +#include <linux/usb/composite.h>

Add:  /* for USB_GADGET_DELAYED_STATUS */

>  
>  
>  /*
> @@ -241,6 +242,7 @@ static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
>  #define xprintk(d,level,fmt,args...) \
>  	printk(level "%s: " fmt , shortname , ## args)
>  
> +#undef DBG
>  #ifdef DEBUG
>  #define DBG(dev,fmt,args...) \
>  	xprintk(dev , KERN_DEBUG , fmt , ## args)
> @@ -256,8 +258,10 @@ static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
>  	do { } while (0)
>  #endif /* DEBUG */
>  
> +#undef ERROR
>  #define ERROR(dev,fmt,args...) \
>  	xprintk(dev , KERN_ERR , fmt , ## args)
> +#undef INFO

Please move these #undef lines up, just after the new #include.  And 
add a comment explaining briefly why they are needed.

Aside from these changes,

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 3/3] usb: gadgetfs: return USB_GADGET_DELAYED_STATUS from setup()

[Andrey Konovalov]

On Mon, Aug 28, 2023 at 4:52 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Aug 28, 2023 at 04:10:32AM +0200, andrey.konovalov@linux.dev wrote:
> > From: Andrey Konovalov <andreyknvl@gmail.com>
> >
> > Return USB_GADGET_DELAYED_STATUS from the setup() callback for 0-length
> > transfers as a workaround to stop some UDC drivers (e.g. dwc3) from
> > automatically proceeding with the status stage.
> >
> > This workaround should be removed once all UDC drivers are fixed to
> > always delay the status stage until a response is queued to EP0.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@gmail.com>
> > ---
> >  drivers/usb/gadget/legacy/inode.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> > index 28249d0bf062..154bbf578ba2 100644
> > --- a/drivers/usb/gadget/legacy/inode.c
> > +++ b/drivers/usb/gadget/legacy/inode.c
> > @@ -31,6 +31,7 @@
> >
> >  #include <linux/usb/gadgetfs.h>
> >  #include <linux/usb/gadget.h>
> > +#include <linux/usb/composite.h>
>
> Add:  /* for USB_GADGET_DELAYED_STATUS */

Will do in v2.

> > +#undef ERROR
> >  #define ERROR(dev,fmt,args...) \
> >       xprintk(dev , KERN_ERR , fmt , ## args)
> > +#undef INFO
>
> Please move these #undef lines up, just after the new #include.  And
> add a comment explaining briefly why they are needed.

Will do in v2.

>
> Aside from these changes,
>
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Thank you, Alan!