Message-ID: <CA+fCnZcJ0JHHY04By+PjYvofpefKtp1br63gJNsEL=J7n1_vCA@mail.gmail.com>
Date:   Wed, 23 Aug 2023 04:30:23 +0200
From:   Andrey Konovalov <andreyknvl@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Thinh Nguyen <thinh.nguyen@...opsys.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: dwc3: unusual handling of setup requests with wLength == 0

On Mon, Aug 21, 2023 at 7:25 PM Alan Stern <stern@...land.harvard.edu> wrote:
>
> > 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 started looking into reworking the UDC drivers to drop the special
case for USB_GADGET_DELAYED_STATUS, but this seems more complicated.

First, I noticed that some of the UDC drivers only expect to handle a
delayed Status stage for SET_CONFIGURATION requests. (Which is
reasonable, as they were developed assuming that only the composite
framework might request to delay the Status stage.) In particular,
dwc3, cdns2, and cdns3 set the gadget state to USB_STATE_CONFIGURED
when handling a delayed Status stage:

dwc3/ep0.c:136: usb_gadget_set_state(dwc->gadget, USB_STATE_CONFIGURED);
cdns3/cdns3-ep0.c:739: usb_gadget_set_state(&priv_dev->gadget,
USB_STATE_CONFIGURED);
gadget/udc/cdns2/cdns2-ep0.c:572: usb_gadget_set_state(&pdev->gadget,
USB_STATE_CONFIGURED);

So I believe an additional check for whether the request was indeed
SET_CONFIGURATION is required. (cdns2 and cdns3 also do other things
besides setting the state to USB_STATE_CONFIGURED, but it should be
possible to hide that under the same check.)

I also looked into how other UDC drivers change the gadget state to
USB_STATE_CONFIGURED:

1. isp1760, mtu3, and bdc immediately set USB_STATE_CONFIGURED once
they receive a SET_CONFIGURATION request, before calling ->setup() for
the gadget driver;
2. gr and mv_u3d do that after the ->setup() call;
3. tegra does it after the first non-control endpoint is enabled;
4. dwc3, cdns2, and cdns3 appear to not set USB_STATE_CONFIGURED if
the Status stage is not delayed;
5. dwc2, cdnsp, and all other UDCs don't set USB_STATE_CONFIGURED at all.

I'm guessing the UDCs in #4 and #5 expect the gadget driver to set
USB_STATE_CONFIGURED.

I see that the composite framework sets the gadget state to
USB_STATE_CONFIGURED even if some of the functions request a delayed
Status stage via USB_GADGET_DELAYED_STATUS. And GadgetFS also sets the
state to USB_STATE_CONFIGURED before delegating the SET_CONFIGURATION
request to userspace. However, Raw Gadget expects the userspace to
issue an ioctl that sets USB_STATE_CONFIGURED before completing the
delayed SET_CONFIGURATION request.

So I am wondering: when is proper time to set USB_STATE_CONFIGURED?
And should this be handled by the UDC driver or the gadget driver?

> > 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.

I think GadgetFS and Raw Gadget are the only two such drivers?

> 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!

Ack!

But I don't think it's within my capability to fix all UDCs,
considering the issues I mentioned above.

Powered by blists - more mailing lists