From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751297AbdAQQBw (ORCPT ); Tue, 17 Jan 2017 11:01:52 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:52164 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751279AbdAQQBu (ORCPT ); Tue, 17 Jan 2017 11:01:50 -0500 Date: Tue, 17 Jan 2017 10:54:53 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Felipe Balbi cc: Baolin Wang , Greg KH , USB , LKML , Linaro Kernel Mailman List , Mark Brown Subject: Re: [PATCH] usb: dwc3: ep0: Fix the possible missed request for handling delay STATUS phase In-Reply-To: <87pojm95ld.fsf@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 16 Jan 2017, Felipe Balbi wrote: > > The gadget driver never calls usb_ep_queue in order to receive the next > > SETUP packet; the UDC driver takes care of SETUP handling > > automatically. > > yeah, that's another thing I'd like to change. Currently, we have no > means to either try to implement device-initiated LPM without adding a > ton of hacks to UDC drivers. If we require upper layers (composite.c, > most of the time) to usb_ep_queue() separate requests for all 3 phases > of a ctrl transfer, we can actually rely on the fact that a new SETUP > phase hasn't been queued yet to trigger U3 entry. I haven't given any thought to LPM. However, requiring gadget drivers to request SETUP packets seems rather questionable. It flies against the USB spec, which requires peripherals to accept SETUP packets at any time -- a device is not allowed to NAK or STALL a SETUP packet (see 8.4.6.4 in the USB-2 spec). In fact, the hardware in UDCs probably isn't capable of doing it. This means that to do what you want, the UDC driver would have to accept SETUP packets at any time, and store the most recent packet contents. Then, when the gadget driver submits a request, the UDC driver would give it this stored data. It would also have to detect and prevent a nasty race where the gadget driver tries to queue a request on ep0 that is a response to an old SETUP, one that has already been overwritten. I'm not even sure preventing this race would be possible in your scheme. The advantage to invoking the gadget driver's setup callback directly from the UDC driver's interrupt handler is that the gadget driver will know immediately when an old SETUP has become stale. (That's what ep0_req_tag is for in f_mass_storage.) It also provides a concurrency guarantee, because the driver does not re-enable UDC SETUP interrupts until the handler is finished. > Another detail that this helps is that PM (overall) becomes simpler as, > most likely, we won't need to mess with transfer cancellation, for > example. System PM on a gadget is always troublesome. Even if the USB connection is a wakeup source, it may not be possible to guarantee that the gadget can wake up quickly enough to handle an incoming packet. > > You are suggesting that status stage requests should not be queued > > automatically by UDC drivers but instead queued explicitly by gadget > > drivers. This would mean changing every UDC driver and every gadget > > driver. > > yes, a bit of work but has been done before. One example that comes to > mind is when I added ->udc_start() and ->udc_stop(). It's totally > doable. We can, for instance, add a temporary > "wants_explicit_ctrl_phases" flag to struct usb_gadget which, if set, > will tell composite.c (or whatever) that the UDC wants explicitly queued > ctrl phases. The term used in the USB spec is "stage", not "phase". "Phase" refers to the packets making up a single transaction: token, data, and handshake. Also, data stages are already explicit. So your temporary flag might better be called "wants_explicit_status_stages". > Then add support for that to each UDC and set the flag. Once all are > converted, add one extra patch to remove the flag and the legacy > code. This has, of course, the draw back of increasing complexity until > everything is converted over; but if it's all done in a single series, I > can't see any problems with that. > > > Also, it won't fix the race that Baolin Wang found. The setup routine > > well, it will help... see below. > > > is always called in interrupt context, so it can't sleep. Doing > > anything non-trivial will require a separate task, and it's possible > > that this task will try to enqueue the data-stage or status-stage > > request before the UDC driver is ready to handle it (for example, > > before or shortly after the setup routine returns). > > > > To work properly, the UDC driver must be able to accept a request for > > ep0 any time after it invokes the setup callback -- either before the > > callback returns or after. > > Right, all UDCs are *already* required to support this case anyway > because of USB_GADGET_DELAYED_STATUS. There was a bug in DWC3, sure, but > it was already required to support this case. > > By removing USB_GADGET_DELAYED_STATUS altogether and making phases more > explict, we enforce this requirement and it'll be much easier to test > for it IMO. Okay, I can see the point of requiring explicit status requests. Implementing it will be a little tricky, because right now some status requests already are explicit (those for length-0 OUT transfers) while others are implicit. (One possible approach would be to have the setup routine return different values for explicit and implicit status stages -- for example, return 1 if it wants to submit an explicit status request. That wouldn't be very different from the current USB_GADGET_DELAYED_STATUS approach.) On the other hand, I am very doubtful about requiring explicit setup requests. Alan Stern