On Thu, Feb 02, 2023, Alan Stern wrote: > On Thu, Feb 02, 2023 at 03:45:24PM +0000, Dan Scally wrote: > > > > On 02/02/2023 14:52, Alan Stern wrote: > > > On Thu, Feb 02, 2023 at 10:12:45AM +0000, Dan Scally wrote: > > > > (+CC roger as the author of the USB_GADGET_DELAYED_STATUS mechanism) > > > > > > > > On 26/01/2023 23:57, Thinh Nguyen wrote: > > > > > 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://urldefense.com/v3/__https://lkml.org/lkml/2018/10/10/138__;!!A4F2R9G_pg!egC57Exy2Wsf5lRzlULu6D3E3fc8Svgx5TnFsmB3Em1KX7OoaEnmD6dW_l8_G4pzybrPYhDqXfZ6f7XoEZVXKqUg4k5v$ > > > > > > > > [2] https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/patch/20190124030228.19840-5-paul.elder@xxxxxxxxxxxxxxxx/__;!!A4F2R9G_pg!egC57Exy2Wsf5lRzlULu6D3E3fc8Svgx5TnFsmB3Em1KX7OoaEnmD6dW_l8_G4pzybrPYhDqXfZ6f7XoEZVXKrC-ESSk$ > > > I'm in favor of the explicit_status approach from [2]. In fact, there > > > was a whole series of patches impementing this, and I don't think any of > > > them were merged. > > > > > > Yep, I'm picking that series up and want to get it merged. > > > > > Keep in mind that there are two separate issues here: > > > > > > Status/data stage for a control-IN or 0-length control-OUT > > > transfer. > > > > > > Status stage for a non-0-length control-OUT transfer. > > > > > > The USB_GADGET_DELAYED_STATUS mechanism was meant to help with the > > > first, not the second. explicit_status was meant to help with the > > > second; it may be able to help with both. While we may not have a convenient function to do the status completion, the gadget driver can always use the same mechanism for delayed status and explicitly queue the status stage on the OUT data completion. The dwc3 driver should already be able to handle that. However, it's better to have a convenient function for that, and probably remove any warning we have in the composite layer. > > > > Ack - thanks. That thread I linked was very informative, I wish I'd found it > > sooner! > > There is still a race in the gadget layer's handling of control > requests. The host can send a SETUP packet at any time. So when a > function driver queues a usb_request for ep0, how does the UDC driver > know whether it is in response to the SETUP packet that just now arrived > or in response to one that arrived earlier (and is now superseded)? Different stages of different the control transfers shouldn't interleave. If a new SETUP packet is received before completing the previous control transfer, the previous control transfer is canceled. Control transfer doesn't have something like bulk stream-id to allow for interleaving. The gadget driver should handle the different control transfers synchronously. > > This race exists even at the hardware level, and I'm pretty sure that a > lot of UDC controllers don't handle it properly. But there's nothing we > can do about that... I can't speak for other controllers, but this is for dwc3 controllers: If the host sends a new SETUP packet without finishing with the previous control transfer data/status, the data/status TRB would be completed with "SETUP_PENDING" status. This indicates that the host canceled the previous control transfer and the UDC driver needs to setup a SETUP TRB to service the new SETUP packet. The previous control transfer would be returned with a canceled status. BR, Thinh > > My thought (and this goes back almost 20 years!) was that a UDC driver > should associate a different tag value with each incoming SETUP packet. > This tag would get passed to the function driver in its ->setup() > callback, and the function driver would copy the value into a new > .control_tag field of the usb_request structure it queues as part of the > control transfer. > > Then the UDC driver could inspect the control_tag value when it is asked > to queue a request for ep0, and it could return failure if the value > doesn't match the UDC's current tag. This can be done while holding the > UDC's spinlock, so it will be free of races. > > The right way to do this would be to add a new argument to the ->setup() > callback, for the tag value. But this would mean changing the gadget > API, and it would require simultaneously updating every UDC driver and > every gadget/function driver. > > Alternatively, there could be a .current_tag field added to the > usb_gadget structure, which is also passed to ->setup(). It would be > more awkward, but drivers not converted to the new mechanism would > simply leave the field permanently set to 0. Provided all genuine tags > are nonzero, the mechanism would be backward compatible with existing > code. > > Of course, this is all independent of the explicit_status changes. > > Alan Stern
