[4/6] usb: gadget: add functions to signal udc driver to delay status stage

Commit Message

Paul Elder Oct. 10, 2018, 2:49 a.m. UTC

A usb gadget function driver may or may not want to delay the status
stage of a control OUT request. An instance it might want to is to
asynchronously validate the data of a class-specific request.

Add a function usb_ep_delay_status to allow function drivers to choose
to delay the status stage in the request completion handler. The UDC
should then check the usb_ep->delayed_status flag and act accordingly to
delay the status stage.

Also add a function usb_ep_send_response as a wrapper for
usb_ep->ops->send_response, whose prototype is added as well. This
function should be called by function drivers to tell the UDC what to
reply in the status stage that it has requested to be delayed.

Signed-off-by: Paul Elder <[e-mail]>
Reviewed-by: Laurent Pinchart <[e-mail]>
---
 drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h    | 11 +++++++++++
 2 files changed, 46 insertions(+)

Comments

Bin Liu Oct. 11, 2018, 4:10 p.m. UTC | #1

Hi,

On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> A usb gadget function driver may or may not want to delay the status
> stage of a control OUT request. An instance it might want to is to
> asynchronously validate the data of a class-specific request.
> 
> Add a function usb_ep_delay_status to allow function drivers to choose
> to delay the status stage in the request completion handler. The UDC
> should then check the usb_ep->delayed_status flag and act accordingly to
> delay the status stage.
> 
> Also add a function usb_ep_send_response as a wrapper for
> usb_ep->ops->send_response, whose prototype is added as well. This
> function should be called by function drivers to tell the UDC what to
> reply in the status stage that it has requested to be delayed.
> 
> Signed-off-by: Paul Elder <[e-mail]>
> Reviewed-by: Laurent Pinchart <[e-mail]>
> ---
>  drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h    | 11 +++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48c1cea..1ec5ce6b43cd 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
>  }
>  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
>  
> +/**
> + * usb_ep_ep_delay_status - set delay_status flag
> + * @ep: the endpoint whose delay_status flag is being set
> + *
> + * This function instructs the UDC to delay the status stage of a control
> + * request. It can only be called from the request completion handler of a
> + * control request.
> + */
> +void usb_ep_delay_status(struct usb_ep *ep)
> +{
> +	ep->delayed_status = true;
> +}
> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);

Is usb_ep_set_delay_status() better? I thought it implies get/return
action if a verb is missing in the function name.

Regards,
-Bin.

Laurent Pinchart Oct. 17, 2018, 11:45 p.m. UTC | #2

Hi Bin,

