Re: WARNING in usb_composite_setup_continue

Alan Stern
2020-11-12T15:59:08Z

On Thu, Nov 12, 2020 at 09:01:11AM +0000, Peter Chen wrote:
> On 20-11-11 14:47:10, Alan Stern wrote:
> > There's still a possible race, although it's a different one: The
> > gadget's delayed status reply can race with the host timing out and
> > sending a new SETUP packet:
> >
> > Host sends SETUP packet A
> >
> > Function receives A and decides
> > to send a delayed status reply
> >
> > Function thread starts to
> > process packet A
> >
> > Host times out waiting for A status
> >
> > Host sends new SETUP packet B
> >
> > Composite core receives packet B
> > and resets cdev->delayed_status
>
> resets cdev->delayed_status? Where to do that?

Sorry, for some reason I though I read a line that set delayed_status to
0 in composite_setup(). I must have been fantasizing...

> Even the host re-try the
> control transfer, it should send the same control request, eg,
> SET_CONFIGURATION, and increase cdev->delayed_status. There is a description
> for possible host sends next control request before receiving status
> packet at USB 2.0 Spec, CH 5.5.5 Control Transfer Data Sequences
>
> If a Setup transaction is received by an endpoint before a previously
> initiated control transfer is completed,
> the device must abort the current transfer/operation and
> handle the new control Setup transaction. A Setup
> transaction should not normally be sent before the completion
> of a previous control transfer. However, if a
> transfer is aborted, for example, due to errors on the bus,
> the host can send the next Setup transaction
> prematurely from the endpoint’s perspective.

Yes. The composite core does not abort the current transfer/operation
when a new Setup transaction occurs. But it should -- and it should set
delayed_status back to 0. (Maybe this is what I was fantasizing.)

> > Function thread finishes and calls
> > usb_composite_setup_continue()
> >
> > The composite core sends a status
> > reply for packet A, not packet B
> >
> > Host receives status for A but thinks
> > it is the status for B!
> >
> > Function thread processes packet B
> >
> > Function thread finishes and calls
> > usb_composite_setup_continue()
> >
> > The composite core sees
> > cdev->delayed_status == 0 and WARNs.
> >
> > At the moment I don't see how to prevent this sort of race from
> > happening. We may need to change the API, giving the composite core a
> > way to match up calls to usb_composite_setup_continue() with the
> > corresponding call to composite_setup(). But even that wouldn't fix
> > the entire problem.
> >
>
> Hi Alan,
>
> I more think a possible reset or disconnect occurrence between them, and
> composite_disconnect is called.

That sounds reasonable.

This is why I think we will need to change the API. There has to be a
way to tell usb_composite_setup_continue() which SETUP it is being
called for. A new SETUP or a disconnect should invalidate the old
SETUP, and then usb_composite_setup_continue() would ignore any calls
that were for the old SETUP packet.

Alan Stern