Message-ID: <20230818194922.ys26zrqc4pocqq7q@synopsys.com>
Date:   Fri, 18 Aug 2023 19:49:27 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Alan Stern <stern@...land.harvard.edu>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        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 Thu, Aug 17, 2023, Alan Stern wrote:
> On Fri, Aug 18, 2023 at 03:10:48AM +0000, Thinh Nguyen wrote:
> > On Thu, Aug 17, 2023, Alan Stern wrote:
> > > On Fri, Aug 18, 2023 at 01:08:19AM +0000, Thinh Nguyen wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Aug 18, 2023, Andrey Konovalov wrote:
> > > > > Hi Alan and Thinh,
> > > > > 
> > > > > I have been testing Raw Gadget with the dwc3 UDC driver and stumbled
> > > > > upon an issue related to how dwc3 handles setup requests with wLength
> > > > > == 0.
> > > > > 
> > > > > When running a simple Raw Gadget-based keyboard emulator [1],
> > > > > everything works as expected until the point when the host sends a
> > > > > SET_CONFIGURATION request, which has wLength == 0.
> > > > > 
> > > > > For setup requests with wLength != 0, just like the other UDC drivers
> > > > > I tested, dwc3 calls the gadget driver's ->setup() callback and then
> > > > > waits until the gadget driver queues an URB to EP0 as a response.
> > > > 
> > > > For the lack of better term, can we use "request" or "usb_request"
> > > > instead of URB for gadget side, I get confused with the host side
> > > > whenever we mention URB.
> > > > 
> > > > > 
> > > > > However, for a setup request with wLength == 0, dwc3 does not wait
> > > > > until the gadget driver queues an URB to ack the transfer. It appears
> > > > > that dwc3 just acks the request internally and then proceeds with
> > > > > calling the ->setup() callback for the next request received from the
> > > > 
> > > > It depends on the bRequest. It should not proceed to ->setup() unless
> > > > the gadget driver already setups the request for it.
> > > 
> > > Let's see if I understand what you're saying.  Some control transfers 
> > > are handled directly by the UDC driver (things like SET_ADDRESS or 
> > > CLEAR_HALT).  For these transfers, the ->setup() callback is not invoked 
> > > and the gadget driver is completely unaware of them.  But for all other 
> > > control transfers, the ->setup() callback _is_ invoked.
> > > 
> > > Is that what you meant?
> > 
> > That's not what I meant.
> > 
> > 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).

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

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

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

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

> 
> > > (IMO that automatic action is a design flaw; the UDC driver should wait 
> > > for the gadget driver to explictly queue a 0-length request or a STALL 
> > > instead of doing it automatically.)
> > 
> > Would every UDC has this capability? I recalled some aren't capable of
> > delayed_status.
> 
> In those cases the UDC driver would just have to do the best it can.  
> Very few modern USB device controllers should have this limitation.
> 
> > > (Another design flaw is that this design doesn't specify what should 
> > > happen if the UDC receives another SETUP packet from the host before the 
> > > Status stage completes.  By sending another SETUP packet, the host is 
> > > indicating that the earlier control transfer has been aborted.  
> > > Presumably the UDC driver will complete all the outstanding requests 
> > > with an error status, but there's a potential race in the gadget driver 
> > > between queuing a request for the first transfer and executing the 
> > > ->setup() callback for the second transfer.)
> > 
> > If there's another SETUP packet coming while there's a pending control
> > transfer, for dwc3 UDC, the pending control TRB should be completed with
> > a Setup_pending status indicating aborted control transfer for dwc3
> > driver to handle that.
> 
> Right.  The difficulty doesn't involve the communication between the HCD 
> and the UDC hardware; it involves the communication between the UDC 
> driver and the gadget driver.  Somehow they need to synchronize so that 
> when the gadget driver queues a usb_request, the UDC driver can tell 
> whether the request was meant for the earlier aborted control transfer 
> or the new active one.  This can matter if the gadget driver has a 
> separate control thread (a work routine or a kthread, for example) that 
> could be queuing requests while the ->setup() callback is running.
> 

Perhaps this can be improved and enforced from the core. At the moment,
it should not be a problem for gadget driver with dwc3 driver (with a
minor fix due to missing check).

BR,
Thinh

Powered by blists - more mailing lists