On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> > A usb gadget function driver may or may not want to delay the status
> > stage of a control OUT request. An instance it might want to is to
> > asynchronously validate the data of a class-specific request.
> > 
> > Add a function usb_ep_delay_status to allow function drivers to choose
> > to delay the status stage in the request completion handler. The UDC
> > should then check the usb_ep->delayed_status flag and act accordingly to
> > delay the status stage.
> > 
> > Also add a function usb_ep_send_response as a wrapper for
> > usb_ep->ops->send_response, whose prototype is added as well. This
> > function should be called by function drivers to tell the UDC what to
> > reply in the status stage that it has requested to be delayed.
> > 
> > Signed-off-by: Paul Elder <[e-mail]>
> > Reviewed-by: Laurent Pinchart <[e-mail]>
> > ---
> > 
> >  drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> >  include/linux/usb/gadget.h    | 11 +++++++++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index af88b48c1cea..1ec5ce6b43cd 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > 
> > +/**
> > + * usb_ep_ep_delay_status - set delay_status flag
> > + * @ep: the endpoint whose delay_status flag is being set
> > + *
> > + * This function instructs the UDC to delay the status stage of a control
> > + * request. It can only be called from the request completion handler of
> > a
> > + * control request.
> > + */
> > +void usb_ep_delay_status(struct usb_ep *ep)
> > +{
> > +	ep->delayed_status = true;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> 
> Is usb_ep_set_delay_status() better? I thought it implies get/return
> action if a verb is missing in the function name.

For what it's worth, I understand the function name as "delay the status 
stage", with "delay" being a verb. Maybe the short description could be 
updated accordingly.

Bin Liu Oct. 18, 2018, 12:46 p.m. UTC | #3

Laurent,

On Thu, Oct 18, 2018 at 02:45:32AM +0300, Laurent Pinchart wrote:
> Hi Bin,
> 
> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> > > A usb gadget function driver may or may not want to delay the status
> > > stage of a control OUT request. An instance it might want to is to
> > > asynchronously validate the data of a class-specific request.
> > > 
> > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > to delay the status stage in the request completion handler. The UDC
> > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > delay the status stage.
> > > 
> > > Also add a function usb_ep_send_response as a wrapper for
> > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > function should be called by function drivers to tell the UDC what to
> > > reply in the status stage that it has requested to be delayed.
> > > 
> > > Signed-off-by: Paul Elder <[e-mail]>
> > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > > ---
> > > 
> > >  drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/usb/gadget.h    | 11 +++++++++++
> > >  2 files changed, 46 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > > 
> > > +/**
> > > + * usb_ep_ep_delay_status - set delay_status flag
> > > + * @ep: the endpoint whose delay_status flag is being set
> > > + *
> > > + * This function instructs the UDC to delay the status stage of a control
> > > + * request. It can only be called from the request completion handler of
> > > a
> > > + * control request.
> > > + */
> > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > +{
> > > +	ep->delayed_status = true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > 
> > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > action if a verb is missing in the function name.
> 
> For what it's worth, I understand the function name as "delay the status 
> stage", with "delay" being a verb. Maybe the short description could be 
> updated accordingly.

Okay, maybe it is just my own understanding problem. I thought about
delay as the verb, but then notice the var is called delayed_status,
then I was thinking delay_status in the function name as the flag.

No worries.

Regards,
-Bin.

Alan Stern Oct. 18, 2018, 2:07 p.m. UTC | #4

On Thu, 18 Oct 2018, Laurent Pinchart wrote:

> Hi Bin,
> 
> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> > > A usb gadget function driver may or may not want to delay the status
> > > stage of a control OUT request. An instance it might want to is to
> > > asynchronously validate the data of a class-specific request.
> > > 
> > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > to delay the status stage in the request completion handler. The UDC
> > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > delay the status stage.
> > > 
> > > Also add a function usb_ep_send_response as a wrapper for
> > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > function should be called by function drivers to tell the UDC what to
> > > reply in the status stage that it has requested to be delayed.
> > > 
> > > Signed-off-by: Paul Elder <[e-mail]>
> > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > > ---
> > > 
> > >  drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > >  include/linux/usb/gadget.h    | 11 +++++++++++
> > >  2 files changed, 46 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > >  }
> > >  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > > 
> > > +/**
> > > + * usb_ep_ep_delay_status - set delay_status flag
> > > + * @ep: the endpoint whose delay_status flag is being set
> > > + *
> > > + * This function instructs the UDC to delay the status stage of a control
> > > + * request. It can only be called from the request completion handler of
> > > a
> > > + * control request.
> > > + */
> > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > +{
> > > +	ep->delayed_status = true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > 
> > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > action if a verb is missing in the function name.
> 
> For what it's worth, I understand the function name as "delay the status 
> stage", with "delay" being a verb. Maybe the short description could be 
> updated accordingly.

Is there a reason for adding a new function for this?  This is exactly
what the USB_GADGET_DELAYED_STATUS return value from the setup callback
is meant for (and it is already used by some gadget drivers).

Is it a question of when the gadget driver learns that it will need to
delay the status stage?  If that's the case, why not always return
USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
calling usb_ep_delay_status() when a delay is needed, you could queue
the status request when a delay isn't needed.

As a more general solution, Felipe has said that a UDC driver should 
_never_ carry out the status stage transaction until the gadget driver 
has told it to do so.  Then there would be no need for any sort of 
delay indicator.  (But implementing this would require significant 
changes to a bunch of different drivers...)

Alan Stern

Paul Elder Nov. 1, 2018, 11:40 p.m. UTC | #5

Hi Alan,

On Thu, Oct 18, 2018 at 10:07:36AM -0400, Alan Stern wrote:
> On Thu, 18 Oct 2018, Laurent Pinchart wrote:
> 
> > Hi Bin,
> > 
> > On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> > > On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> > > > A usb gadget function driver may or may not want to delay the status
> > > > stage of a control OUT request. An instance it might want to is to
> > > > asynchronously validate the data of a class-specific request.
> > > > 
> > > > Add a function usb_ep_delay_status to allow function drivers to choose
> > > > to delay the status stage in the request completion handler. The UDC
> > > > should then check the usb_ep->delayed_status flag and act accordingly to
> > > > delay the status stage.
> > > > 
> > > > Also add a function usb_ep_send_response as a wrapper for
> > > > usb_ep->ops->send_response, whose prototype is added as well. This
> > > > function should be called by function drivers to tell the UDC what to
> > > > reply in the status stage that it has requested to be delayed.
> > > > 
> > > > Signed-off-by: Paul Elder <[e-mail]>
> > > > Reviewed-by: Laurent Pinchart <[e-mail]>
> > > > ---
> > > > 
> > > >  drivers/usb/gadget/udc/core.c | 35 +++++++++++++++++++++++++++++++++++
> > > >  include/linux/usb/gadget.h    | 11 +++++++++++
> > > >  2 files changed, 46 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > > > index af88b48c1cea..1ec5ce6b43cd 100644
> > > > --- a/drivers/usb/gadget/udc/core.c
> > > > +++ b/drivers/usb/gadget/udc/core.c
> > > > @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> > > > 
> > > > +/**
> > > > + * usb_ep_ep_delay_status - set delay_status flag
> > > > + * @ep: the endpoint whose delay_status flag is being set
> > > > + *
> > > > + * This function instructs the UDC to delay the status stage of a control
> > > > + * request. It can only be called from the request completion handler of
> > > > a
> > > > + * control request.
> > > > + */
> > > > +void usb_ep_delay_status(struct usb_ep *ep)
> > > > +{
> > > > +	ep->delayed_status = true;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > > 
> > > Is usb_ep_set_delay_status() better? I thought it implies get/return
> > > action if a verb is missing in the function name.
> > 
> > For what it's worth, I understand the function name as "delay the status 
> > stage", with "delay" being a verb. Maybe the short description could be 
> > updated accordingly.
> 
> Is there a reason for adding a new function for this?  This is exactly
> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> is meant for (and it is already used by some gadget drivers).

In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
However, there are a few ambiguities that prevent us from doing so.

First of all, we want to delay only the status stage for control OUT
requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
delaying the "data/status stages". Does this mean that it delays the
status stage only or does it delay both stages? If the slash means
"and", then we cannot use USB_GADGET_DELAYED_STATUS.

Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
which has already been observed in the UVC gadget driver previously [0].
The raceiness stems from the fact that things can happen in between
returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
it - especially if usb_composite_setup_continue is called within that
window it causes a WARN. In any case, the fact that the mechanism itself
is racey suggests that it needs improvement, and using it wouldn't be a
good solution in this case.

> Is it a question of when the gadget driver learns that it will need to
> delay the status stage?  If that's the case,

Not really.

> why not always return
> USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
> calling usb_ep_delay_status() when a delay is needed, you could queue
> the status request when a delay isn't needed.

Theoretically this might work, but see the problems mentioned above.

> As a more general solution, Felipe has said that a UDC driver should 
> _never_ carry out the status stage transaction until the gadget driver 
> has told it to do so.  Then there would be no need for any sort of 
> delay indicator.

Yeah, but,

> (But implementing this would require significant 
> changes to a bunch of different drivers...)

exactly :/

[0] https://www.spinics.net/lists/linux-usb/msg169208.html


Paul

Laurent Pinchart Nov. 2, 2018, 12:44 p.m. UTC | #6

Hello,

On Friday, 2 November 2018 01:40:59 EET Paul Elder wrote:
> On Thu, Oct 18, 2018 at 10:07:36AM -0400, Alan Stern wrote:
> > On Thu, 18 Oct 2018, Laurent Pinchart wrote:
> >> On Thursday, 11 October 2018 19:10:21 EEST Bin Liu wrote:
> >>> On Tue, Oct 09, 2018 at 10:49:01PM -0400, Paul Elder wrote:
> >>>> A usb gadget function driver may or may not want to delay the status
> >>>> stage of a control OUT request. An instance it might want to is to
> >>>> asynchronously validate the data of a class-specific request.
> >>>> 
> >>>> Add a function usb_ep_delay_status to allow function drivers to
> >>>> choose to delay the status stage in the request completion handler.
> >>>> The UDC should then check the usb_ep->delayed_status flag and act
> >>>> accordingly to delay the status stage.
> >>>> 
> >>>> Also add a function usb_ep_send_response as a wrapper for
> >>>> usb_ep->ops->send_response, whose prototype is added as well. This
> >>>> function should be called by function drivers to tell the UDC what
> >>>> to reply in the status stage that it has requested to be delayed.
> >>>> 
> >>>> Signed-off-by: Paul Elder <[e-mail]>
> >>>> Reviewed-by: Laurent Pinchart <[e-mail]>
> >>>> ---
> >>>> 
> >>>>  drivers/usb/gadget/udc/core.c | 35 ++++++++++++++++++++++++++++++++
> >>>>  include/linux/usb/gadget.h    | 11 +++++++++++
> >>>>  2 files changed, 46 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/usb/gadget/udc/core.c
> >>>> b/drivers/usb/gadget/udc/core.c
> >>>> index af88b48c1cea..1ec5ce6b43cd 100644
> >>>> --- a/drivers/usb/gadget/udc/core.c
> >>>> +++ b/drivers/usb/gadget/udc/core.c
> >>>> @@ -443,6 +443,41 @@ void usb_ep_fifo_flush(struct usb_ep *ep)
> >>>> 
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
> >>>> 
> >>>> +/**
> >>>> + * usb_ep_ep_delay_status - set delay_status flag
> >>>> + * @ep: the endpoint whose delay_status flag is being set
> >>>> + *
> >>>> + * This function instructs the UDC to delay the status stage of a
> >>>> control
> >>>> + * request. It can only be called from the request completion
> >>>> handler of a
> >>>> + * control request.
> >>>> + */
> >>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>> +{
> >>>> +	ep->delayed_status = true;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>> 
> >>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> >>> action if a verb is missing in the function name.
> >> 
> >> For what it's worth, I understand the function name as "delay the status
> >> stage", with "delay" being a verb. Maybe the short description could be
> >> updated accordingly.
> > 
> > Is there a reason for adding a new function for this?  This is exactly
> > what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> > is meant for (and it is already used by some gadget drivers).
> 
> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> However, there are a few ambiguities that prevent us from doing so.
> 
> First of all, we want to delay only the status stage for control OUT
> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> delaying the "data/status stages". Does this mean that it delays the
> status stage only or does it delay both stages? If the slash means
> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
> 
> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> which has already been observed in the UVC gadget driver previously [0].
> The raceiness stems from the fact that things can happen in between
> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> it - especially if usb_composite_setup_continue is called within that
> window it causes a WARN. In any case, the fact that the mechanism itself
> is racey suggests that it needs improvement, and using it wouldn't be a
> good solution in this case.
> 
> > Is it a question of when the gadget driver learns that it will need to
> > delay the status stage?  If that's the case,
> 
> Not really.
> 
> > why not always return
> > USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
> > calling usb_ep_delay_status() when a delay is needed, you could queue
> > the status request when a delay isn't needed.
> 
> Theoretically this might work, but see the problems mentioned above.
> 
> > As a more general solution, Felipe has said that a UDC driver should
> > _never_ carry out the status stage transaction until the gadget driver
> > has told it to do so.  Then there would be no need for any sort of
> > delay indicator.
> 
> Yeah, but,
> 
> > (But implementing this would require significant
> > changes to a bunch of different drivers...)
> 
> exactly :/
> 
> [0] https://www.spinics.net/lists/linux-usb/msg169208.html

Alan, Felipe, how do we move forward with this ? There are several issues with 
the existing control request handling mechanism in the USB gadget stack, and 
while Paul could work on improving the mechanism, we need to provide clear 
guidance regarding the direction we want to take.

For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS 
mechanism are

- The mechanism is inherently racy. It relies on signaling the delay at the 
very end of the processing in the setup handler, which by definition occurs 
after the work to process the control request is queued (in the generic sense, 
regardless of whether this involves a kernel workqueue or passing the work to 
userspace). There is thus a race window after queuing the work and before 
signaling the delay during which the work handler could signal completion.

- The mechanism is poorly documented. As Paul mentioned, comments in the code 
state that USB_GADGET_DELAYED_STATUS delay the "data/status stages". This is 
very unclear, and the only three UDCs that implement the mechanism seem to do 
so in different ways:

  - The mtu driver states in a comment that it will "handle the delay STATUS 
phase till receive ep_queue on ep0".

  - The bdc driver states in a comment that "The ep0 state will remain 
WAIT_FOR_DATA_START till we received ep_queue on ep0".

  - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the 
SET_CONFIG request only.

- The mechanism relies on queueing a request to the UDC to signal that it 
should continue with the status stage. That request can be queued either by 
the USB gadget function driver directly, or by the composite layer in 
usb_composite_setup_continue() (the latter is restricted to requests that 
carry no data as it sets the request length to 0). This is problematic if we 
want to delay the status phase after completing the data phase, in order to 
validate the setup phase data and the data phase data (for a control OUT 
request) together.


For those reasons I think a new mechanism is needed. It should either signal 
the status phase delay through an explicit function call instead of a return 
value (to solve the race mentioned above), or by requiring all requests to be 
explicitly completed (but that will require changing all USB function 
drivers). Furthermore, the mechanism need to support delaying the status phase 
after queuing the request for the data phase, so we need an explicit way to 
signal that the UDC should proceed with the status phase, other than queueing 
the request.

Thoughts ? Preferences ?

Laurent Pinchart Nov. 2, 2018, 2:36 p.m. UTC | #7

Hi Felipe,

On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> Laurent Pinchart writes:
> >>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>>> 
> >>>>> +{
> >>>>> +	ep->delayed_status = true;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>>> 
> >>>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> >>>> action if a verb is missing in the function name.
> >>> 
> >>> For what it's worth, I understand the function name as "delay the
> >>> status stage", with "delay" being a verb. Maybe the short description
> >>> could be updated accordingly.
> >>> 
> >>> Is there a reason for adding a new function for this?  This is exactly
> >>> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> >>> is meant for (and it is already used by some gadget drivers).
> >> 
> >> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> >> However, there are a few ambiguities that prevent us from doing so.
> >> 
> >> First of all, we want to delay only the status stage for control OUT
> >> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> >> delaying the "data/status stages". Does this mean that it delays the
> >> status stage only or does it delay both stages? If the slash means
> >> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
> >> 
> >> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> >> which has already been observed in the UVC gadget driver previously [0].
> >> The raceiness stems from the fact that things can happen in between
> >> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> >> it - especially if usb_composite_setup_continue is called within that
> >> window it causes a WARN. In any case, the fact that the mechanism itself
> >> is racey suggests that it needs improvement, and using it wouldn't be a
> >> good solution in this case.
> >> 
> >>> Is it a question of when the gadget driver learns that it will need to
> >>> delay the status stage?  If that's the case,
> >> 
> >> Not really.
> >> 
> >>> why not always return
> >>> USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
> >>> calling usb_ep_delay_status() when a delay is needed, you could queue
> >>> the status request when a delay isn't needed.
> >> 
> >> Theoretically this might work, but see the problems mentioned above.
> >> 
> >>> As a more general solution, Felipe has said that a UDC driver should
> >>> _never_ carry out the status stage transaction until the gadget driver
> >>> has told it to do so.  Then there would be no need for any sort of
> >>> delay indicator.
> >> 
> >> Yeah, but,
> >> 
> >>> (But implementing this would require significant
> >>> changes to a bunch of different drivers...)
> >> 
> >> exactly :/
> 
> add a flag to gadget structure. Something like
> "supports_explicit_status_stage" and add a new return value
> USB_EXPLICIT_STATUS_STAGE.
> 
> Then, take uvc for example, implement the new setup:
> 
> if (supports_explicit_status_stage)
> 	return USB_EXPLICIT_STATUS_STAGE;
> 
> then on dwc3 you would:
> 
> switch (ret) {
> case USB_EXPLICIT_STATUS_STAGE:
> case USB_GADGET_DELAYED_STATUS:
> 	wait_for_status_request_queue();
>         break;
> default:
> 	start_status_stage();
> }
> 
> If this works with dwc3 + uvc, then we have a good recipe on how to
> implement for the other drivers.

Given that we need to delay the status stage and not the data stage, we can't 
explicitly request the status stage through a usb request queue. Would a new 
explicit function call work for you for that purpose ?

> > Alan, Felipe, how do we move forward with this ? There are several issues
> > with the existing control request handling mechanism in the USB gadget
> > stack, and while Paul could work on improving the mechanism, we need to
> > provide clear guidance regarding the direction we want to take.
> > 
> > For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> > mechanism are
> > 
> > - The mechanism is inherently racy. It relies on signaling the delay at
> > the very end of the processing in the setup handler, which by definition
> > occurs after the work to process the control request is queued (in the
> > generic sense, regardless of whether this involves a kernel workqueue or
> > passing the work to userspace). There is thus a race window after queuing
> > the work and before signaling the delay during which the work handler
> > could signal completion.
> 
> We won't fix this until all functions and UDCs are converted over, but
> it's doable.

It could be fixed by signaling the delay through an explicit function call 
before queueing the work instead of through a return value though, but I agree 
that long term requesting the status stage explicitly would likely be cleaner.

> > - The mechanism is poorly documented. As Paul mentioned, comments in the
> > code state that USB_GADGET_DELAYED_STATUS delay the "data/status stages".
> > This is very unclear, and the only three UDCs that implement the
> > mechanism seem to do so in different ways:
> > 
> >   - The mtu driver states in a comment that it will "handle the delay
> >   STATUS phase till receive ep_queue on ep0".
> > 
> >   - The bdc driver states in a comment that "The ep0 state will remain
> >   WAIT_FOR_DATA_START till we received ep_queue on ep0".
> > 
> >   - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> >   SET_CONFIG request only.
> 
> that's the only one that has needed it so far. I'm all for making status
> stage ALWAYS explicit, that will, in the long run, simplify UDC
> drivers and make the API easier to understand.
> 
> > - The mechanism relies on queueing a request to the UDC to signal that it
> > should continue with the status stage. That request can be queued either
> > by the USB gadget function driver directly, or by the composite layer in
> > usb_composite_setup_continue() (the latter is restricted to requests that
> > carry no data as it sets the request length to 0). This is problematic if
> > we want to delay the status phase after completing the data phase, in
> > order to validate the setup phase data and the data phase data (for a
> > control OUT request) together.
> 
> It shouldn't cause problems, actually. Most of the issues come from the
> fact that sometimes gadget driver just returns a success and expects UDC
> to initiate status stage and sometimes gadget driver wants to handle
> status stage explicitly.

Requesting the status stage explicitly requires an API to do so (quite 
obviously). Your proposal is to use the usb request queue as a signal to 
continue to the next stage. My point is that this can't work for control OUT 
requests, where we may want to delay the status stage after the data is 
received by the UDC, and thus after the request is queued. We need a different 
API for that.

For control IN, we may want to delay the data stage if we can't respond 
directly, or proceed with the data stage immediately. In both cases this can 
be signaled by a request being queued. There is no need to delay the status 
stage as it's initiated by the host.

For control OUT, we may want to delay the data stage if we need to validate 
the setup stage data asynchronously. Proceeding to the data stage can be 
signaled by queueing a request. We may also want to delay the status stage if 
we need to validate the data stage data asynchronously. This can't be signaled 
by queueing a request.

I wonder if there's really a use case for delaying the data stage of control 
OUT requests, as it seems to me that we can perform the asynchronous 
validation of the setup and data stages together, in which case we would 
always proceed to the data stage, and only potentially delay the status stage. 
However, if we switch to an explicit API where the transition from the setup 
to the data stage is triggered by queueing a request, and given that such a 
transition may need to be delayed for the control IN case, delaying the data 
stage for control OUT would essentially come for free.

In any case we need an API to delay the status stage of a control OUT request. 
There are two options here. We can consider that the status stage shouldn't be 
delayed by default and add a new function to be called from the data stage 
completion handler to request a status stage delay (using the return value of 
the completion handler isn't a good idea as it would be racy as explained 
above). A second function would be needed to request the status stage (which, 
as explained above too, can't be done by queueing a request). A second option 
is to consider that the status stage is delayed by default until explcitly 
required. In both cases the same new function is needed to request the status 
stage.

Note that delaying the data stage and delaying the status stage are two 
different problems, and don't necessarily need to be solved together. However, 
if I understand things correctly, we currently delay the data stage of a few 
control OUT requests (they all have a 0 bytes data stage) as a mean to 
completion of the request. I believe that this use case could be implemented 
by delaying the status stage instead, so the two are still related in a way.

If we end up moving to explicit state handling, with the data stage being 
entered by queueing a request, and the status stage being entered by calling a 
new function, control OUT requests with 0 bytes of data that can be handled 
synchronously in the setup handler would require function drivers to both 
queue a zero-length request and call the status function. This would make the 
function code more complex, and I wonder whether a shortcut would be a good 
idea, perhaps in the form of a flag in the request that tells the UDC to 
automatically proceed to the status stage immediately after the data stage. Or 
we could make that behaviour the default when the request doesn't have a 
completion handler (as moving explicitly to the status stage should be done at 
the earliest from the data stage completion handler).

> > For those reasons I think a new mechanism is needed. It should either
> > signal the status phase delay through an explicit function call instead
> > of a return value (to solve the race mentioned above), or by requiring
> > all requests to be explicitly completed (but that will require changing
> > all USB function drivers). Furthermore, the mechanism need to support
> > delaying the status phase after queuing the request for the data phase,
> > so we need an explicit way to signal that the UDC should proceed with the
> > status phase, other than queueing the request.
> > 
> > Thoughts ? Preferences ?
> 
> how about making status stage always explicit?

If we implement a proof of concept, could you help us converting drivers over 
to the new API ? I'll assume we'll have to address all UDCs first, and then 
the function drivers.

Alan Stern Nov. 2, 2018, 4:18 p.m. UTC | #8

On Fri, 2 Nov 2018, Laurent Pinchart wrote:

> Hi Felipe,
> 
> On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> > Laurent Pinchart writes:
> > >>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> > >>>>> 
> > >>>>> +{
> > >>>>> +	ep->delayed_status = true;
> > >>>>> +}
> > >>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> > >>>> 
> > >>>> Is usb_ep_set_delay_status() better? I thought it implies get/return
> > >>>> action if a verb is missing in the function name.
> > >>> 
> > >>> For what it's worth, I understand the function name as "delay the
> > >>> status stage", with "delay" being a verb. Maybe the short description
> > >>> could be updated accordingly.
> > >>> 
> > >>> Is there a reason for adding a new function for this?  This is exactly
> > >>> what the USB_GADGET_DELAYED_STATUS return value from the setup callback
> > >>> is meant for (and it is already used by some gadget drivers).
> > >> 
> > >> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for this.
> > >> However, there are a few ambiguities that prevent us from doing so.
> > >> 
> > >> First of all, we want to delay only the status stage for control OUT
> > >> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> > >> delaying the "data/status stages". Does this mean that it delays the
> > >> status stage only or does it delay both stages? If the slash means
> > >> "and", then we cannot use USB_GADGET_DELAYED_STATUS.

The comment in composite.h is wrong; it should refer only to the status
stage.  In fact, it should refer only to the status stage for
control-OUT transfers; there's no reason ever to delay the status stage
of a control-IN transfer (the driver should instead delay the data
stage if it needs to).

> > >> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> > >> which has already been observed in the UVC gadget driver previously [0].
> > >> The raceiness stems from the fact that things can happen in between
> > >> returning USB_GADGET_DELAYED_STATUS and the composite layer reacting to
> > >> it - especially if usb_composite_setup_continue is called within that
> > >> window it causes a WARN. In any case, the fact that the mechanism itself
> > >> is racey suggests that it needs improvement, and using it wouldn't be a
> > >> good solution in this case.

I don't understand this at all.  The composite layer reacts to 
USB_GADGET_DELAYED_STATUS as soon as it receives the return value.  Can 
Paul or Laurent give a more explicit example of this race?

Assuming you are correct, wouldn't it make sense to fix or eliminate 
the race by changing composite.c?

> > >>> Is it a question of when the gadget driver learns that it will need to
> > >>> delay the status stage?  If that's the case,
> > >> 
> > >> Not really.
> > >> 
> > >>> why not always return
> > >>> USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
> > >>> calling usb_ep_delay_status() when a delay is needed, you could queue
> > >>> the status request when a delay isn't needed.
> > >> 
> > >> Theoretically this might work, but see the problems mentioned above.
> > >> 
> > >>> As a more general solution, Felipe has said that a UDC driver should
> > >>> _never_ carry out the status stage transaction until the gadget driver
> > >>> has told it to do so.  Then there would be no need for any sort of
> > >>> delay indicator.
> > >> 
> > >> Yeah, but,
> > >> 
> > >>> (But implementing this would require significant
> > >>> changes to a bunch of different drivers...)
> > >> 
> > >> exactly :/
> > 
> > add a flag to gadget structure. Something like
> > "supports_explicit_status_stage" and add a new return value
> > USB_EXPLICIT_STATUS_STAGE.
> > 
> > Then, take uvc for example, implement the new setup:
> > 
> > if (supports_explicit_status_stage)
> > 	return USB_EXPLICIT_STATUS_STAGE;
> > 
> > then on dwc3 you would:
> > 
> > switch (ret) {
> > case USB_EXPLICIT_STATUS_STAGE:
> > case USB_GADGET_DELAYED_STATUS:
> > 	wait_for_status_request_queue();
> >         break;
> > default:
> > 	start_status_stage();
> > }
> > 
> > If this works with dwc3 + uvc, then we have a good recipe on how to
> > implement for the other drivers.
> 
> Given that we need to delay the status stage and not the data stage, we can't 
> explicitly request the status stage through a usb request queue.

Why not?  The status stage for a control-OUT transfer is simply a
zero-length IN transaction.  It's easy to queue a request for such a
transaction.  Is the issue that there's no way to specify the direction
of the request (hence no direct way to tell whether a zero-length
request is for the data stage or the status stage)?

Admittedly, it might be nice to provide a library routine in the UDC
core to queue such requests, since it involves a bunch of uninteresting
boilerplate operations.

> Would a new 
> explicit function call work for you for that purpose ?

It would be okay, but I question whether one is really needed.

> > > Alan, Felipe, how do we move forward with this ? There are several issues
> > > with the existing control request handling mechanism in the USB gadget
> > > stack, and while Paul could work on improving the mechanism, we need to
> > > provide clear guidance regarding the direction we want to take.
> > > 
> > > For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> > > mechanism are
> > > 
> > > - The mechanism is inherently racy. It relies on signaling the delay at
> > > the very end of the processing in the setup handler, which by definition
> > > occurs after the work to process the control request is queued (in the
> > > generic sense, regardless of whether this involves a kernel workqueue or
> > > passing the work to userspace). There is thus a race window after queuing
> > > the work and before signaling the delay during which the work handler
> > > could signal completion.

I'm not at all sure what you're talking about here.  Do you mean this 
code near the end of composite_setup()?

check_value:
	/* respond with data transfer before status phase? */
	if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
		req->length = value;
		req->context = cdev;
		req->zero = value < w_length;
		value = composite_ep0_queue(cdev, req, GFP_ATOMIC);

Plainly the code checks for USB_GADGET_DELAYED_STATUS _before_ queuing 
req, not after.  So where's the race?

> > We won't fix this until all functions and UDCs are converted over, but
> > it's doable.
> 
> It could be fixed by signaling the delay through an explicit function call 
> before queueing the work instead of through a return value though, but I agree 
> that long term requesting the status stage explicitly would likely be cleaner.

Yes, I agree that relying on an implicit status stage is not a good 
idea for the long term.

> > > - The mechanism is poorly documented. As Paul mentioned, comments in the
> > > code state that USB_GADGET_DELAYED_STATUS delay the "data/status stages".
> > > This is very unclear, and the only three UDCs that implement the
> > > mechanism seem to do so in different ways:

We can fix comments and documentation pretty easily.  :-)

> > >   - The mtu driver states in a comment that it will "handle the delay
> > >   STATUS phase till receive ep_queue on ep0".
> > > 
> > >   - The bdc driver states in a comment that "The ep0 state will remain
> > >   WAIT_FOR_DATA_START till we received ep_queue on ep0".
> > > 
> > >   - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> > >   SET_CONFIG request only.
> > 
> > that's the only one that has needed it so far. I'm all for making status
> > stage ALWAYS explicit, that will, in the long run, simplify UDC
> > drivers and make the API easier to understand.
> > 
> > > - The mechanism relies on queueing a request to the UDC to signal that it
> > > should continue with the status stage. That request can be queued either
> > > by the USB gadget function driver directly, or by the composite layer in
> > > usb_composite_setup_continue() (the latter is restricted to requests that
> > > carry no data as it sets the request length to 0). This is problematic if
> > > we want to delay the status phase after completing the data phase, in
> > > order to validate the setup phase data and the data phase data (for a
> > > control OUT request) together.
> > 
> > It shouldn't cause problems, actually. Most of the issues come from the
> > fact that sometimes gadget driver just returns a success and expects UDC
> > to initiate status stage and sometimes gadget driver wants to handle
> > status stage explicitly.
> 
> Requesting the status stage explicitly requires an API to do so (quite 
> obviously). Your proposal is to use the usb request queue as a signal to 
> continue to the next stage. My point is that this can't work for control OUT 
> requests, where we may want to delay the status stage after the data is 
> received by the UDC, and thus after the request is queued. We need a different 
> API for that.

Why so?  Consider the following logic for ep0 in the UDC:

	A SETUP packet is received, for an OUT transfer.

	If setup.wLength > 0 then the next request queued for ep0 is
	the data stage, so it is an OUT request.

	Otherwise (i.e., if setup.wLength == 0 or for the second
	request queued after the SETUP is received), the next request
	is the status stage, so it must be a 0-length IN request.

This requires the UDC to specifically keep track of the direction of
the current transfer and whether or not a data-stage transfer has
already been queued.  That shouldn't be hard.  (But it does involve a
race in cases where the host gets tired of waiting and issues another
SETUP packet before the processing of the first transfer is finished.)

The corresponding logic for IN transfers is simpler, since IN transfers 
are not allowed to have zero length.  The first request following the 
SETUP packet is the data stage and the second is the status stage.

> For control IN, we may want to delay the data stage if we can't respond 
> directly, or proceed with the data stage immediately. In both cases this can 
> be signaled by a request being queued. There is no need to delay the status 
> stage as it's initiated by the host.

Yes.

> For control OUT, we may want to delay the data stage if we need to validate 
> the setup stage data asynchronously. Proceeding to the data stage can be 
> signaled by queueing a request. We may also want to delay the status stage if 
> we need to validate the data stage data asynchronously. This can't be signaled 
> by queueing a request.

It can be, if we use the logic outlined above.

> I wonder if there's really a use case for delaying the data stage of control 
> OUT requests, as it seems to me that we can perform the asynchronous 
> validation of the setup and data stages together, in which case we would 
> always proceed to the data stage, and only potentially delay the status stage. 
> However, if we switch to an explicit API where the transition from the setup 
> to the data stage is triggered by queueing a request, and given that such a 
> transition may need to be delayed for the control IN case, delaying the data 
> stage for control OUT would essentially come for free.
> 
> In any case we need an API to delay the status stage of a control OUT request. 
> There are two options here. We can consider that the status stage shouldn't be 
> delayed by default and add a new function to be called from the data stage 
> completion handler to request a status stage delay (using the return value of 
> the completion handler isn't a good idea as it would be racy as explained 
> above). A second function would be needed to request the status stage (which, 
> as explained above too, can't be done by queueing a request). A second option 
> is to consider that the status stage is delayed by default until explcitly 
> required. In both cases the same new function is needed to request the status 
> stage.
> 
> Note that delaying the data stage and delaying the status stage are two 
> different problems, and don't necessarily need to be solved together. However, 
> if I understand things correctly, we currently delay the data stage of a few 
> control OUT requests (they all have a 0 bytes data stage) as a mean to 
> completion of the request. I believe that this use case could be implemented 
> by delaying the status stage instead, so the two are still related in a way.
> 
> If we end up moving to explicit state handling, with the data stage being 
> entered by queueing a request, and the status stage being entered by calling a 
> new function, control OUT requests with 0 bytes of data that can be handled 
> synchronously in the setup handler would require function drivers to both 
> queue a zero-length request and call the status function. This would make the 
> function code more complex, and I wonder whether a shortcut would be a good 
> idea, perhaps in the form of a flag in the request that tells the UDC to 
> automatically proceed to the status stage immediately after the data stage. Or 
> we could make that behaviour the default when the request doesn't have a 
> completion handler (as moving explicitly to the status stage should be done at 
> the earliest from the data stage completion handler).
> 
> > > For those reasons I think a new mechanism is needed. It should either
> > > signal the status phase delay through an explicit function call instead
> > > of a return value (to solve the race mentioned above), or by requiring
> > > all requests to be explicitly completed (but that will require changing
> > > all USB function drivers). Furthermore, the mechanism need to support
> > > delaying the status phase after queuing the request for the data phase,
> > > so we need an explicit way to signal that the UDC should proceed with the
> > > status phase, other than queueing the request.
> > > 
> > > Thoughts ? Preferences ?
> > 
> > how about making status stage always explicit?
> 
> If we implement a proof of concept, could you help us converting drivers over 
> to the new API ? I'll assume we'll have to address all UDCs first, and then 
> the function drivers.

I'll certainly help for the drivers I'm familiar with.

Alan Stern

Laurent Pinchart Nov. 2, 2018, 5:10 p.m. UTC | #9

Hi Alan,

On Friday, 2 November 2018 18:18:45 EET Alan Stern wrote:
> On Fri, 2 Nov 2018, Laurent Pinchart wrote:
> > On Friday, 2 November 2018 15:17:20 EET Felipe Balbi wrote:
> >> Laurent Pinchart writes:
> >>>>>>> +void usb_ep_delay_status(struct usb_ep *ep)
> >>>>>>> 
> >>>>>>> +{
> >>>>>>> +	ep->delayed_status = true;
> >>>>>>> +}
> >>>>>>> +EXPORT_SYMBOL_GPL(usb_ep_delay_status);
> >>>>>> 
> >>>>>> Is usb_ep_set_delay_status() better? I thought it implies
> >>>>>> get/return action if a verb is missing in the function name.
> >>>>> 
> >>>>> For what it's worth, I understand the function name as "delay the
> >>>>> status stage", with "delay" being a verb. Maybe the short
> >>>>> description could be updated accordingly.
> >>>>> 
> >>>>> Is there a reason for adding a new function for this?  This is
> >>>>> exactly what the USB_GADGET_DELAYED_STATUS return value from the setup
> >>>>> callback is meant for (and it is already used by some gadget drivers).
> >>>> 
> >>>> In theory, we might be able to use USB_GADGET_DELAYED_STATUS for
> >>>> this. However, there are a few ambiguities that prevent us from doing
> >>>> so.
> >>>> 
> >>>> First of all, we want to delay only the status stage for control OUT
> >>>> requests; according to composite.h, USB_GADGET_DELAYED_STATUS is for
> >>>> delaying the "data/status stages". Does this mean that it delays the
> >>>> status stage only or does it delay both stages? If the slash means
> >>>> "and", then we cannot use USB_GADGET_DELAYED_STATUS.
> 
> The comment in composite.h is wrong; it should refer only to the status
> stage.  In fact, it should refer only to the status stage for
> control-OUT transfers; there's no reason ever to delay the status stage
> of a control-IN transfer (the driver should instead delay the data
> stage if it needs to).

Agreed.

> >>>> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
> >>>> which has already been observed in the UVC gadget driver previously
> >>>> [0]. The raceiness stems from the fact that things can happen in
> >>>> between returning USB_GADGET_DELAYED_STATUS and the composite layer
> >>>> reacting to it - especially if usb_composite_setup_continue is called
> >>>> within that window it causes a WARN. In any case, the fact that the
> >>>> mechanism itself is racey suggests that it needs improvement, and using
> >>>> it wouldn't be a good solution in this case.
> 
> I don't understand this at all.  The composite layer reacts to
> USB_GADGET_DELAYED_STATUS as soon as it receives the return value.  Can
> Paul or Laurent give a more explicit example of this race?

The composite layer only handles USB_GADGET_DELAYED_STATUS for 
USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in 
composite_setup()). It increments cdev->delayed_status immediately. Then, in 
usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues 
a ZLP, and warns otherwise.

This mechanism delays the data stage, not the status stage (or, to be precise, 
it delays the status stage insofar as the status stage comes after the data 
stage), and only supports control OUT requests with 0 bytes of data (which is 
the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all 
other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the 
UDC.

The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a 
delayed_status flag in an internal structure. I haven't inspected in details 
what they do next as I'm not familiar with all of them, but the dwc3 driver 
just skips the handling of the status phase in dwc3_ep0_xfernotready() and 
delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests, 
with no data phase.

Even when limited to 0-length control OUT requests, this mechanism is racy. 
The setup handler, when it wants to delay the status phase, will queue 
asynchronous work that will, when it completes, call 
usb_composite_setup_continue() to proceed with the status phase. Queuing the 
work has to be done before the setup handler returns, and the cdev-
>delayed_status is only incremented after the setup handler returns, in 
composite_setup(). There is thus a time window during which the asynchronous 
work can call usb_composite_setup_continue() before cdev->delayed_status has 
been incremented. We have managed to hit this in practice, with a surprisingly 
high rate seeing how small the window is.

Now that I've written all this, I realize that cdev->delayed_status is guarded 
by cdev->lock. I thus wonder whether our analysis was correct, or if we were 
hitting a different bug :-S Paul, could you test this again ? Please note, 
however, that the race described here is not related to this patch series, 
except in how it influences the API design to avoid race conditions.

> Assuming you are correct, wouldn't it make sense to fix or eliminate
> the race by changing composite.c?

I was about to write that we would need to lock access to cdev-
>delayed_status, and found out that we already use cdev->lock to do so. More 
investigations are needed.

Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length 
control OUT requests, so the problem that led to this patch series still 
exists, even if the race condition I thought was there doesn't exist.

> >>>>> Is it a question of when the gadget driver learns that it will need
> >>>>> to delay the status stage?  If that's the case,
> >>>> 
> >>>> Not really.
> >>>> 
> >>>>> why not always return
> >>>>> USB_GADGET_DELAYED_STATUS from the setup callback?  Then instead of
> >>>> calling usb_ep_delay_status() when a delay is needed, you could
> >>>>> queue the status request when a delay isn't needed.
> >>>> 
> >>>> Theoretically this might work, but see the problems mentioned above.
> >>>> 
> >>>>> As a more general solution, Felipe has said that a UDC driver should
> >>>>> _never_ carry out the status stage transaction until the gadget
> >>>>> driver has told it to do so.  Then there would be no need for any sort
> >>>>> of delay indicator.
> >>>> 
> >>>> Yeah, but,
> >>>> 
> >>>>> (But implementing this would require significant
> >>>>> changes to a bunch of different drivers...)
> >>>> 
> >>>> exactly :/
> >> 
> >> add a flag to gadget structure. Something like
> >> "supports_explicit_status_stage" and add a new return value
> >> USB_EXPLICIT_STATUS_STAGE.
> >> 
> >> Then, take uvc for example, implement the new setup:
> >> 
> >> if (supports_explicit_status_stage)
> >> 
> >> 	return USB_EXPLICIT_STATUS_STAGE;
> >> 
> >> then on dwc3 you would:
> >> 
> >> switch (ret) {
> >> case USB_EXPLICIT_STATUS_STAGE:
> >> 
> >> case USB_GADGET_DELAYED_STATUS:
> >> 	wait_for_status_request_queue();
> >> 	
> >>         break;
> >> 
> >> default:
> >> 	start_status_stage();
> >> 
> >> }
> >> 
> >> If this works with dwc3 + uvc, then we have a good recipe on how to
> >> implement for the other drivers.
> > 
> > Given that we need to delay the status stage and not the data stage, we
> > can't explicitly request the status stage through a usb request queue.
> 
> Why not?  The status stage for a control-OUT transfer is simply a
> zero-length IN transaction.  It's easy to queue a request for such a
> transaction.  Is the issue that there's no way to specify the direction
> of the request (hence no direct way to tell whether a zero-length
> request is for the data stage or the status stage)?

OK, I suppose we could queue a request for this, in which case we would have 
to queue two requests for control OUT transfers (one for the data stage and 
one for the status stage). I'm however not convinced that would be the best 
API to handle the status stage, as the function driver would need to queue a 
request and the UDC would then need to check whether that request corresponds 
to a status stage and process it accordingly. A new operation specific to this 
would be easier for both the function driver and the UDC in my opinion. 
There's also the fact that requests can specify a completion handler, but only 
the data stage request would see its completion handler called (unless we 
require UDCs to call completion requests at the completion of the status 
stage, but I'm not sure that all UDCs can report the event to the driver, and 
that would likely be useless as nobody needs that feature).

> Admittedly, it might be nice to provide a library routine in the UDC
> core to queue such requests, since it involves a bunch of uninteresting
> boilerplate operations.
> 
> > Would a new explicit function call work for you for that purpose ?
> 
> It would be okay, but I question whether one is really needed.

I think the API would be cleaner, but it might just be a matter of taste.

> >>> Alan, Felipe, how do we move forward with this ? There are several
> >>> issues with the existing control request handling mechanism in the USB
> >>> gadget stack, and while Paul could work on improving the mechanism, we
> >>> need to provide clear guidance regarding the direction we want to
> >>> take.
> >>> 
> >>> For reference, the issues I know about for the USB_GADGET_DELAYED_STATUS
> >>> mechanism are
> >>> 
> >>> - The mechanism is inherently racy. It relies on signaling the delay
> >>> at the very end of the processing in the setup handler, which by
> >>> definition occurs after the work to process the control request is
> >>> queued (in the generic sense, regardless of whether this involves a
> >>> kernel workqueue or passing the work to userspace). There is thus a race
> >>> window after queuing the work and before signaling the delay during
> >>> which the work handler could signal completion.
> 
> I'm not at all sure what you're talking about here.  Do you mean this
> code near the end of composite_setup()?
> 
> check_value:
> 	/* respond with data transfer before status phase? */
> 	if (value >= 0 && value != USB_GADGET_DELAYED_STATUS) {
> 		req->length = value;
> 		req->context = cdev;
> 		req->zero = value < w_length;
> 		value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> 
> Plainly the code checks for USB_GADGET_DELAYED_STATUS _before_ queuing
> req, not after.  So where's the race?

Please see above.

> >> We won't fix this until all functions and UDCs are converted over, but
> >> it's doable.
> > 
> > It could be fixed by signaling the delay through an explicit function call
> > before queueing the work instead of through a return value though, but I
> > agree that long term requesting the status stage explicitly would likely
> > be cleaner.
> 
> Yes, I agree that relying on an implicit status stage is not a good
> idea for the long term.
> 
> >>> - The mechanism is poorly documented. As Paul mentioned, comments in
> >>> the code state that USB_GADGET_DELAYED_STATUS delay the "data/status
> >>> stages". This is very unclear, and the only three UDCs that implement
> >>> the mechanism seem to do so in different ways:
> 
> We can fix comments and documentation pretty easily.  :-)

It's harder to fix them if different implementations interpret them in 
different ways :-) That might not be the case though, as mentioned above I 
haven't studied the three UDCs that implement this in details, I only had a 
look at the dwc3.

> >>>   - The mtu driver states in a comment that it will "handle the delay
> >>>   STATUS phase till receive ep_queue on ep0".
> >>>   
> >>>   - The bdc driver states in a comment that "The ep0 state will remain
> >>>   WAIT_FOR_DATA_START till we received ep_queue on ep0".
> >>>   
> >>>   - The dwc3 driver seems to handle USB_GADGET_DELAYED_STATUS for the
> >>>   SET_CONFIG request only.
> >> 
> >> that's the only one that has needed it so far. I'm all for making status
> >> stage ALWAYS explicit, that will, in the long run, simplify UDC
> >> drivers and make the API easier to understand.
> >> 
> >>> - The mechanism relies on queueing a request to the UDC to signal that
> >>> it should continue with the status stage. That request can be queued
> >>> either by the USB gadget function driver directly, or by the composite
> >>> layer in usb_composite_setup_continue() (the latter is restricted to
> >>> requests that carry no data as it sets the request length to 0). This is
> >>> problematic if we want to delay the status phase after completing the
> >>> data phase, in order to validate the setup phase data and the data phase
> >>> data (for a control OUT request) together.
> >> 
> >> It shouldn't cause problems, actually. Most of the issues come from the
> >> fact that sometimes gadget driver just returns a success and expects UDC
> >> to initiate status stage and sometimes gadget driver wants to handle
> >> status stage explicitly.
> > 
> > Requesting the status stage explicitly requires an API to do so (quite
> > obviously). Your proposal is to use the usb request queue as a signal to
> > continue to the next stage. My point is that this can't work for control
> > OUT requests, where we may want to delay the status stage after the data
> > is received by the UDC, and thus after the request is queued. We need a
> > different API for that.
> 
> Why so?  Consider the following logic for ep0 in the UDC:
> 
> 	A SETUP packet is received, for an OUT transfer.
> 
> 	If setup.wLength > 0 then the next request queued for ep0 is
> 	the data stage, so it is an OUT request.
> 
> 	Otherwise (i.e., if setup.wLength == 0 or for the second
> 	request queued after the SETUP is received), the next request
> 	is the status stage, so it must be a 0-length IN request.

As explained above, yes, I agree that we could use the request queue operation 
both to queue a request for the data stage, and to request processing the 
status stage. I still think a separate function would be best to request 
processing the status stage (at the very least as a helper that would queue 
the request), but I could be convinced otherwise.

> This requires the UDC to specifically keep track of the direction of
> the current transfer and whether or not a data-stage transfer has
> already been queued.  That shouldn't be hard.

It's "just" a state machine so it wouldn't be too hard. What we need to agree 
on is how the state machine operates, and then the API to control it. That's 
what I tried to describe below in my previous e-mail.

> (But it does involve a
> race in cases where the host gets tired of waiting and issues another
> SETUP packet before the processing of the first transfer is finished.)

I wonder how many UDCs handle that race correctly today :-)

> The corresponding logic for IN transfers is simpler, since IN transfers
> are not allowed to have zero length.  The first request following the
> SETUP packet is the data stage and the second is the status stage.
> 
> > For control IN, we may want to delay the data stage if we can't respond
> > directly, or proceed with the data stage immediately. In both cases this
> > can be signaled by a request being queued. There is no need to delay the
> > status stage as it's initiated by the host.
> 
> Yes.
> 
> > For control OUT, we may want to delay the data stage if we need to
> > validate the setup stage data asynchronously. Proceeding to the data stage
> > can be signaled by queueing a request. We may also want to delay the
> > status stage if we need to validate the data stage data asynchronously.
> > This can't be signaled by queueing a request.
> 
> It can be, if we use the logic outlined above.
> 
> > I wonder if there's really a use case for delaying the data stage of
> > control OUT requests, as it seems to me that we can perform the
> > asynchronous validation of the setup and data stages together, in which
> > case we would always proceed to the data stage, and only potentially
> > delay the status stage. However, if we switch to an explicit API where
> > the transition from the setup to the data stage is triggered by queueing
> > a request, and given that such a transition may need to be delayed for
> > the control IN case, delaying the data stage for control OUT would
> > essentially come for free.

What do you think about this ? Should we allow function drivers to delay the 
data stage of control OUT requests ?

> > In any case we need an API to delay the status stage of a control OUT
> > request. There are two options here. We can consider that the status
> > stage shouldn't be delayed by default and add a new function to be called
> > from the data stage completion handler to request a status stage delay
> > (using the return value of the completion handler isn't a good idea as it
> > would be racy as explained above). A second function would be needed to
> > request the status stage (which, as explained above too, can't be done by
> > queueing a request). A second option is to consider that the status stage
> > is delayed by default until explcitly required. In both cases the same
> > new function is needed to request the status stage.
> > 
> > Note that delaying the data stage and delaying the status stage are two
> > different problems, and don't necessarily need to be solved together.
> > However, if I understand things correctly, we currently delay the data
> > stage of a few control OUT requests (they all have a 0 bytes data stage)
> > as a mean to completion of the request. I believe that this use case
> > could be implemented by delaying the status stage instead, so the two are
> > still related in a way.
> > 
> > If we end up moving to explicit state handling, with the data stage being
> > entered by queueing a request, and the status stage being entered by
> > calling a new function, control OUT requests with 0 bytes of data that
> > can be handled synchronously in the setup handler would require function
> > drivers to both queue a zero-length request and call the status function.
> > This would make the function code more complex, and I wonder whether a
> > shortcut would be a good idea, perhaps in the form of a flag in the
> > request that tells the UDC to automatically proceed to the status stage
> > immediately after the data stage. Or we could make that behaviour the
> > default when the request doesn't have a completion handler (as moving
> > explicitly to the status stage should be done at the earliest from the
> > data stage completion handler).

From an API point of view, towards function drivers, I really want an explicit 
function to proceed with the status stage. That could internally queue a ZLP 
request or call another API, but in any case I don't want the status stage ZLP 
request to be visible to the function drivers. Do you agree with this ?

To simplify function drivers, do you think the above proposal of adding a flag 
to the (data stage) request to request an automatic transition to the status 
stage is a good idea ? We could even possibly invert the logic and transition 
to the status stage when the flag is not set. Essentially this would call the 
status stage request function right after the data stage request completion 
handler returns, instead of forcing all function drivers to do so explicitly 
at the end of the completion handler.

> >>> For those reasons I think a new mechanism is needed. It should either
> >>> signal the status phase delay through an explicit function call
> >>> instead of a return value (to solve the race mentioned above), or by
> >>> requiring all requests to be explicitly completed (but that will require
> >>> changing all USB function drivers). Furthermore, the mechanism need to
> >>> support delaying the status phase after queuing the request for the data
> >>> phase, so we need an explicit way to signal that the UDC should proceed
> >>> with the status phase, other than queueing the request.
> >>> 
> >>> Thoughts ? Preferences ?
> >> 
> >> how about making status stage always explicit?
> > 
> > If we implement a proof of concept, could you help us converting drivers
> > over to the new API ? I'll assume we'll have to address all UDCs first,
> > and then the function drivers.
> 
> I'll certainly help for the drivers I'm familiar with.

Thank you.

Alan Stern Nov. 2, 2018, 7:46 p.m. UTC | #10

On Fri, 2 Nov 2018, Laurent Pinchart wrote:

> Hi Alan,

Hi, Laurent.

> The composite layer only handles USB_GADGET_DELAYED_STATUS for 
> USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in 
> composite_setup()). It increments cdev->delayed_status immediately. Then, in 
> usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues 
> a ZLP, and warns otherwise.
> 
> This mechanism delays the data stage, not the status stage (or, to be precise, 
> it delays the status stage insofar as the status stage comes after the data 
> stage), and only supports control OUT requests with 0 bytes of data (which is 
> the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all 
> other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the 
> UDC.
> 
> The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a 
> delayed_status flag in an internal structure. I haven't inspected in details 
> what they do next as I'm not familiar with all of them, but the dwc3 driver 
> just skips the handling of the status phase in dwc3_ep0_xfernotready() and 
> delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests, 
> with no data phase.
> 
> Even when limited to 0-length control OUT requests, this mechanism is racy. 
> The setup handler, when it wants to delay the status phase, will queue 
> asynchronous work that will, when it completes, call 
> usb_composite_setup_continue() to proceed with the status phase. Queuing the 
> work has to be done before the setup handler returns, and the cdev-
> >delayed_status is only incremented after the setup handler returns, in 
> composite_setup(). There is thus a time window during which the asynchronous 
> work can call usb_composite_setup_continue() before cdev->delayed_status has 
> been incremented. We have managed to hit this in practice, with a surprisingly 
> high rate seeing how small the window is.

I see.  Thanks for the detailed explanation.

> Now that I've written all this, I realize that cdev->delayed_status is guarded 
> by cdev->lock. I thus wonder whether our analysis was correct, or if we were 
> hitting a different bug :-S Paul, could you test this again ? Please note, 
> however, that the race described here is not related to this patch series, 
> except in how it influences the API design to avoid race conditions.

Perhaps the async work routine doesn't acquire cdev->lock.

> > Assuming you are correct, wouldn't it make sense to fix or eliminate
> > the race by changing composite.c?
> 
> I was about to write that we would need to lock access to cdev-
> >delayed_status, and found out that we already use cdev->lock to do so. More 
> investigations are needed.
> 
> Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length 
> control OUT requests, so the problem that led to this patch series still 
> exists, even if the race condition I thought was there doesn't exist.

Yes, it is too limited.


> > > Given that we need to delay the status stage and not the data stage, we
> > > can't explicitly request the status stage through a usb request queue.
> > 
> > Why not?  The status stage for a control-OUT transfer is simply a
> > zero-length IN transaction.  It's easy to queue a request for such a
> > transaction.  Is the issue that there's no way to specify the direction
> > of the request (hence no direct way to tell whether a zero-length
> > request is for the data stage or the status stage)?
> 
> OK, I suppose we could queue a request for this, in which case we would have 
> to queue two requests for control OUT transfers (one for the data stage and 
> one for the status stage). I'm however not convinced that would be the best 
> API to handle the status stage, as the function driver would need to queue a 
> request and the UDC would then need to check whether that request corresponds 
> to a status stage and process it accordingly. A new operation specific to this 
> would be easier for both the function driver and the UDC in my opinion. 
> There's also the fact that requests can specify a completion handler, but only 
> the data stage request would see its completion handler called (unless we 
> require UDCs to call completion requests at the completion of the status 
> stage, but I'm not sure that all UDCs can report the event to the driver, and 
> that would likely be useless as nobody needs that feature).
> 
> > Admittedly, it might be nice to provide a library routine in the UDC
> > core to queue such requests, since it involves a bunch of uninteresting
> > boilerplate operations.
> > 
> > > Would a new explicit function call work for you for that purpose ?
> > 
> > It would be okay, but I question whether one is really needed.
> 
> I think the API would be cleaner, but it might just be a matter of taste.

I have to agree that a separate function would be cleaner.  Whether
this function should be part of the gadget API or just a core library
routine is something we need to decide.


> > (But it does involve a
> > race in cases where the host gets tired of waiting and issues another
> > SETUP packet before the processing of the first transfer is finished.)
> 
> I wonder how many UDCs handle that race correctly today :-)

Probably none of them.  I can think of two ways of doing it:

	Have the UDC driver associate a new integer tag with each
	SETUP packet, and have function drivers include the
	corresponding tag when they queue a request on ep0.  Then the
	UDC driver could refuse requests whose tag was out of date.

	Create an new API routine for function drivers to call in
	their setup handler.  This routine would acknowledge receipt
	of the new SETUP packet, assuring the UDC driver that no more
	requests would be queued in response to earlier SETUPs.  Until
	the routine was called, the UDC would refuse all requests for
	ep0.  Function drivers would be responsible for their own
	internal synchronization.

Both of these would involve nontrivial API changes, although the first
could be relatively uninvasive.  I doubt either of them is worthwhile
at this point.

There's a similar race at the hardware level.  What happens if the
controller receives a new SETUP packet and concurrently the driver is
setting up the controller registers for a response to an earlier
SETUP?  I don't know how real controllers handle this.


> > > I wonder if there's really a use case for delaying the data stage of
> > > control OUT requests, as it seems to me that we can perform the
> > > asynchronous validation of the setup and data stages together, in which
> > > case we would always proceed to the data stage, and only potentially
> > > delay the status stage. However, if we switch to an explicit API where
> > > the transition from the setup to the data stage is triggered by queueing
> > > a request, and given that such a transition may need to be delayed for
> > > the control IN case, delaying the data stage for control OUT would
> > > essentially come for free.
> 
> What do you think about this ? Should we allow function drivers to delay the 
> data stage of control OUT requests ?

You mean, should we allow function drivers to queue the data-stage
request after the setup handler has returned?  I don't see any reason
why not.  After all, some drivers may require this.  Likewise for the 
data stage of a control-IN.

Another thing we should do is give function drivers a way to send a
STALL response for the status stage.  Currently there's no way to do
it, if a data stage is present.


> From an API point of view, towards function drivers, I really want an explicit 
> function to proceed with the status stage. That could internally queue a ZLP 
> request or call another API, but in any case I don't want the status stage ZLP 
> request to be visible to the function drivers. Do you agree with this ?

It's okay with me.

> To simplify function drivers, do you think the above proposal of adding a flag 
> to the (data stage) request to request an automatic transition to the status 
> stage is a good idea ? We could even possibly invert the logic and transition 
> to the status stage when the flag is not set. Essentially this would call the 
> status stage request function right after the data stage request completion 
> handler returns, instead of forcing all function drivers to do so explicitly 
> at the end of the completion handler.

That makes sense.  Function drivers then wouldn't have to be aware of
the new API.  We'd only need to convert the UDC drivers (plus the users
of USB_GADGET_DELAYED_STATUS).

