Re: Explicit status phase for DWC3

[Dan Scally <dan.scally@ideasonboard.com>]

(+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 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://lkml.org/lkml/2018/10/10/138

[2] 
https://patchwork.kernel.org/project/linux-usb/patch/20190124030228.19840-5-paul.elder@ideasonboard.com/

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