Re: Explicit status phase for DWC3

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Thu, Feb 02, 2023, Dan Scally wrote:
> (+CC roger as the author of the USB_GADGET_DELAYED_STATUS mechanism)
> 
> On 26/01/2023 23:57, Thinh Nguyen wrote:
> > On Thu, Jan 26, 2023, Alan Stern wrote:
> > > On Thu, Jan 26, 2023 at 07:31:34PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 26, 2023, Dan Scally wrote:
> > > > > Hi Thinh
> > > > > 
> > > > > On 26/01/2023 00:20, Thinh Nguyen wrote:
> > > > > > On Tue, Jan 24, 2023, Dan Scally wrote:
> > > > > > > Hi Thinh
> > > > > > > 
> > > > > > > 
> > > > > > > I'm trying to update the DWC3 driver to allow the status phase of a
> > > > > > > transaction to be explicit; meaning the gadget driver has the choice to
> > > > > > > either Ack or Stall _after_ the data phase so that the contents of the data
> > > > > > > phase can be validated. I thought that I should be able to achieve this by
> > > > > > > preventing dwc3_ep0_xfernotready() from calling dwc3_ep0_do_control_status()
> > > > > > > (relying on an "explicit_status" flag added to the usb_request to decide
> > > > > > > whether or not to do so) and then calling it manually later once the data
> > > > > > > phase was validated by the gadget driver (or indeed userspace). A very
> > > > > > > barebones version of my attempt to do that looks like this:
> > > > > > > 
> > > > > > We shouldn't do this. At the protocol level, there must be better ways
> > > > > > to do handshake than relying on protocol STALL _after_ the data stage.
> > > > > > Note that not all controllers support this.
> > > > > 
> > > > > Maybe I'm misunderstanding, but isn't this how the USB spec expects it to
> > > > > work? Reading "Reporting Status Results (8.5.3.1)" in the USB 2.0 spec for
> > > > > the status stage in a control write it says "The function responds with
> > > > > either a handshake or a zero-length data packet to indicate its current
> > > > > status", and the handshake can be either STALL or NAK. If we can't do this,
> > > > > how else can we indicate to the host that the data sent during a control out
> > > > > transfer is in some way invalid?
> > > > > 
> > > > My concern is from the documentation note[*] added from this commit:
> > > > 579c2b46f74 ("USB Gadget: documentation update")
> > > When the gadget subsystem was originally designed, it made no allowance
> > > for sending a STALL in the status stage.  The UDC drivers existing at
> > > that time would automatically send their own zero-length status packet
> > > when the control data was received.
> > > 
> > > Drivers written since then have copied that approach.  They had to, if
> > > they wanted to work with the existing gadget drivers.  So the end result
> > > is that fully supporting status stalls will require changing pretty much
> > > every UDC driver.
> > > 
> > > As for whether the UDC hardware has support...  I don't know.  Some of
> > > the earlier devices might not, but I expect that the more popular recent
> > > designs would provide a way to do it.
> > > 
> > Right, it's just a bit concerning when the document also noted this:
> > "Note that some USB device controllers disallow protocol stall responses
> > in some cases."
> > 
> > It could be just for older controllers as you mentioned.
> > 
> > 
> > Hi Dan,
> > 
> > We should already have this mechanism in place to do protocol STALL.
> > Please look into delayed_status and set halt.
> 
> 
> Thanks; I tried this by returning USB_GADGET_DELAYED_STATUS from the
> function's .setup() callback and later (after userspace checks the data
> packet) either calling usb_ep_queue() or usb_ep_set_halt() and it does seem
> to be working. This surprises me, as my understanding was that the purpose
> of USB_GADGET_DELAYED_STATUS  is to pause all control transfers including
> the data phase to give the function driver enough time to queue a request
> (and possibly only for specific requests). Regardless though I think the
> conclusion from previous discussions on this topic (see [1] for example) was
> that we don't want to rely on USB_GADGET_DELAYED_STATUS to do this which is

My comment initially was more for handling from the host and how it
should behave. If the UVC spec requires this, then we should implement
it. Since you only handle the device side, we have no control how the
host would behave.

I probably shouldn't have brought it up at all as it may cause more
confusion.

Thanks,
Thinh

> why I had avoided it in the first place. A colleague made a series [2] some
> time ago that adds a flag to usb_request which function drivers can set when
> queuing the data phase request. UDC drivers then read that flag to decide
> whether to delay the status phase until after another usb_ep_queue(), and
> that's what I'm trying to implement here.
> 
> 
> [1] https://urldefense.com/v3/__https://lkml.org/lkml/2018/10/10/138__;!!A4F2R9G_pg!ZSl3snbG53YKu4tSa2gHu3gsxEjYW43QyGXm1YIR3oRfBqePu1Nxk3aS-cecwoVt4bCqNU6y3ZUEbrH2BScfSck_xq7_$
> 
> [2] https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/patch/20190124030228.19840-5-paul.elder@ideasonboard.com/__;!!A4F2R9G_pg!ZSl3snbG53YKu4tSa2gHu3gsxEjYW43QyGXm1YIR3oRfBqePu1Nxk3aS-cecwoVt4bCqNU6y3ZUEbrH2BScfSacsmPOj$
> 
> > 
> > Regarding this question:
> > 	How else can we indicate to the host that the data sent during a
> > 	control out transfer is in some way invalid?
> > 
> > Typically there should be another request checking for the command
> > status. I suppose if we use protocol STALL, you only need to send status
> > request check on error cases.
> > 
> > Thanks,
> > Thinh