Should control-IN transfers use the flag also?  I can't imagine why
anyone would want to delay or otherwise alter a control-IN status
stage.

Alan Stern

Felipe Balbi Nov. 6, 2018, 11:17 a.m. UTC | #11

Hi,

Laurent Pinchart <[e-mail]> writes:
>> >>>> Furthermore, we have found that USB_GADGET_DELAYED_STATUS is racey,
>> >>>> which has already been observed in the UVC gadget driver previously
>> >>>> [0]. The raceiness stems from the fact that things can happen in
>> >>>> between returning USB_GADGET_DELAYED_STATUS and the composite layer
>> >>>> reacting to it - especially if usb_composite_setup_continue is called
>> >>>> within that window it causes a WARN. In any case, the fact that the
>> >>>> mechanism itself is racey suggests that it needs improvement, and using
>> >>>> it wouldn't be a good solution in this case.
>> 
>> I don't understand this at all.  The composite layer reacts to
>> USB_GADGET_DELAYED_STATUS as soon as it receives the return value.  Can
>> Paul or Laurent give a more explicit example of this race?
>
> The composite layer only handles USB_GADGET_DELAYED_STATUS for 
> USB_REQ_SET_CONFIGURATION (in set_config()) and for USB_REQ_SET_INTERFACE (in 
> composite_setup()). It increments cdev->delayed_status immediately. Then, in 
> usb_composite_setup_continue(), if cdev->delayed_status is not zero, it queues 
> a ZLP, and warns otherwise.
>
> This mechanism delays the data stage, not the status stage (or, to be precise, 
> it delays the status stage insofar as the status stage comes after the data 
> stage), and only supports control OUT requests with 0 bytes of data (which is 
> the case of both USB_REQ_SET_INTERFACE and USB_REQ_SET_CONFIGURATION). For all 
> other requests, the composite layer passes USB_GADGET_DELAYED_STATUS to the 
> UDC.

