Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Paul Elder <paul.elder@ideasonboard.com>]

On Sat, Jun 15, 2024 at 10:33:28PM -0400, Alan Stern wrote:
> On Sat, Jun 15, 2024 at 10:49:46PM +0200, Andrey Konovalov wrote:
> > On Sat, Jun 15, 2024 at 4:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > There has been a patch posted to support UDC drivers that don't
> > > automatically acknowledge non-zero-length control-OUT transfers.  But
> > > the patch hasn't been merged, and even if it were, all the existing UDC
> > > drivers would still need to be updated.
> > 
> > This series below is the one you're referring to, right?
> > 
> > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
> 
> Yes, that's it.  I'm impressed that you were able to find it; I had lost 
> track of it.
> 
> > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > the latest version I managed to find.
> 
> I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't 
> very interested in fixing the problem.

So that's why we never continued with merging it...

Is it time to dust it off and try to upstream it again? :)


Paul

> 
> > Also, just to check my understanding: with that series in place and
> > assuming the UDC drivers are updated, a gadget driver would need to
> > first do usb_ep_queue with the proper length and explicit_status ==
> > true to get the data for the control OUT request, and then either do
> > usb_ep_queue again with length 0 to ack or do usb_ep_set_halt to
> > stall?
> 
> Yes, that's how it worked.  Alternatively, if the gadget driver didn't 
> set explicit_status in the control-OUT request then the UDC core would 
> automatically call usb_ep_queue again with a 0-length transfer to send 
> the status.  That way existing gadget drivers would continue to work 
> after the UDC drivers were updated, and updated UDC drivers wouldn't 
> have to worry about doing an automatic acknowledge only some of the 
> time.
> 
> Note that in order to avoid breaking things during the transition 
> period, it would also be necessary to add a flag to the usb_gadget 
> structure, indicating that the UDC driver has been updated to support 
> explicit_status.
> 
> Alan Stern
> 
> PS: There's another weakness in the Gadget API which you might possibly 
> run across in your project.  It's less likely to arise because it 
> involves lengthy delays.
> 
> Say there's a control transfer with delayed status, and the gadget 
> driver delays for so long that the host times out the transfer.  Then 
> the host starts a new control transfer before the gadget driver queues 
> its status reply.  Since the Gadget API doesn't have any way to indicate 
> which control transfer a usb_request was meant for, the reply that was 
> meant for the old transfer would get sent to the host, and the host 
> would think it was a reply to the new transfer.
> 
> This problem could be solved by adding a unique ID tag to each 
> usb_request, and passing the transfer ID as an extra argument to the 
> gadget driver's setup() callback.  That would explicitly indicate which 
> transfer a request was meant for.  But doing this would also require 
> updating every function driver and every UDC driver.  Probably not worth 
> the effort, considering how unlikely it is that the situation will ever 
> arise.