Message-ID: <CA+fCnZfekbQ2upu085g6RPO+_bRKHcnxqfh8Cvsbt8XXz-DEwA@mail.gmail.com>
Date:   Sat, 19 Aug 2023 03:54:48 +0200
From:   Andrey Konovalov <andreyknvl@...il.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     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

[I will skip the discussion about the racy or aborted SETUP requests
and the potential design flaws in the UDC subsystem: this is not
what's happening in my case. ]

On Sat, Aug 19, 2023 at 2:07 AM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> > > > One question is why Andrey is observing a new ->setup() callback
> > > > happening so soon?  The host is supposed to allow a fairly long time for
> > > > standard control requests to complete.  If the userspace component of
> > > > the Raw Gadget takes too long to act, the transfer could time out and be
> > > > cancelled on the host.  But "too long" means several seconds -- is that
> > > > really what's going on here?

No, this is not what's happening. All the requests get processed
relatively quickly, there are no big delays.

> > > As I noted initially, it should not happen so I'm not sure what's really
> > > the problem here. Granted that the setup_pending check for data phase is
> > > missing in dwc3 driver, there should not be a problem unless the host
> > > actually aborted a control transfer. Also, there should be no data phase
> > > for wlength=0 even for IN direction if we go through the composite
> > > layer. So, I doubt that's what happening here.
> > >
> > > Perhaps Andrey can clarify.
> >
> > My impression from his initial email was that something different was
> > happening.  It sounded like his ->setup() callback was invoked with
> > wLength = 0, but before the Raw Gadget driver was ready to queue a
> > response request, the UDC driver sent an automatic status, at which
> > point the host sent another SETUP packet.  So the gadget driver got two
> > ->setup() callbacks in a row with no chance to do anything in between.

Precisely!

> What else should the gadget driver do? There's no data stage for
> wLength=0.

Quoting one of the Alan's responses:

> But if the SETUP packet's wLength value is 0 then when the gadget driver
> is ready, it queues a 0-length IN request which will act as the Status
> stage.  In this situation the UDC does not automatically create a
> Status-stage request.
>
> Note that the gadget driver is allowed to queue its various requests
> either while the ->setup() callback is running or after it has returned.

So there's no Data stage indeed, but the problem is that dwc3 proceeds
with the Status stage by itself without waiting for an (empty) request
to be queued by the gadget driver.

> > > For dwc3, it's been like this since the beginning that it automatically
> > > queues the status upon host request. USB_GADGET_DELAYED_STATUS was
> > > introduced to support scenarios where the device may need a longer time
> > > to process the specific request (such as SET_CONFIGURATION).
> >
> > It's hard to be sure, but that may be the cause of Andrey's problem.

Yes, I believe this is exactly the problem.

> > He
> > mentioned something about the Raw Gadget's ->setup() routine returning
> > USB_GADGET_DELAYED_STATUS when the problem occurred, but I think he
> > meant that it did this for the second callback, not the first one.

Right!

I just tried changing Raw Gadget to return USB_GADGET_DELAYED_STATUS
from ->setup() whenever wLength == 0, and the enumeration now works
properly. However, I don't think this hack is the proper solution. I
think dwc3 should just do what other UDC drivers do and do not
autocomplete the Status stage for wLength == 0 requests.

> > Still, I recommand that dwc3 not automatically queue a Status request
> > when wLength is 0.

Yes, I believe this should fix the problem that I'm having.

> > Gadget drivers don't expect UDC drivers to handle
> > this for them.  For example, look at the composite_setup() function in
> > composite.c.  It does this:
> >
> >       /* respond with data transfer before status phase? */
> >       if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
> >               req->length = value;
> >               req->context = cdev;
> >               req->zero = value < w_length;
> >               value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> >
> > Clearly it doesn't want the UDC driver to queue a request for it, no
> > matter what wLength or value is set to.
> >
>
> dwc3 parse the SETUP data and determine whether it's a 3-state or
> 2-stage control transfer. If wLength > 0, then it must be a 3-stage
> control transfer. The dwc3 driver would not queue the status immediately
> until the data stage is completed.
>
> To enforce the gadget driver to manually queue the status would take
> some effort to ensure all the UDC driver comply to it.

I think most (or all except dwc3?) UDC drivers already comply with
this behavior. At the very least, Dummy UDC, dwc2, musb, chipidea, and
net2280 drivers work with Raw Gadget, so they must be complying. dwc3
is the first UDC I tested that caused a problem.

> We would also need to update the composite framework.

I would expect that the composite framework already takes this
behavior into account. I suspect that it's the dwc3 that just happens
to work with the composite framework, as the composite framework
quickly queues an empty response to a wLength == 0 request (which gets
apparently ignored by dwc3) before the next ->setup() calls happens.

Thank you!

Powered by blists - more mailing lists