DATA stage always depends on a usb_ep_queue() from gadget driver. So
it's always "delayed" in that sense.

> The three UDCs that implement USB_GADGET_DELAYED_STATUS support set a 
> delayed_status flag in an internal structure. I haven't inspected in details 
> what they do next as I'm not familiar with all of them, but the dwc3 driver 
> just skips the handling of the status phase in dwc3_ep0_xfernotready() and 
> delays it to __dwc3_gadget_ep0_queue(). This only works for 0-length requests, 
> with no data phase.

data stage always depends on usb_ep_queue(). There was never any need
for UDC to handle Data stage internally. Also, status stage is always a ZLP.

> Even when limited to 0-length control OUT requests, this mechanism is racy. 
> The setup handler, when it wants to delay the status phase, will queue 
> asynchronous work that will, when it completes, call 
> usb_composite_setup_continue() to proceed with the status phase. Queuing the 
> work has to be done before the setup handler returns, and the cdev-
>>delayed_status is only incremented after the setup handler returns, in 
> composite_setup(). There is thus a time window during which the asynchronous 
> work can call usb_composite_setup_continue() before cdev->delayed_status has 
> been incremented. We have managed to hit this in practice, with a surprisingly 
> high rate seeing how small the window is.

that's only the case because we have two different "modes" for this. One
where UDC handles it internally and another where gadget driver has to
queue a request. I'm vouching for making status stage always explicit,
i.e. we should always expect a usb_ep_queue().

