From: Alan Stern <stern@rowland.harvard.edu>
To: Dan Scally <dan.scally@ideasonboard.com>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
rogerq@kernel.org
Subject: Re: Explicit status phase for DWC3
Date: Thu, 2 Feb 2023 11:37:02 -0500 [thread overview]
Message-ID: <Y9vmrgeblXC7zeQj@rowland.harvard.edu> (raw)
In-Reply-To: <9da07e03-7cd2-cfeb-8c67-4562948aa948@ideasonboard.com>
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://lkml.org/lkml/2018/10/10/138
> > >
> > > [2] https://patchwork.kernel.org/project/linux-usb/patch/20190124030228.19840-5-paul.elder@ideasonboard.com/
> > 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.
>
> 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)?
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...
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
next prev parent reply other threads:[~2023-02-02 16:37 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-24 14:27 Explicit status phase for DWC3 Dan Scally
2023-01-26 0:20 ` Thinh Nguyen
2023-01-26 10:30 ` Dan Scally
2023-01-26 19:31 ` Thinh Nguyen
2023-01-26 20:31 ` Alan Stern
2023-01-26 23:57 ` Thinh Nguyen
2023-02-02 10:12 ` Dan Scally
2023-02-02 14:51 ` Roger Quadros
2023-02-02 14:52 ` Alan Stern
2023-02-02 15:45 ` Dan Scally
2023-02-02 16:37 ` Alan Stern [this message]
2023-02-02 19:48 ` Thinh Nguyen
2023-02-02 20:15 ` Alan Stern
2023-04-05 19:35 ` Hans Petter Selasky
2023-02-02 20:01 ` Thinh Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y9vmrgeblXC7zeQj@rowland.harvard.edu \
--to=stern@rowland.harvard.edu \
--cc=Thinh.Nguyen@synopsys.com \
--cc=dan.scally@ideasonboard.com \
--cc=linux-usb@vger.kernel.org \
--cc=rogerq@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.