Message-ID: <20230818031045.wovf5tj2un7nwf72@synopsys.com>
Date:   Fri, 18 Aug 2023 03:10:48 +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 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.


> 
> > > host. This confuses Raw Gadget, as it does not expect to get a new
> > > ->setup() call before it explicitly acks the previous one by queuing
> > > an URB. As a result, the emulation fails.
> > 
> > If the host intent is to send a 3-stage control request with a 0-length
> > data packet, the gadget driver needs to return USB_GADGET_DELAYED_STATUS
> > to prepare a 0-length request. For SET_CONFIGURATION, we don't expect
> > a data phase, why should the gadget driver queue a 0-length data?
> 
> The USB-2 spec prohibits 3-stage control requests with wLength == 0 (see 
> sections 9.3.1 and 9.3.5).  Therefore the host's intent can never be to 
> send a 3-stage control request with a 0-length Data-stage packet.

Right. Forgot about that, but my point was about the sequential flow of
the control transfer.

> 
> > > I suspect this issue has not been observed with other gadget drivers,
> > > as they queue an URB immediately after receiving a ->setup() call:
> > > dwc3 appears to somehow correctly handle this internally even though
> > > it acks the transfer by itself. But the timings with Raw Gadget are
> > > different, as it requires userspace to ack the transfer. Sometimes
> > > though, the Raw Gadget-based emulator also manages to queue an URB
> > > before the next request is received from the host and the enumeration
> > > continues properly (until the next request with wLength == 0).
> > > 
> > > What do you think would be the best approach to deal with this?
> > 
> > The communication should be clearly defined. That is, the dwc3 needs to
> > know if this is a 3-stage or 2-stage control transfer. It knows about
> > the standard requests, but not the vendor/non-standard ones. If the raw
> > gadget defined some unknown OUT request, it needs to tell dwc3 whether
> > it should expect the data stage or not.
> 
> The communication _is_ clearly defined.  Here's how it works:
> 
> For control transfers that aren't handled directly by the UDC, the UDC 
> driver invokes the ->setup() callback and waits for the gadget driver to 
> queue a request.  If the SETUP packet's wLength value is > 0 then the 
> gadget driver queues an IN or OUT request (depending on the transfer's 
> direction) and the UDC waits for the host to transfer the Data stage 
> packets, completing the request.  After this happens, the UDC driver 
> automatically queues an internal 0-length request in the opposite 
> direction for the Status stage.  Data-stage transfers are not allowed to 
> span more than one usb_request.

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.

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

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

> 
> If the dwc3 UDC driver doesn't behave this way then it needs to be 
> fixed.
> 

BR,
Thinh

Powered by blists - more mailing lists