> Now that I've written all this, I realize that cdev->delayed_status is guarded 
> by cdev->lock. I thus wonder whether our analysis was correct, or if we were 
> hitting a different bug :-S Paul, could you test this again ? Please note, 
> however, that the race described here is not related to this patch series, 
> except in how it influences the API design to avoid race conditions.
>
>> Assuming you are correct, wouldn't it make sense to fix or eliminate
>> the race by changing composite.c?
>
> I was about to write that we would need to lock access to cdev-
>>delayed_status, and found out that we already use cdev->lock to do so. More 
> investigations are needed.
>
> Please note, however, that USB_GADGET_DELAYED_STATUS is limited to 0-length 
> control OUT requests, so the problem that led to this patch series still 
> exists, even if the race condition I thought was there doesn't exist.

And that problem is...?

DATA stage always depends on a usb_ep_queue() from gadget driver.

>> >> If this works with dwc3 + uvc, then we have a good recipe on how to
>> >> implement for the other drivers.
>> > 
>> > Given that we need to delay the status stage and not the data stage, we
>> > can't explicitly request the status stage through a usb request queue.
>> 
>> Why not?  The status stage for a control-OUT transfer is simply a
>> zero-length IN transaction.  It's easy to queue a request for such a
>> transaction.  Is the issue that there's no way to specify the direction
>> of the request (hence no direct way to tell whether a zero-length
>> request is for the data stage or the status stage)?
>
> OK, I suppose we could queue a request for this, in which case we would have 
> to queue two requests for control OUT transfers (one for the data stage and 
> one for the status stage). I'm however not convinced that would be the best 

