Message-ID: <45d9ef53-e2be-4740-a93a-d36f18a49b39@rowland.harvard.edu>
Date:   Fri, 18 Aug 2023 19:06:58 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     Andrey Konovalov <andreyknvl@...il.com>,
        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 Fri, Aug 18, 2023 at 07:49:27PM +0000, Thinh Nguyen wrote:
> On Thu, Aug 17, 2023, Alan Stern wrote:
> > On Fri, Aug 18, 2023 at 03:10:48AM +0000, Thinh Nguyen wrote:
> > > I was referring to the next request. It should not be processed until
> > > the first request is completed. Depending on the type of request, if
> > > there's a delayed_status, the dwc3 driver will not prepare for the
> > > Status stage and Setup stage (after status completion) to proceed to the
> > > _next_ ->setup callback.
> > > 
> > > My understanding from the described problem is that somehow dwc3
> > > processes the next request immediately without waiting for the raw
> > > gadget preparing the data stage.
> > 
> > Um.  This is one of the design flaws I mentioned: a new SETUP packet 
> > arriving before the old control transfer is finished.  The USB spec 
> > requires devices to accept the new SETUP packet and abort the old 
> > transfer.  So in this case, processing the next request immediately is 
> > the right thing to do.
> 
> No, as I've mentioned, from the gadget driver, it should not see the
> next ->setup until the first control transfer completion, regardless
> whether the transfer completed with error code due to abort or not.
> Everything should happen sequentially from the gadget driver. This
> should be handled in the dwc3 driver (though we missed a setup_pending
> status check in the data phase that needs to be fixed now that I look at
> it again).

Actually I agree with you.  When a new SETUP packet arrives before the 
old control transfer has finished, the UDC driver should cancel all 
pending requests and then invoke the ->setup() callback.  (I don't think 
there is a standard error code for the cancelled requests; net2280 seems 
to use -EPROTO whereas dummy-hcd seems to use -EOVERFLOW.)

> Perhaps the core design should be so that this synchronization does not
> depend on the udc driver implementation.

Do you mean the UDC core?  I don't see how it could get involved in 
managing control transfers.

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

> > > I was talking in context of 0-length transfer (albeit I forgot about the
> > > special case of control OUT doesn't have 3-stage).
> > > 
> > > If it's a vendor request 0-length transfer, without responding with
> > > USB_GADGET_DELAYED_STATUS, the dwc3 will proceed with preparing the
> > > status stage.
> > 
> > This may be a holdover from the early days of the Gadget subsystem.  My 
> > memory from back then isn't very good; I vaguely recall that the first 
> > UDC drivers would queue their automatic Status-stage requests if wLength 
> > was 0 and ->setup() returned 0 (which would explain why 
> > USB_GADGET_DELAYED_STATUS had to be invented).  Unless I'm completely 
> > confused, that's not how UDC drivers are supposed to act now.

I did a little checking.  The USB_GADGET_DELAYED_STATUS mechanism was 
introduced for use by the composite framework -- not in order to make a 
UDC driver work properly.

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

Still, I recommand that dwc3 not automatically queue a Status request 
when wLength is 0.  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.

Alan Stern

Powered by blists - more mailing lists