Re: dwc3: unusual handling of setup requests with wLength == 0

[Thinh Nguyen]

On Mon, Aug 21, 2023, Alan Stern wrote:
> On Mon, Aug 21, 2023 at 06:13:05PM +0200, Andrey Konovalov wrote:
> > Out of curiosity, I also did some digging around USB_GADGET_DELAYED_STATUS.
> > 
> > 1. USB_GADGET_DELAYED_STATUS was introduced in 1b9ba000177 ("usb:
> > gadget: composite: Allow function drivers to pause control
> > transfers"). It looks like it was indeed initially intended for the
> > composite framework, as nor that commit nor the directly following
> > commits use USB_GADGET_DELAYED_STATUS in any UDC drivers. However,
> > this commit had an unintended (?) side-effect of returning
> > USB_GADGET_DELAYED_STATUS from the ->setup() call of the composite
> > framework gadget driver.
> > 
> > 2. In 5bdb1dcc6330 ("usb: dwc3: ep0: handle delayed_status again"),
> > the dwc3 driver was the first one to start relying on
> > USB_GADGET_DELAYED_STATUS to decide when to avoid auto-completing the
> > Status stage (+Sebastian). The commit description mentions some
> > previous rework of the driver that made it lose the ability of handle
> > delayed Status stage handling, but I couldn't figure out the exact
> > commit it refers to.
> > 
> > 3. Following that, a few other UDC drivers also started using
> > USB_GADGET_DELAYED_STATUS, potentially using the dwc3 behavior as a
> > reference.
> > 
> > Interestingly, in 946ef68ad4e4 ("usb: gadget: ffs: Let setup() return
> > USB_GADGET_DELAYED_STATUS"), the FunctionFS composite driver had to
> > add USB_GADGET_DELAYED_STATUS to specifically avoid failures when dwc3
> > is used. This is the same "fix" that worked for me with Raw Gadget.
> > 
> > Right now dwc2, dwc3, mtu3, cdns3, bdc, and renesas have special
> > handling for USB_GADGET_DELAYED_STATUS. Surprisingly, dwc2 works with
> > Raw Gadget as is nevertheless. dwc3 fails as I described. For the
> > others, I don't have the hardware to test them.
> > 
> > I guess the proper solution would be to contain
> > USB_GADGET_DELAYED_STATUS within the composite framework and make all
> > UDCs not to handle the Status stage on their own. However, this would
> > require a collaborative effort from the maintainers of the UDC drivers
> > I mentioned.
> 
> Most if not all of the UDC drivers you listed are actively maintained.  
> It shouldn't be too hard to get people to remove the special treatment 
> of USB_GADGET_DELAYED_STATUS in them.
> 
> The necessary changes should be pretty small: Whenever wLength is 0, 
> treat any non-negative return value from ->setup() as if it were 
> USB_GADGET_DELAYED_STATUS.  This would probably end up shrinking the 
> UDC driver code a little.  :-)

I don't think a simple removal of a few lines in dwc3 and all will be
fine. This will impact some users (including us and the tools we use for
validation). Also, this will require quite a bit of reviewing and
testing.

Nonetheless, this is noted.

> 
> > An alternative would to declare USB_GADGET_DELAYED_STATUS to be usable
> > outside of the composite framework and leave everything as is
> > otherwise (but change Raw Gadget to return USB_GADGET_DELAYED_STATUS).
> > The downside is the discrepancy in the interface of different UDCs
> > (some require USB_GADGET_DELAYED_STATUS, others imply), but perhaps
> > it's not that bad provided that this discrepancy is documented.
> 
> This alternative is less desirable, because the legacy gadgets (some of 
> which don't use the composite framework) may not be compatible with it.
> 
> And as a matter of general principle, allowing UDC drivers to start 
> automatically send Status replies to 0-length control transfers is a 
> step in the wrong direction.  What we really should focus our energy on 
> is getting them to _stop_ sending automatic Status replies to 
> non-zero-length control transfers!
> 

Thanks,
Thinh