that's correct. This is what "make status stage always explicit" mean :)

> API to handle the status stage, as the function driver would need to queue a 

it avoids all the special cases. UDC drivers can implement a single
handling for struct usb_request. We could do away with special return
values and so on...

> request and the UDC would then need to check whether that request corresponds 
> to a status stage and process it accordingly. A new operation specific to this 

no, it wouldn't. UDC would have to check the size of request, that's
all:

	if (r->length == 0)
        	special_zlp_handling();
	else
        	regular_non_zlp_handling();

But we don't need to care about special return values and the like. We
don't even need to care (from UDC perspective) if we're dealing with
2-stage or 3-stage control transfers (well, dwc3 needs to care because
of different TRB types that needs to be used, but that's another story)

> would be easier for both the function driver and the UDC in my opinion. 

it wouldn't. We would just be moving the special case to another
function, rather than eliminating it.

> There's also the fact that requests can specify a completion handler, but only 
> the data stage request would see its completion handler called (unless we 
> require UDCs to call completion requests at the completion of the status 
> stage, but I'm not sure that all UDCs can report the event to the driver, and 
> that would likely be useless as nobody needs that feature).

you still wanna know if the host actually processed your status
stage. udc-core can (and should) provide a generic status stage
completion function which, at a minimum, aids with some tracepoints.

One way to satisfy what you want, with what I want is to have UDC core
implement something like below:

int usb_ep_start_status_stage(struct usb_gadget *g)
{
	return usb_ep_queue(g->ep0, &g->ep0_status_request);
}

special function for you, usb_ep_queue() for me :-p

>> Admittedly, it might be nice to provide a library routine in the UDC
>> core to queue such requests, since it involves a bunch of uninteresting
>> boilerplate operations.
>> 
>> > Would a new explicit function call work for you for that purpose ?
>> 
>> It would be okay, but I question whether one is really needed.
>
> I think the API would be cleaner, but it might just be a matter of taste.

From a UDC perspective, I'm more inclined to removing special cases,
rather than making them more apparent. Having a single method for
handling all three stages of a control transfer, IMO, is far more
beneficial as it removes magic return values and several branches on UDC
driver.

>> >>> - The mechanism is poorly documented. As Paul mentioned, comments in
>> >>> the code state that USB_GADGET_DELAYED_STATUS delay the "data/status
>> >>> stages". This is very unclear, and the only three UDCs that implement
>> >>> the mechanism seem to do so in different ways:
>> 
>> We can fix comments and documentation pretty easily.  :-)
>
> It's harder to fix them if different implementations interpret them in 
> different ways :-) That might not be the case though, as mentioned above I 

that's where a proper audit of the code comes into play :)

> haven't studied the three UDCs that implement this in details, I only had a 
> look at the dwc3.

that's perfectly fine. We can Cc other folks involved with the other
UDCs and have them chip in.

>> This requires the UDC to specifically keep track of the direction of
>> the current transfer and whether or not a data-stage transfer has
>> already been queued.  That shouldn't be hard.
>
> It's "just" a state machine so it wouldn't be too hard. What we need to agree 
> on is how the state machine operates, and then the API to control it. That's 
> what I tried to describe below in my previous e-mail.
>
>> (But it does involve a
>> race in cases where the host gets tired of waiting and issues another
>> SETUP packet before the processing of the first transfer is finished.)

Host would stall first in that case. Driver is already required to
handle stalls for several other conditions. If thehre are bugs in that
area, I'd prefer catching them.

>> > I wonder if there's really a use case for delaying the data stage of
>> > control OUT requests, as it seems to me that we can perform the
>> > asynchronous validation of the setup and data stages together, in which
>> > case we would always proceed to the data stage, and only potentially
>> > delay the status stage. However, if we switch to an explicit API where
>> > the transition from the setup to the data stage is triggered by queueing
>> > a request, and given that such a transition may need to be delayed for
>> > the control IN case, delaying the data stage for control OUT would
>> > essentially come for free.
>
> What do you think about this ? Should we allow function drivers to delay the 
> data stage of control OUT requests ?

it's already delayed. UDC won't start data stage unless it has a buffer
to put the data. Without a usb_ep_queue(), UDC doesn't have a buffer.

>> > If we end up moving to explicit state handling, with the data stage being
>> > entered by queueing a request, and the status stage being entered by
>> > calling a new function, control OUT requests with 0 bytes of data that
>> > can be handled synchronously in the setup handler would require function
>> > drivers to both queue a zero-length request and call the status function.
>> > This would make the function code more complex, and I wonder whether a
>> > shortcut would be a good idea, perhaps in the form of a flag in the
>> > request that tells the UDC to automatically proceed to the status stage
>> > immediately after the data stage. Or we could make that behaviour the
>> > default when the request doesn't have a completion handler (as moving
>> > explicitly to the status stage should be done at the earliest from the
>> > data stage completion handler).
>
> From an API point of view, towards function drivers, I really want an explicit 
> function to proceed with the status stage. That could internally queue a ZLP 
> request or call another API, but in any case I don't want the status stage ZLP 
> request to be visible to the function drivers. Do you agree with this ?

why can't it be visible? I don't mind having a function wrapping
usb_ep_queue(), but why is it bad to have functions call usb_ep_queue()
directly?

> To simplify function drivers, do you think the above proposal of adding a flag 
> to the (data stage) request to request an automatic transition to the status 
> stage is a good idea ? We could even possibly invert the logic and transition 

no, I don't think so. Making the status phase always explicit is far
better. UDCs won't have to check flags, or act on magic return
values. It just won't do anything until a request is queued.

Felipe Balbi Nov. 6, 2018, 11:24 a.m. UTC | #12

Hi,

Alan Stern <[e-mail]> writes:
> There's a similar race at the hardware level.  What happens if the
> controller receives a new SETUP packet and concurrently the driver is
> setting up the controller registers for a response to an earlier
> SETUP?  I don't know how real controllers handle this.

That's HW implementation detail. DWC3, for instance, will ignore the
TRBs and return me the status "setup packet pending". Then I just start
a new SETUP TRB.

>> > > I wonder if there's really a use case for delaying the data stage of
>> > > control OUT requests, as it seems to me that we can perform the
>> > > asynchronous validation of the setup and data stages together, in which
>> > > case we would always proceed to the data stage, and only potentially
>> > > delay the status stage. However, if we switch to an explicit API where
>> > > the transition from the setup to the data stage is triggered by queueing
>> > > a request, and given that such a transition may need to be delayed for
>> > > the control IN case, delaying the data stage for control OUT would
>> > > essentially come for free.
>> 
>> What do you think about this ? Should we allow function drivers to delay the 
>> data stage of control OUT requests ?
>
> You mean, should we allow function drivers to queue the data-stage
> request after the setup handler has returned?  I don't see any reason

that's already done:

static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
			const struct dwc3_event_depevt *event)
{
	struct dwc3_ep		*dep = dwc->eps[event->endpoint_number];

	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
	dep->resource_index = 0;
	dwc->setup_packet_pending = false;

	switch (dwc->ep0state) {
	case EP0_SETUP_PHASE:
		dwc3_ep0_inspect_setup(dwc, event);
		break;
[...]
}

static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
		const struct dwc3_event_depevt *event)
{
	struct usb_ctrlrequest *ctrl = (void *) dwc->ep0_trb;
	int ret = -EINVAL;
	u32 len;

	if (!dwc->gadget_driver)
		goto out;

	trace_dwc3_ctrl_req(ctrl);

	len = le16_to_cpu(ctrl->wLength);
	if (!len) {
		dwc->three_stage_setup = false;
		dwc->ep0_expect_in = false;
		dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
	} else {
		dwc->three_stage_setup = true;
		dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
		dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
	}
[...]
}

static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
		struct dwc3_request *req)
{
	struct dwc3		*dwc = dep->dwc;

	req->request.actual	= 0;
	req->request.status	= -EINPROGRESS;
	req->epnum		= dep->number;

	list_add_tail(&req->list, &dep->pending_list);

[...]

	if (dwc->three_stage_setup) {
		unsigned        direction;

		direction = dwc->ep0_expect_in;
		dwc->ep0state = EP0_DATA_PHASE;

		__dwc3_ep0_do_control_data(dwc, dwc->eps[direction], req);

		dep->flags &= ~DWC3_EP0_DIR_IN;
	}

	return 0;
}

Regardless of the direction, control data always depends on a call to
usb_ep_queue()

> why not.  After all, some drivers may require this.  Likewise for the 
> data stage of a control-IN.
>
> Another thing we should do is give function drivers a way to send a
> STALL response for the status stage.  Currently there's no way to do
> it, if a data stage is present.

Status stage can only be stalled if host tries to move data on the wrong
direction. Currently, that's handled internally by UDCs since that's
easy enough to track.

Data stage already has explicit stall handling.

Alan Stern Nov. 6, 2018, 2:51 p.m. UTC | #13

On Tue, 6 Nov 2018, Felipe Balbi wrote:

> DATA stage always depends on a usb_ep_queue() from gadget driver. So
> it's always "delayed" in that sense.

However, it's conceivable that some UDC drivers might behave 
differently depending on whether the usb_ep_queue call occurs within 
the setup callback or after that callback returns.  They _shouldn't_, 
but they might.

> it avoids all the special cases. UDC drivers can implement a single
> handling for struct usb_request. We could do away with special return
> values and so on...

It's not quite so simple, because the UDC driver will need to keep 
track of whether a request queued on ep0 should be in the IN or the OUT 
direction.  (Maybe they have to do this already, I don't know.)

> > request and the UDC would then need to check whether that request corresponds 
> > to a status stage and process it accordingly. A new operation specific to this 
> 
> no, it wouldn't. UDC would have to check the size of request, that's
> all:
> 
> 	if (r->length == 0)
>         	special_zlp_handling();
> 	else
>         	regular_non_zlp_handling();

Checking the length isn't enough.  A data stage can have 0 length.

> But we don't need to care about special return values and the like. We
> don't even need to care (from UDC perspective) if we're dealing with
> 2-stage or 3-stage control transfers (well, dwc3 needs to care because
> of different TRB types that needs to be used, but that's another story)

No, we do need to care because of the direction issue.

> > There's also the fact that requests can specify a completion handler, but only 
> > the data stage request would see its completion handler called (unless we 
> > require UDCs to call completion requests at the completion of the status 
> > stage, but I'm not sure that all UDCs can report the event to the driver, and 
> > that would likely be useless as nobody needs that feature).
> 
> you still wanna know if the host actually processed your status
> stage. udc-core can (and should) provide a generic status stage
> completion function which, at a minimum, aids with some tracepoints.

Helping with tracepoints is fine.  However, I don't think function 
drivers really need to know whether the status stage was processed by 
the host.  Can you point out any examples where such information would 
be useful?

> One way to satisfy what you want, with what I want is to have UDC core
> implement something like below:
> 
> int usb_ep_start_status_stage(struct usb_gadget *g)
> {
> 	return usb_ep_queue(g->ep0, &g->ep0_status_request);
> }
> 
> special function for you, usb_ep_queue() for me :-p

Sure, this is one of the options Laurent and I have discussed.

> >> (But it does involve a
> >> race in cases where the host gets tired of waiting and issues another
> >> SETUP packet before the processing of the first transfer is finished.)
> 
> Host would stall first in that case.

I don't follow.  Suppose the host sends a SETUP packet for an IN 
transfer, but the gadget takes so long to send the IN data back that 
the host times out.  So then the host sends a SETUP packet for a new 
transfer.  No stalls.

(Besides, hosts never send STALL packets anyway.  Only peripherals do.)

> Driver is already required to
> handle stalls for several other conditions. If thehre are bugs in that
> area, I'd prefer catching them.

> > To simplify function drivers, do you think the above proposal of adding a flag 
> > to the (data stage) request to request an automatic transition to the status 
> > stage is a good idea ? We could even possibly invert the logic and transition 
> 
> no, I don't think so. Making the status phase always explicit is far
> better. UDCs won't have to check flags, or act on magic return
> values. It just won't do anything until a request is queued.

I don't agree.  This would be a simple test in a localized area (the 
completion callback for control requests).  It could even be 
implemented by a library routine; the UDC driver would simply have to 
call this routine immediately after invoking the callback.

Alan Stern

Alan Stern Nov. 6, 2018, 3:01 p.m. UTC | #14

On Tue, 6 Nov 2018, Felipe Balbi wrote:

> 
> Hi,
> 
> Alan Stern <[e-mail]> writes:
> > There's a similar race at the hardware level.  What happens if the
> > controller receives a new SETUP packet and concurrently the driver is
> > setting up the controller registers for a response to an earlier
> > SETUP?  I don't know how real controllers handle this.
> 
> That's HW implementation detail. DWC3, for instance, will ignore the
> TRBs and return me the status "setup packet pending". Then I just start
> a new SETUP TRB.

You mean the UDC hardware sets a "setup pending" flag in some register,
and then ignores any attempts to do anything with ep0 until the driver
clears this flag?

We could do something similar at the software level.  In fact, that
would be one of the two proposals I outlined in an earlier email.

> > You mean, should we allow function drivers to queue the data-stage
> > request after the setup handler has returned?  I don't see any reason
> 
> that's already done:
> 
> static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
> 			const struct dwc3_event_depevt *event)
> {
> 	struct dwc3_ep		*dep = dwc->eps[event->endpoint_number];
> 
> 	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> 	dep->resource_index = 0;
> 	dwc->setup_packet_pending = false;
> 
> 	switch (dwc->ep0state) {
> 	case EP0_SETUP_PHASE:
> 		dwc3_ep0_inspect_setup(dwc, event);
> 		break;
> [...]
> }

...

You mean, it's already done in DWC3.  What about other UDC drivers?

> > why not.  After all, some drivers may require this.  Likewise for the 
> > data stage of a control-IN.
> >
> > Another thing we should do is give function drivers a way to send a
> > STALL response for the status stage.  Currently there's no way to do
> > it, if a data stage is present.
> 
> Status stage can only be stalled if host tries to move data on the wrong
> direction.

The USB-2 spec disagrees.  See Table 8-7 in section 8.5.3.1 and the
following paragraphs.  (Although, I can't see why a function would ever
fail to complete the command sequence for a control-IN transfer after
the data had already been sent.)

Alan Stern

Felipe Balbi Nov. 7, 2018, 6:53 a.m. UTC | #15

Hi,

Alan Stern <[e-mail]> writes:
>> Alan Stern <[e-mail]> writes:
>> > There's a similar race at the hardware level.  What happens if the
>> > controller receives a new SETUP packet and concurrently the driver is
>> > setting up the controller registers for a response to an earlier
>> > SETUP?  I don't know how real controllers handle this.
>> 
>> That's HW implementation detail. DWC3, for instance, will ignore the
>> TRBs and return me the status "setup packet pending". Then I just start
>> a new SETUP TRB.
>
> You mean the UDC hardware sets a "setup pending" flag in some register,
> and then ignores any attempts to do anything with ep0 until the driver
> clears this flag?

Yes, except that the "flag" is a status on the TRB itself (TRB is dwc3's
DMA transfer descriptor).

>> > You mean, should we allow function drivers to queue the data-stage
>> > request after the setup handler has returned?  I don't see any reason
>> 
>> that's already done:
>> 
>> static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
>> 			const struct dwc3_event_depevt *event)
>> {
>> 	struct dwc3_ep		*dep = dwc->eps[event->endpoint_number];
>> 
>> 	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>> 	dep->resource_index = 0;
>> 	dwc->setup_packet_pending = false;
>> 
>> 	switch (dwc->ep0state) {
>> 	case EP0_SETUP_PHASE:
>> 		dwc3_ep0_inspect_setup(dwc, event);
>> 		break;
>> [...]
>> }
>
> ...
>
> You mean, it's already done in DWC3.  What about other UDC drivers?

if they're not implementing this possibility, then that's a bug on
those UDC drivers :)

>> > why not.  After all, some drivers may require this.  Likewise for the 
>> > data stage of a control-IN.
>> >
>> > Another thing we should do is give function drivers a way to send a
>> > STALL response for the status stage.  Currently there's no way to do
>> > it, if a data stage is present.
>> 
>> Status stage can only be stalled if host tries to move data on the wrong
>> direction.
>
> The USB-2 spec disagrees.  See Table 8-7 in section 8.5.3.1 and the
> following paragraphs.  (Although, I can't see why a function would ever
> fail to complete the command sequence for a control-IN transfer after
> the data had already been sent.)

I can't see how we could ever STALL after returning the data requested
by the host. Seems like that wasn't well thought out.

Felipe Balbi Nov. 7, 2018, 7 a.m. UTC | #16

Hi,

Alan Stern <[e-mail]> writes:
>> DATA stage always depends on a usb_ep_queue() from gadget driver. So
>> it's always "delayed" in that sense.
>
> However, it's conceivable that some UDC drivers might behave 
> differently depending on whether the usb_ep_queue call occurs within 
> the setup callback or after that callback returns.  They _shouldn't_, 
> but they might.

but now we're speculating. Should we really care before we catch
regressions?

>> it avoids all the special cases. UDC drivers can implement a single
>> handling for struct usb_request. We could do away with special return
>> values and so on...
>
> It's not quite so simple, because the UDC driver will need to keep 
> track of whether a request queued on ep0 should be in the IN or the OUT 
> direction.  (Maybe they have to do this already, I don't know.)

UDC drivers already have to do that.

>> > request and the UDC would then need to check whether that request corresponds 
>> > to a status stage and process it accordingly. A new operation specific to this 
>> 
>> no, it wouldn't. UDC would have to check the size of request, that's
>> all:
>> 
>> 	if (r->length == 0)
>>         	special_zlp_handling();
>> 	else
>>         	regular_non_zlp_handling();
>
> Checking the length isn't enough.  A data stage can have 0 length.

apologies, I meant wLength, like so:

	len = le16_to_cpu(ctrl->wLength);
	if (!len) {
		dwc->three_stage_setup = false;
		dwc->ep0_expect_in = false;
		dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
	} else {
		dwc->three_stage_setup = true;
		dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
		dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
	}

>> But we don't need to care about special return values and the like. We
>> don't even need to care (from UDC perspective) if we're dealing with
>> 2-stage or 3-stage control transfers (well, dwc3 needs to care because
>> of different TRB types that needs to be used, but that's another story)
>
> No, we do need to care because of the direction issue.

special return values would be rendered uncessary if there's agreement
that status stage is always explicit. Why would need
USB_GADGET_DELAYED_STATUS if every case returns that?

>> > There's also the fact that requests can specify a completion handler, but only 
>> > the data stage request would see its completion handler called (unless we 
>> > require UDCs to call completion requests at the completion of the status 
>> > stage, but I'm not sure that all UDCs can report the event to the driver, and 
>> > that would likely be useless as nobody needs that feature).
>> 
>> you still wanna know if the host actually processed your status
>> stage. udc-core can (and should) provide a generic status stage
>> completion function which, at a minimum, aids with some tracepoints.
>
> Helping with tracepoints is fine.  However, I don't think function 
> drivers really need to know whether the status stage was processed by 
> the host.  Can you point out any examples where such information would 
> be useful?

If you know your STATUS stage completed, you have a guarantee that your
previous control transfer is complete. It's a very clear signal that you
should prepare for more control transfers.

>> >> (But it does involve a
>> >> race in cases where the host gets tired of waiting and issues another
>> >> SETUP packet before the processing of the first transfer is finished.)
>> 
>> Host would stall first in that case.
>
> I don't follow.  Suppose the host sends a SETUP packet for an IN 
> transfer, but the gadget takes so long to send the IN data back that 
> the host times out.  So then the host sends a SETUP packet for a new 
> transfer.  No stalls.
>
> (Besides, hosts never send STALL packets anyway.  Only peripherals do.)

oh okay. This is the setup_packet_pending case.

>> > To simplify function drivers, do you think the above proposal of adding a flag 
>> > to the (data stage) request to request an automatic transition to the status 
>> > stage is a good idea ? We could even possibly invert the logic and transition 
>> 
>> no, I don't think so. Making the status phase always explicit is far
>> better. UDCs won't have to check flags, or act on magic return
>> values. It just won't do anything until a request is queued.
>
> I don't agree.  This would be a simple test in a localized area (the 
> completion callback for control requests).  It could even be 
> implemented by a library routine; the UDC driver would simply have to 
> call this routine immediately after invoking the callback.

I don't follow what you mean here.

Alan Stern Nov. 7, 2018, 4:23 p.m. UTC | #17

On Wed, 7 Nov 2018, Felipe Balbi wrote:

> Hi,
> 
> Alan Stern <[e-mail]> writes:
> >> Alan Stern <[e-mail]> writes:
> >> > There's a similar race at the hardware level.  What happens if the
> >> > controller receives a new SETUP packet and concurrently the driver is
> >> > setting up the controller registers for a response to an earlier
> >> > SETUP?  I don't know how real controllers handle this.
> >> 
> >> That's HW implementation detail. DWC3, for instance, will ignore the
> >> TRBs and return me the status "setup packet pending". Then I just start
> >> a new SETUP TRB.
> >
> > You mean the UDC hardware sets a "setup pending" flag in some register,
> > and then ignores any attempts to do anything with ep0 until the driver
> > clears this flag?
> 
> Yes, except that the "flag" is a status on the TRB itself (TRB is dwc3's
> DMA transfer descriptor).

Hmmm.  So there must be a mechanism for the driver to tell the hardware 
that the endpoint's ring should start up again, right?  (I'm assuming 
the controller stops the ring when the SETUP is received, to avoid 
taking invalid actions for TRBs that are now out of date.)


> >> > Another thing we should do is give function drivers a way to send a
> >> > STALL response for the status stage.  Currently there's no way to do
> >> > it, if a data stage is present.
> >> 
> >> Status stage can only be stalled if host tries to move data on the wrong
> >> direction.
> >
> > The USB-2 spec disagrees.  See Table 8-7 in section 8.5.3.1 and the
> > following paragraphs.  (Although, I can't see why a function would ever
> > fail to complete the command sequence for a control-IN transfer after
> > the data had already been sent.)
> 
> I can't see how we could ever STALL after returning the data requested
> by the host. Seems like that wasn't well thought out.

Yes, it doesn't make a lot of sense.  However, STALLing the status
stage of a control-OUT transfer does make sense, so we should be able
to do it.  The obvious approach is to call ep0's set_halt() method
instead of submitting an explicit status request.


> > Checking the length isn't enough.  A data stage can have 0 length.
> 
> apologies, I meant wLength, like so:
> 
> 	len = le16_to_cpu(ctrl->wLength);
> 	if (!len) {
> 		dwc->three_stage_setup = false;
> 		dwc->ep0_expect_in = false;
> 		dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> 	} else {
> 		dwc->three_stage_setup = true;
> 		dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
> 		dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
> 	}

Presumably you invert the value of ep0_expect_in and set ep0_next_event
to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted
for ep0.  Okay.

> special return values would be rendered uncessary if there's agreement
> that status stage is always explicit. Why would need
> USB_GADGET_DELAYED_STATUS if every case returns that?

Agreed.

> >> > There's also the fact that requests can specify a completion handler, but only 
> >> > the data stage request would see its completion handler called (unless we 
> >> > require UDCs to call completion requests at the completion of the status 
> >> > stage, but I'm not sure that all UDCs can report the event to the driver, and 
> >> > that would likely be useless as nobody needs that feature).
> >> 
> >> you still wanna know if the host actually processed your status
> >> stage. udc-core can (and should) provide a generic status stage
> >> completion function which, at a minimum, aids with some tracepoints.
> >
> > Helping with tracepoints is fine.  However, I don't think function 
> > drivers really need to know whether the status stage was processed by 
> > the host.  Can you point out any examples where such information would 
> > be useful?
> 
> If you know your STATUS stage completed, you have a guarantee that your
> previous control transfer is complete. It's a very clear signal that you
> should prepare for more control transfers.

That doesn't seem to make sense, because in reality you don't have
this guarantee.  Consider the following scenario: The host starts a
control-IN transfer.  You send the data-stage request succesfully and
then submit the status-stage request.  That request will complete
before the host receives the ACK for its 0-length status OUT
transaction.  In fact, the host may never receive that ACK and so the
transfer may never complete.

Besides, you don't need a signal (clear or otherwise) to prepare for
more control transfers.  You should start preparing as soon as the
status-stage request has been submitted.  At that point, what else is
there for you to do?

For that matter, you should be prepared to receive a new setup callback 
at any time.  The host doesn't have to wait for an old control transfer 
to complete before starting a new one.

I just don't see any value in knowing the completion code of a
status-stage request.

> >> > To simplify function drivers, do you think the above proposal of adding a flag 
> >> > to the (data stage) request to request an automatic transition to the status 
> >> > stage is a good idea ? We could even possibly invert the logic and transition 
> >> 
> >> no, I don't think so. Making the status phase always explicit is far
> >> better. UDCs won't have to check flags, or act on magic return
> >> values. It just won't do anything until a request is queued.
> >
> > I don't agree.  This would be a simple test in a localized area (the 
> > completion callback for control requests).  It could even be 
> > implemented by a library routine; the UDC driver would simply have to 
> > call this routine immediately after invoking the callback.
> 
> I don't follow what you mean here.

Suppose we have a core library routine like this:

void usb_gadget_control_complete(struct usb_gadget *gadget,
		unsigned int no_implicit_status, int status)
{
	struct usb_request *req;

	if (no_implicit_status || status != 0)
		return;

	/* Send an implicit status-stage request for ep0 */
	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
	if (req) {
		req->length = 0;
		req->no_implicit_status = 1;
		req->complete = /* req's deallocation routine */
		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
	}
}

Then all a UDC driver would need to do is call 
usb_gadget_control_complete() after invoking a control request's 
completion handler.  The no_implicit_status and status arguments would 
be taken from the request that was just completed.

With this one call added to each UDC, all the existing function drivers
would work correctly.  Even though they don't explicitly queue
status-stage requests, the new routine will do so for them,
transparently.  Function drivers that want to handle their own
status-stage requests explicitly will merely have to set the
req->no_implicit_status bit.

(We might or might not need to watch out for 0-length control-OUT 
transfers.  Function drivers _do_ queue status-stage requests for 
those.)

Alan Stern

Paul Elder Dec. 14, 2018, 3:47 a.m. UTC | #18

[snip]

> > >> > Another thing we should do is give function drivers a way to send a
> > >> > STALL response for the status stage.  Currently there's no way to do
> > >> > it, if a data stage is present.
> > >> 
> > >> Status stage can only be stalled if host tries to move data on the wrong
> > >> direction.
> > >
> > > The USB-2 spec disagrees.  See Table 8-7 in section 8.5.3.1 and the
> > > following paragraphs.  (Although, I can't see why a function would ever
> > > fail to complete the command sequence for a control-IN transfer after
> > > the data had already been sent.)
> > 
> > I can't see how we could ever STALL after returning the data requested
> > by the host. Seems like that wasn't well thought out.
> 
> Yes, it doesn't make a lot of sense.  However, STALLing the status
> stage of a control-OUT transfer does make sense, so we should be able
> to do it.  The obvious approach is to call ep0's set_halt() method
> instead of submitting an explicit status request.

Exactly, that's what we want to be able to do.

> > > Checking the length isn't enough.  A data stage can have 0 length.
> > 
> > apologies, I meant wLength, like so:
> > 
> > 	len = le16_to_cpu(ctrl->wLength);
> > 	if (!len) {
> > 		dwc->three_stage_setup = false;
> > 		dwc->ep0_expect_in = false;
> > 		dwc->ep0_next_event = DWC3_EP0_NRDY_STATUS;
> > 	} else {
> > 		dwc->three_stage_setup = true;
> > 		dwc->ep0_expect_in = !!(ctrl->bRequestType & USB_DIR_IN);
> > 		dwc->ep0_next_event = DWC3_EP0_NRDY_DATA;
> > 	}
> 
> Presumably you invert the value of ep0_expect_in and set ep0_next_event
> to DWC3_EP0_NRDY_STATUS when the next (data-stage) request is submitted
> for ep0.  Okay.
> 
> > special return values would be rendered uncessary if there's agreement
> > that status stage is always explicit. Why would need
> > USB_GADGET_DELAYED_STATUS if every case returns that?
> 
> Agreed.
> 
> > >> > There's also the fact that requests can specify a completion handler, but only 
> > >> > the data stage request would see its completion handler called (unless we 
> > >> > require UDCs to call completion requests at the completion of the status 
> > >> > stage, but I'm not sure that all UDCs can report the event to the driver, and 
> > >> > that would likely be useless as nobody needs that feature).
> > >> 
> > >> you still wanna know if the host actually processed your status
> > >> stage. udc-core can (and should) provide a generic status stage
> > >> completion function which, at a minimum, aids with some tracepoints.
> > >
> > > Helping with tracepoints is fine.  However, I don't think function 
> > > drivers really need to know whether the status stage was processed by 
> > > the host.  Can you point out any examples where such information would 
> > > be useful?
> > 
> > If you know your STATUS stage completed, you have a guarantee that your
> > previous control transfer is complete. It's a very clear signal that you
> > should prepare for more control transfers.
> 
> That doesn't seem to make sense, because in reality you don't have
> this guarantee.  Consider the following scenario: The host starts a
> control-IN transfer.  You send the data-stage request succesfully and
> then submit the status-stage request.  That request will complete
> before the host receives the ACK for its 0-length status OUT
> transaction.  In fact, the host may never receive that ACK and so the
> transfer may never complete.
> 
> Besides, you don't need a signal (clear or otherwise) to prepare for
> more control transfers.  You should start preparing as soon as the
> status-stage request has been submitted.  At that point, what else is
> there for you to do?
> 
> For that matter, you should be prepared to receive a new setup callback 
> at any time.  The host doesn't have to wait for an old control transfer 
> to complete before starting a new one.
> 
> I just don't see any value in knowing the completion code of a
> status-stage request.

I agree.

> > >> > To simplify function drivers, do you think the above proposal of adding a flag 
> > >> > to the (data stage) request to request an automatic transition to the status 
> > >> > stage is a good idea ? We could even possibly invert the logic and transition 
> > >> 
> > >> no, I don't think so. Making the status phase always explicit is far
> > >> better. UDCs won't have to check flags, or act on magic return
> > >> values. It just won't do anything until a request is queued.
> > >
> > > I don't agree.  This would be a simple test in a localized area (the 
> > > completion callback for control requests).  It could even be 
> > > implemented by a library routine; the UDC driver would simply have to 
> > > call this routine immediately after invoking the callback.
> > 
> > I don't follow what you mean here.
> 
> Suppose we have a core library routine like this:
> 
> void usb_gadget_control_complete(struct usb_gadget *gadget,
> 		unsigned int no_implicit_status, int status)
> {
> 	struct usb_request *req;
> 
> 	if (no_implicit_status || status != 0)
> 		return;
> 
> 	/* Send an implicit status-stage request for ep0 */
> 	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> 	if (req) {
> 		req->length = 0;
> 		req->no_implicit_status = 1;
> 		req->complete = /* req's deallocation routine */
> 		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> 	}
> }
> 
> Then all a UDC driver would need to do is call 
> usb_gadget_control_complete() after invoking a control request's 
> completion handler.  The no_implicit_status and status arguments would 
> be taken from the request that was just completed.
> 
> With this one call added to each UDC, all the existing function drivers
> would work correctly.  Even though they don't explicitly queue
> status-stage requests, the new routine will do so for them,
> transparently.  Function drivers that want to handle their own
> status-stage requests explicitly will merely have to set the
> req->no_implicit_status bit.

I think this is a good idea. We still get the benefits of explicit
status stage without being overly intrusive in the conversion, and we
maintain the queue-based API.

Would it be fine for me to proceed in this direction for a v2?

> (We might or might not need to watch out for 0-length control-OUT 
> transfers.  Function drivers _do_ queue status-stage requests for 
> those.)

Thanks,

Paul Elder

Alan Stern Dec. 14, 2018, 3:35 p.m. UTC | #19

On Thu, 13 Dec 2018, Paul Elder wrote:

> > Suppose we have a core library routine like this:
> > 
> > void usb_gadget_control_complete(struct usb_gadget *gadget,
> > 		unsigned int no_implicit_status, int status)
> > {
> > 	struct usb_request *req;
> > 
> > 	if (no_implicit_status || status != 0)
> > 		return;
> > 
> > 	/* Send an implicit status-stage request for ep0 */
> > 	req = usb_ep_alloc_request(gadget->ep0, GFP_ATOMIC);
> > 	if (req) {
> > 		req->length = 0;
> > 		req->no_implicit_status = 1;
> > 		req->complete = /* req's deallocation routine */
> > 		usb_ep_queue(gadget->ep0, req, GFP_ATOMIC);
> > 	}
> > }
> > 
> > Then all a UDC driver would need to do is call 
> > usb_gadget_control_complete() after invoking a control request's 
> > completion handler.  The no_implicit_status and status arguments would 
> > be taken from the request that was just completed.
> > 
> > With this one call added to each UDC, all the existing function drivers
> > would work correctly.  Even though they don't explicitly queue
> > status-stage requests, the new routine will do so for them,
> > transparently.  Function drivers that want to handle their own
> > status-stage requests explicitly will merely have to set the
> > req->no_implicit_status bit.
> 
> I think this is a good idea. We still get the benefits of explicit
> status stage without being overly intrusive in the conversion, and we
> maintain the queue-based API.
> 
> Would it be fine for me to proceed in this direction for a v2?

It is as far as I'm concerned (Felipe might not agree).  Knock yourself
out.  :-)

Alan Stern

> > (We might or might not need to watch out for 0-length control-OUT 
> > transfers.  Function drivers _do_ queue status-stage requests for 
> > those.)
> 
> Thanks,
> 
> Paul Elder

Patch

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index af88b48c1cea..1ec5ce6b43cd 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -443,6 +443,41 @@  void usb_ep_fifo_flush(struct usb_ep *ep)
 }
 EXPORT_SYMBOL_GPL(usb_ep_fifo_flush);
 
+/**
+ * usb_ep_ep_delay_status - set delay_status flag
+ * @ep: the endpoint whose delay_status flag is being set
+ *
+ * This function instructs the UDC to delay the status stage of a control
+ * request. It can only be called from the request completion handler of a
+ * control request.
+ */
+void usb_ep_delay_status(struct usb_ep *ep)
+{
+	ep->delayed_status = true;
+}
+EXPORT_SYMBOL_GPL(usb_ep_delay_status);
+
+/**
+ * usb_ep_send_response - reply to control OUT request
+ * @ep: the endpoint to send reply
+ * @stall: true for STALL, false for ACK
+ *
+ * Instruct the UDC to complete the status stage of a control request that was
+ * previously delayed with a call to usb_ep_delay_status().
+ */
+int usb_ep_send_response(struct usb_ep *ep, bool stall)
+{
+	if (!ep->ops->send_response)
+		return -ENOSYS;
+
+	if (!ep->delayed_status)
+		return -EINVAL;
+
+	ep->delayed_status = false;
+	return ep->ops->send_response(ep, stall);
+}
+EXPORT_SYMBOL_GPL(usb_ep_send_response);
+
 /* ------------------------------------------------------------------------- */
 
 /**
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index e5cd84a0f84a..d39c221d4b68 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -144,6 +144,8 @@  struct usb_ep_ops {
 
 	int (*fifo_status) (struct usb_ep *ep);
 	void (*fifo_flush) (struct usb_ep *ep);
+
+	int (*send_response) (struct usb_ep *ep, bool stall);
 };
 
 /**
@@ -209,6 +211,8 @@  struct usb_ep_caps {
  *	enabled and remains valid until the endpoint is disabled.
  * @comp_desc: In case of SuperSpeed support, this is the endpoint companion
  *	descriptor that is used to configure the endpoint
+ * @delayed_status: True if status stage is being delayed. Valid only for
+ *	control endpoints.
  *
  * the bus controller driver lists all the general purpose endpoints in
  * gadget->ep_list.  the control endpoint (gadget->ep0) is not in that list,
@@ -232,6 +236,7 @@  struct usb_ep {
 	u8			address;
 	const struct usb_endpoint_descriptor	*desc;
 	const struct usb_ss_ep_comp_descriptor	*comp_desc;
+	bool			delayed_status;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -249,6 +254,8 @@  int usb_ep_clear_halt(struct usb_ep *ep);
 int usb_ep_set_wedge(struct usb_ep *ep);
 int usb_ep_fifo_status(struct usb_ep *ep);
 void usb_ep_fifo_flush(struct usb_ep *ep);
+void usb_ep_delay_status(struct usb_ep *ep);
+int usb_ep_send_response(struct usb_ep *ep, bool stall);
 #else
 static inline void usb_ep_set_maxpacket_limit(struct usb_ep *ep,
 		unsigned maxpacket_limit)
@@ -278,6 +285,10 @@  static inline int usb_ep_fifo_status(struct usb_ep *ep)
 { return 0; }
 static inline void usb_ep_fifo_flush(struct usb_ep *ep)
 { }
+static inline void usb_ep_delay_status(struct usb_ep *ep)
+{ }
+static inline int usb_ep_send_response(struct usb_ep *ep, bool stall)
+{ }
 #endif /* USB_GADGET */
 
 /*-------------------------------------------------------------------------*/