[PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

A delayed status request may be queued before composite framework returns
USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run
on a different core in parallel with the control request irq.

SETUP XferComplete IRQ        		fsg_main_thread
----------------------        		---------------
	|					|
spin_lock_irqsave(&dwc->lock)               sleeping
	|					|
	...					...
dwc3_ep0_inspect_setup()			|
	|					|
dwc3_ep0_delegate_req() 			|
	|					|
	...					|
spin_unlock(&dwc->lock);			|
	|					|
fsg_set_alt()   ======> Signal Wakeup ====>	|
	| 					|
other gadgets->set_alt() 	       handle exception
	|					|
	|			usb_composite_setup_continue()
	|					|
	|			spin_lock_irqsave(&dwc->lock)
  	|			     __dwc3_gadget_ep0_queue()
	|				 delay_status is false
	|			spin_unlock_irqrestore(&dwc->lock)
	|					|
	|				     sleeping
spin_lock(&dwc->lock); 				|
	|					|
delayed_status=true				|
	|					|

		STATUS XferNotReady IRQ
		------------------------
			|
		dwc3_ep0_xfernotready()
			|
		   delayed_status is true, return;

The result is the status packet will never be transferred, and
delayed_status is not cleared.

Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
Reported-by: Zhou Liping <liping.zhou@intel.com>
---
 drivers/usb/dwc3/ep0.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 21a3520..07292c0 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -1020,7 +1020,11 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
 		if (dwc->delayed_status) {
 			WARN_ON_ONCE(event->endpoint_number != 1);
 			dev_vdbg(dwc->dev, "Mass Storage delayed status\n");
-			return;
+
+			if (list_empty(&dwc->eps[0]->request_list))
+				return;
+			else
+				dwc->delayed_status = false;
 		}
 
 		dwc3_ep0_do_control_status(dwc, event);
-- 
1.7.9.5

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Wed, 7 May 2014, Zhuang Jin Can wrote:

> A delayed status request may be queued before composite framework returns
> USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run
> on a different core in parallel with the control request irq.
> 
> SETUP XferComplete IRQ        		fsg_main_thread
> ----------------------        		---------------
> 	|					|
> spin_lock_irqsave(&dwc->lock)               sleeping
> 	|					|
> 	...					...
> dwc3_ep0_inspect_setup()			|
> 	|					|
> dwc3_ep0_delegate_req() 			|
> 	|					|
> 	...					|
> spin_unlock(&dwc->lock);			|
> 	|					|
> fsg_set_alt()   ======> Signal Wakeup ====>	|
> 	| 					|
> other gadgets->set_alt() 	       handle exception
> 	|					|
> 	|			usb_composite_setup_continue()
> 	|					|
> 	|			spin_lock_irqsave(&dwc->lock)
>   	|			     __dwc3_gadget_ep0_queue()
> 	|				 delay_status is false
> 	|			spin_unlock_irqrestore(&dwc->lock)
> 	|					|
> 	|				     sleeping
> spin_lock(&dwc->lock); 				|
> 	|					|
> delayed_status=true				|
> 	|					|
> 
> 		STATUS XferNotReady IRQ
> 		------------------------
> 			|
> 		dwc3_ep0_xfernotready()
> 			|
> 		   delayed_status is true, return;
> 
> The result is the status packet will never be transferred, and
> delayed_status is not cleared.
> 
> Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
> Reported-by: Zhou Liping <liping.zhou@intel.com>

A similar problem can occur in the opposite sense: The thread queuing
the delayed status request might be delayed for so long that another
SETUP packet arrives from the host first.  In that case, the delayed
status request is a response for a stale transfer, so it must not be
sent to the host.

Do dwc3 and composite.c handle this case correctly?

Back in the old g_file_storage driver, I addressed this issue by
keeping a counter of all the setup requests.  When it came time to send
a delayed status response, the response would be sent only if the
counter had not changed from when the original setup request was
received.

As far as I can see, composite.c doesn't do anything like that.

Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

On Wed, May 07, 2014 at 11:03:42AM -0400, Alan Stern wrote:
> On Wed, 7 May 2014, Zhuang Jin Can wrote:
> 
> > A delayed status request may be queued before composite framework returns
> > USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run
> > on a different core in parallel with the control request irq.
> > 
> > SETUP XferComplete IRQ        		fsg_main_thread
> > ----------------------        		---------------
> > 	|					|
> > spin_lock_irqsave(&dwc->lock)               sleeping
> > 	|					|
> > 	...					...
> > dwc3_ep0_inspect_setup()			|
> > 	|					|
> > dwc3_ep0_delegate_req() 			|
> > 	|					|
> > 	...					|
> > spin_unlock(&dwc->lock);			|
> > 	|					|
> > fsg_set_alt()   ======> Signal Wakeup ====>	|
> > 	| 					|
> > other gadgets->set_alt() 	       handle exception
> > 	|					|
> > 	|			usb_composite_setup_continue()
> > 	|					|
> > 	|			spin_lock_irqsave(&dwc->lock)
> >   	|			     __dwc3_gadget_ep0_queue()
> > 	|				 delay_status is false
> > 	|			spin_unlock_irqrestore(&dwc->lock)
> > 	|					|
> > 	|				     sleeping
> > spin_lock(&dwc->lock); 				|
> > 	|					|
> > delayed_status=true				|
> > 	|					|
> > 
> > 		STATUS XferNotReady IRQ
> > 		------------------------
> > 			|
> > 		dwc3_ep0_xfernotready()
> > 			|
> > 		   delayed_status is true, return;
> > 
> > The result is the status packet will never be transferred, and
> > delayed_status is not cleared.
> > 
> > Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
> > Reported-by: Zhou Liping <liping.zhou@intel.com>
> 
> A similar problem can occur in the opposite sense: The thread queuing
> the delayed status request might be delayed for so long that another
> SETUP packet arrives from the host first.  In that case, the delayed
> status request is a response for a stale transfer, so it must not be
> sent to the host.
> 
> Do dwc3 and composite.c handle this case correctly?
> 
So the situation you describe is that we get the STATUS XferNotReady
event, but gadget queues a status request when control transfer already
failed. dwc3 can't move to SETUP phase until the status request arrives,
so any SETUP transaction from host will fail. If status request
eventually arrives, it already missed the first control transfer, and
I don't know how the controller will behave. If we still can get a
STATUS XferComplete event without actually transfer anything on the
bus, then we can move back to SETUP PHASE which will remove the stale
delayed status request and start the new SETUP transaction. But I think
in this situation, the host should already lose it patience and start
to reset the bus.

Per my understanding, it's impossible for dwc3 to send a stale STATUS
request for a new SETUP transaction. 

> Back in the old g_file_storage driver, I addressed this issue by
> keeping a counter of all the setup requests.  When it came time to send
> a delayed status response, the response would be sent only if the
> counter had not changed from when the original setup request was
> received.
> 
> As far as I can see, composite.c doesn't do anything like that.
> 
> Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Thu, 8 May 2014, Zhuang Jin Can wrote:

> > A similar problem can occur in the opposite sense: The thread queuing
> > the delayed status request might be delayed for so long that another
> > SETUP packet arrives from the host first.  In that case, the delayed
> > status request is a response for a stale transfer, so it must not be
> > sent to the host.
> > 
> > Do dwc3 and composite.c handle this case correctly?
> > 
> So the situation you describe is that we get the STATUS XferNotReady
> event, but gadget queues a status request when control transfer already
> failed.

When the host already timed out the control transfer and started a new 
one.  Here's what I'm talking about:

	Host sends a Set-Configuration request.

	The UDC driver calls the gadget driver's setup function.

	The setup function returns DELAYED_STATUS.

	After a few seconds, the host gets tired of waiting and
	sends a Get-Descriptor request

	The gadget driver finally submits the delayed request response
	to the Set-Configuration request.  But it is now too late,
	because the host expects a response to the Get-Descriptor 
	request.

>  dwc3 can't move to SETUP phase until the status request arrives,
> so any SETUP transaction from host will fail. If status request
> eventually arrives, it already missed the first control transfer, and
> I don't know how the controller will behave. If we still can get a
> STATUS XferComplete event without actually transfer anything on the
> bus, then we can move back to SETUP PHASE which will remove the stale
> delayed status request and start the new SETUP transaction. But I think
> in this situation, the host should already lose it patience and start
> to reset the bus.

My point is that the UDC driver can't handle this.  Therefore the
gadget driver has to prevent this from happening.

That means composite.c has to avoid sending delayed status responses if 
a new SETUP packet has been received already.

> Per my understanding, it's impossible for dwc3 to send a stale STATUS
> request for a new SETUP transaction. 

dwc3 won't know that the status response is stale.  It will think the 
response was meant for the new transfer, not the old one.

Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

On Wed, May 07, 2014 at 12:59:06PM -0400, Alan Stern wrote:
> On Thu, 8 May 2014, Zhuang Jin Can wrote:
> 
> > > A similar problem can occur in the opposite sense: The thread queuing
> > > the delayed status request might be delayed for so long that another
> > > SETUP packet arrives from the host first.  In that case, the delayed
> > > status request is a response for a stale transfer, so it must not be
> > > sent to the host.
> > > 
> > > Do dwc3 and composite.c handle this case correctly?
> > > 
> > So the situation you describe is that we get the STATUS XferNotReady
> > event, but gadget queues a status request when control transfer already
> > failed.
> 
> When the host already timed out the control transfer and started a new 
> one.  Here's what I'm talking about:
> 
> 	Host sends a Set-Configuration request.
> 
> 	The UDC driver calls the gadget driver's setup function.
> 
> 	The setup function returns DELAYED_STATUS.
> 
> 	After a few seconds, the host gets tired of waiting and
> 	sends a Get-Descriptor request
My understanding is dwc3 will return NYET to host for this
Get-Descriptor request transaction, as dwc3 is still in STATUS phase,
there's no buffer to receive anything in ep0-out. And your below
comments is not applicapable to dwc3.
> 
> 	The gadget driver finally submits the delayed request response
> 	to the Set-Configuration request.  But it is now too late,
> 	because the host expects a response to the Get-Descriptor 
> 	request.
> 
> >  dwc3 can't move to SETUP phase until the status request arrives,
> > so any SETUP transaction from host will fail. If status request
> > eventually arrives, it already missed the first control transfer, and
> > I don't know how the controller will behave. If we still can get a
> > STATUS XferComplete event without actually transfer anything on the
> > bus, then we can move back to SETUP PHASE which will remove the stale
> > delayed status request and start the new SETUP transaction. But I think
> > in this situation, the host should already lose it patience and start
> > to reset the bus.
> 
> My point is that the UDC driver can't handle this.  Therefore the
> gadget driver has to prevent this from happening.
> 
> That means composite.c has to avoid sending delayed status responses if 
> a new SETUP packet has been received already.
> 
> > Per my understanding, it's impossible for dwc3 to send a stale STATUS
> > request for a new SETUP transaction. 
> 
> dwc3 won't know that the status response is stale.  It will think the 
> response was meant for the new transfer, not the old one.
> 
> Alan Stern
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Thu, 8 May 2014, Zhuang Jin Can wrote:

> > When the host already timed out the control transfer and started a new 
> > one.  Here's what I'm talking about:
> > 
> > 	Host sends a Set-Configuration request.
> > 
> > 	The UDC driver calls the gadget driver's setup function.
> > 
> > 	The setup function returns DELAYED_STATUS.
> > 
> > 	After a few seconds, the host gets tired of waiting and
> > 	sends a Get-Descriptor request
> My understanding is dwc3 will return NYET to host for this
> Get-Descriptor request transaction, as dwc3 is still in STATUS phase,
> there's no buffer to receive anything in ep0-out.

dwc3 _cannot_ return NYET to a SETUP packet.  The USB protocol does not 
allow it.  A device must always respond to SETUP with ACK.

On the other hand, dwc3 _can_ return NYET to the token packet that
follows the SETUP transaction.  That's what it should do.  But at this
point it should be in the DATA stage, not the STATUS stage.  Receiving
a SETUP packet should abort a STATUS stage.

>  And your below
> comments is not applicapable to dwc3.

True, they apply to composite.c rather than dwc3.  However, they
address an issue very similar to your patch, so I raised this topic in
your email thread.

Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

On Thu, May 08, 2014 at 10:25:46AM -0400, Alan Stern wrote:
> On Thu, 8 May 2014, Zhuang Jin Can wrote:
> 
> > > When the host already timed out the control transfer and started a new 
> > > one.  Here's what I'm talking about:
> > > 
> > > 	Host sends a Set-Configuration request.
> > > 
> > > 	The UDC driver calls the gadget driver's setup function.
> > > 
> > > 	The setup function returns DELAYED_STATUS.
> > > 
> > > 	After a few seconds, the host gets tired of waiting and
> > > 	sends a Get-Descriptor request
> > My understanding is dwc3 will return NYET to host for this
> > Get-Descriptor request transaction, as dwc3 is still in STATUS phase,
> > there's no buffer to receive anything in ep0-out.
> 
> dwc3 _cannot_ return NYET to a SETUP packet.  The USB protocol does not 
> allow it.  A device must always respond to SETUP with ACK.
It true that device can not return NYET to a SETUP packet.
A device must always respond to SETUP with ACK _if_ the SETUP packet is
correctly received. Because there's no buffer prepared in ep0 for dwc3
to receive the SETUP packet, I guess there will be no handshake
returned to host. I can confirm this by doing an experiment tomorrow:)

Jincan

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Thu, 8 May 2014, Zhuang Jin Can wrote:

> > dwc3 _cannot_ return NYET to a SETUP packet.  The USB protocol does not 
> > allow it.  A device must always respond to SETUP with ACK.
> It true that device can not return NYET to a SETUP packet.
> A device must always respond to SETUP with ACK _if_ the SETUP packet is
> correctly received. Because there's no buffer prepared in ep0 for dwc3
> to receive the SETUP packet, I guess there will be no handshake
> returned to host. I can confirm this by doing an experiment tomorrow:)

The dwc3 driver should always prepare a buffer for the next ep0 SETUP
packet as soon as it retrieves the information for the current SETUP 
packet from the buffer.

Otherwise, as you described it, if the gadget driver never sends its 
delayed status response then the UDC will never respond to any more 
control transfers.

Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

On Thu, May 08, 2014 at 11:22:36AM -0400, Alan Stern wrote:
> On Thu, 8 May 2014, Zhuang Jin Can wrote:
> 
> > > dwc3 _cannot_ return NYET to a SETUP packet.  The USB protocol does not 
> > > allow it.  A device must always respond to SETUP with ACK.
> > It true that device can not return NYET to a SETUP packet.
> > A device must always respond to SETUP with ACK _if_ the SETUP packet is
> > correctly received. Because there's no buffer prepared in ep0 for dwc3
> > to receive the SETUP packet, I guess there will be no handshake
> > returned to host. I can confirm this by doing an experiment tomorrow:)
> 
> The dwc3 driver should always prepare a buffer for the next ep0 SETUP
> packet as soon as it retrieves the information for the current SETUP 
> packet from the buffer.
> 
In current model dwc3 doesn't prepare a buffer for the next ep0 SETUP
packet, and normally host should not send another SETUP packet if the
current control transfer is not finished. So below model works well
if every control transfer succeeds one by one.

Here's the 2-stage control transfer model in dwc3.
*******************************************
*         SETUP PHASE:			  *
*Setup a Control-Setup TRB, start Transfer*<-------------
*******************************************		|
		|					|
	   XferComplete irq	 			|
		|					|
		V					|
	***********************				|
	*Interpret Setup bytes*				|
	***********************				|
		|					|
	      2 stage				   XferComplete
		|				Setup Pending=0 or 1					
		V					|
	*****************				|
	* Wait for Host *				|
	*****************				|
		|					|
	Status XferNotReady irq				|
		|					|
		V					|
*******************************************		|
*	STATUS PHASE:			  *		|
*Setup Control-Status2 TRB, Start Transfer*-------------|
*******************************************
(note: in *STATUS PHASE* it can't start transfer if
the delayed status request is not queued by gadget driver).

If the gadget driver can't queue the delayed status request in time,
dwc3 will stay at *STATUS PHASE*.
Then, current control transfer fails. Host starts to send a new SETUP
packet.
At this point, it really depends on how dwc3 controller will behave when
it receives the new SETUP packet. If it can move on to *SETUP PHASE*
with similar way to [Tree-stage back to back SETUP handling] (see
below), then the stale delayed status request could cause a problem.

[ Tree-stage back to back SETUP handling]
For a tree-stage control transfer, dwc3 can handle back to back
SETUP packets. Below is quoted from dwc3 ep0.c
/*
 * Unfortunately we have uncovered a limitation wrt the Data
 * Phase.
 *
 * Section 9.4 says we can wait for the XferNotReady(DATA) event
 * to
 * come before issueing Start Transfer command, but if we do, we
 * will
 * miss situations where the host starts another SETUP phase
 * instead of
 * the DATA phase.  Such cases happen at least on TD.7.6 of the
 * Link
 * Layer Compliance Suite.
 *
 * The problem surfaces due to the fact that in case of
 * back-to-back
 * SETUP packets there will be no XferNotReady(DATA) generated
 * and we
 * will be stuck waiting for XferNotReady(DATA) forever.
 *
 * By looking at tables 9-13 and 9-14 of the Databook, we can
 * see that
 * it tells us to start Data Phase right away. It also mentions
 * that if
 * we receive a SETUP phase instead of the DATA phase, core will
 * issue
 * XferComplete for the DATA phase, before actually initiating
 * it in
 * the wire, with the TRB's status set to "SETUP_PENDING". Such
 * status
 * can only be used to print some debugging logs, as the core
 * expects
 * us to go through to the STATUS phase and start a
 * CONTROL_STATUS TRB,
 * just so it completes right away, without transferring
 * anything and,
 * only then, we can go back to the SETUP phase.
 *
 * Because of this scenario, SNPS decided to change the
 * programming
 * model of control transfers and support on-demand transfers
 * only for
 * the STATUS phase. To fix the issue we have now, we will
 * always wait
 * for gadget driver to queue the DATA phase's struct
 * usb_request, then
 * start it right away.
 *
 * If we're actually in a 2-stage transfer, we will wait for
 * XferNotReady(STATUS).
 */

> Otherwise, as you described it, if the gadget driver never sends its 
> delayed status response then the UDC will never respond to any more 
> control transfers.
If the device already fails the SET_CONFIG, what's the benefit of device
being able to repond to succeeding control transfer? Stop responding
might not be bad idea, host should eventually reset the bus to
re-enumerate the device.


Jincan

RE: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Paul Zimmerman]

> From: linux-usb-owner@vger.kernel.org [mailto:linux-usb-owner@vger.kernel.org] On Behalf Of Alan Stern
> Sent: Wednesday, May 07, 2014 9:59 AM
> 
> On Thu, 8 May 2014, Zhuang Jin Can wrote:
> 
> > > A similar problem can occur in the opposite sense: The thread queuing
> > > the delayed status request might be delayed for so long that another
> > > SETUP packet arrives from the host first.  In that case, the delayed
> > > status request is a response for a stale transfer, so it must not be
> > > sent to the host.
> > >
> > > Do dwc3 and composite.c handle this case correctly?
> > >
> > So the situation you describe is that we get the STATUS XferNotReady
> > event, but gadget queues a status request when control transfer already
> > failed.
> 
> When the host already timed out the control transfer and started a new
> one.  Here's what I'm talking about:
> 
> 	Host sends a Set-Configuration request.
> 
> 	The UDC driver calls the gadget driver's setup function.
> 
> 	The setup function returns DELAYED_STATUS.
> 
> 	After a few seconds, the host gets tired of waiting and
> 	sends a Get-Descriptor request
> 
> 	The gadget driver finally submits the delayed request response
> 	to the Set-Configuration request.  But it is now too late,
> 	because the host expects a response to the Get-Descriptor
> 	request.
> 
> >  dwc3 can't move to SETUP phase until the status request arrives,
> > so any SETUP transaction from host will fail. If status request
> > eventually arrives, it already missed the first control transfer, and
> > I don't know how the controller will behave. If we still can get a
> > STATUS XferComplete event without actually transfer anything on the
> > bus, then we can move back to SETUP PHASE which will remove the stale
> > delayed status request and start the new SETUP transaction. But I think
> > in this situation, the host should already lose it patience and start
> > to reset the bus.
> 
> My point is that the UDC driver can't handle this.  Therefore the
> gadget driver has to prevent this from happening.
> 
> That means composite.c has to avoid sending delayed status responses if
> a new SETUP packet has been received already.
> 
> > Per my understanding, it's impossible for dwc3 to send a stale STATUS
> > request for a new SETUP transaction.
> 
> dwc3 won't know that the status response is stale.  It will think the
> response was meant for the new transfer, not the old one.

The DWC3 controller will actually handle this case on its own. If
it sees another SETUP packet come in before the previous Control
transfer has completed, it will not send any DATA or STATUS phase
packets for the previous Control transfer to the host. But it will
"fake" the correct responses to the software, so the dwc3 driver will
think that the DATA/STATUS stages completed successfully, even though
nothing actually went out on the bus.

For other controllers that can't do this, maybe it should be handled
in the UDC driver rather than in the composite gadget?

-- 
Paul

RE: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Thu, 8 May 2014, Paul Zimmerman wrote:

> > When the host already timed out the control transfer and started a new
> > one.  Here's what I'm talking about:
> > 
> > 	Host sends a Set-Configuration request.
> > 
> > 	The UDC driver calls the gadget driver's setup function.
> > 
> > 	The setup function returns DELAYED_STATUS.
> > 
> > 	After a few seconds, the host gets tired of waiting and
> > 	sends a Get-Descriptor request
> > 
> > 	The gadget driver finally submits the delayed request response
> > 	to the Set-Configuration request.  But it is now too late,
> > 	because the host expects a response to the Get-Descriptor
> > 	request.
> > 
> > >  dwc3 can't move to SETUP phase until the status request arrives,
> > > so any SETUP transaction from host will fail. If status request
> > > eventually arrives, it already missed the first control transfer, and
> > > I don't know how the controller will behave. If we still can get a
> > > STATUS XferComplete event without actually transfer anything on the
> > > bus, then we can move back to SETUP PHASE which will remove the stale
> > > delayed status request and start the new SETUP transaction. But I think
> > > in this situation, the host should already lose it patience and start
> > > to reset the bus.
> > 
> > My point is that the UDC driver can't handle this.  Therefore the
> > gadget driver has to prevent this from happening.
> > 
> > That means composite.c has to avoid sending delayed status responses if
> > a new SETUP packet has been received already.
> > 
> > > Per my understanding, it's impossible for dwc3 to send a stale STATUS
> > > request for a new SETUP transaction.
> > 
> > dwc3 won't know that the status response is stale.  It will think the
> > response was meant for the new transfer, not the old one.
> 
> The DWC3 controller will actually handle this case on its own. If
> it sees another SETUP packet come in before the previous Control
> transfer has completed, it will not send any DATA or STATUS phase
> packets for the previous Control transfer to the host. But it will
> "fake" the correct responses to the software, so the dwc3 driver will
> think that the DATA/STATUS stages completed successfully, even though
> nothing actually went out on the bus.

That doesn't handle the problem I described above.  When the dwc3 
driver gets the late delayed status response, it will think it is a 
response to the new SETUP packet, and so it will carry out a bogus 
transfer.  It won't know that the status request was meant to be a 
response to a defunct control transfer.

> For other controllers that can't do this, maybe it should be handled
> in the UDC driver rather than in the composite gadget?

The only place this can be handled properly is in the gadget driver:
composite.c for those gadgets using it, otherwise in the higher level 
driver (if there are any remaining gadgets that don't use the composite 
framework).

Alan Stern

RE: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Paul Zimmerman]

> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, May 08, 2014 2:18 PM
> 
> On Thu, 8 May 2014, Paul Zimmerman wrote:
> 
> > > When the host already timed out the control transfer and started a new
> > > one.  Here's what I'm talking about:
> > >
> > > 	Host sends a Set-Configuration request.
> > >
> > > 	The UDC driver calls the gadget driver's setup function.
> > >
> > > 	The setup function returns DELAYED_STATUS.
> > >
> > > 	After a few seconds, the host gets tired of waiting and
> > > 	sends a Get-Descriptor request
> > >
> > > 	The gadget driver finally submits the delayed request response
> > > 	to the Set-Configuration request.  But it is now too late,
> > > 	because the host expects a response to the Get-Descriptor
> > > 	request.
> > >
> > > >  dwc3 can't move to SETUP phase until the status request arrives,
> > > > so any SETUP transaction from host will fail. If status request
> > > > eventually arrives, it already missed the first control transfer, and
> > > > I don't know how the controller will behave. If we still can get a
> > > > STATUS XferComplete event without actually transfer anything on the
> > > > bus, then we can move back to SETUP PHASE which will remove the stale
> > > > delayed status request and start the new SETUP transaction. But I think
> > > > in this situation, the host should already lose it patience and start
> > > > to reset the bus.
> > >
> > > My point is that the UDC driver can't handle this.  Therefore the
> > > gadget driver has to prevent this from happening.
> > >
> > > That means composite.c has to avoid sending delayed status responses if
> > > a new SETUP packet has been received already.
> > >
> > > > Per my understanding, it's impossible for dwc3 to send a stale STATUS
> > > > request for a new SETUP transaction.
> > >
> > > dwc3 won't know that the status response is stale.  It will think the
> > > response was meant for the new transfer, not the old one.
> >
> > The DWC3 controller will actually handle this case on its own. If
> > it sees another SETUP packet come in before the previous Control
> > transfer has completed, it will not send any DATA or STATUS phase
> > packets for the previous Control transfer to the host. But it will
> > "fake" the correct responses to the software, so the dwc3 driver will
> > think that the DATA/STATUS stages completed successfully, even though
> > nothing actually went out on the bus.
> 
> That doesn't handle the problem I described above.  When the dwc3
> driver gets the late delayed status response, it will think it is a
> response to the new SETUP packet, and so it will carry out a bogus
> transfer.  It won't know that the status request was meant to be a
> response to a defunct control transfer.

I think you misunderstood me. What I meant was, once the DWC3
controller sees the next SETUP packet, it will still accept the
commands from the dwc3 driver to start the DATA and STATUS phases
for the previous Control transfer, and will send back (fake) completion
events for those commands to the driver. But it won't actually send
anything on the wire.

So it should be impossible for the dwc3 driver to carry out a bogus
transfer. This is a feature of the DWC3 controller intended to
simplify what the software needs to handle, and to automatically
take care of the corner cases involved here.

> > For other controllers that can't do this, maybe it should be handled
> > in the UDC driver rather than in the composite gadget?
> 
> The only place this can be handled properly is in the gadget driver:
> composite.c for those gadgets using it, otherwise in the higher level
> driver (if there are any remaining gadgets that don't use the composite
> framework).

Why can't the UDC drivers handle this? AFAIK they all keep track of
which phase of a Control transfer they are in. If they see another
SETUP packet arrive, they could "fake" the DATA/STATUS stages of the
previous transfer, before passing on the next SETUP packet to the
gadget driver. Similar to what the DWC3 controller does in hardware.

Although, I guess it would be simpler to do it once in composite.c,
instead of in each individual UDC driver. But there would have to be
a quirk or something, to disable the code if the dwc3 driver is in
use. And that wouldn't fix the problem for gadgets that don't use
composite.c.

-- 
Paul

RE: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Alan Stern]

On Thu, 8 May 2014, Paul Zimmerman wrote:

> > That doesn't handle the problem I described above.  When the dwc3
> > driver gets the late delayed status response, it will think it is a
> > response to the new SETUP packet, and so it will carry out a bogus
> > transfer.  It won't know that the status request was meant to be a
> > response to a defunct control transfer.
> 
> I think you misunderstood me. What I meant was, once the DWC3
> controller sees the next SETUP packet, it will still accept the
> commands from the dwc3 driver to start the DATA and STATUS phases
> for the previous Control transfer, and will send back (fake) completion
> events for those commands to the driver. But it won't actually send
> anything on the wire.

I see.  The hardware keeps track of which transfer is being acted on.

> So it should be impossible for the dwc3 driver to carry out a bogus
> transfer. This is a feature of the DWC3 controller intended to
> simplify what the software needs to handle, and to automatically
> take care of the corner cases involved here.
> 
> > > For other controllers that can't do this, maybe it should be handled
> > > in the UDC driver rather than in the composite gadget?
> > 
> > The only place this can be handled properly is in the gadget driver:
> > composite.c for those gadgets using it, otherwise in the higher level
> > driver (if there are any remaining gadgets that don't use the composite
> > framework).
> 
> Why can't the UDC drivers handle this? AFAIK they all keep track of
> which phase of a Control transfer they are in. If they see another
> SETUP packet arrive, they could "fake" the DATA/STATUS stages of the
> previous transfer, before passing on the next SETUP packet to the
> gadget driver. Similar to what the DWC3 controller does in hardware.

That would be possible, yes.

> Although, I guess it would be simpler to do it once in composite.c,
> instead of in each individual UDC driver. But there would have to be
> a quirk or something, to disable the code if the dwc3 driver is in
> use. And that wouldn't fix the problem for gadgets that don't use
> composite.c.

Would the dwc3 driver really need such a quirk?  From what you said
before, I got the impression that if a new SETUP packet arrived before
the old transfer was complete, dwc3 wouldn't inform the gadget driver 
about it.  It would wait until the gadget driver submitted its 
status response for the old transfer and _then_ tell it about the new 
SETUP.

As for gadgets not using the composite framework, I don't think there
are very many of them left.  gadgetfs certainly, but hardly any others.  
Felipe should know for sure.

Alan Stern

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

Hi Balbi,

Do you have any comment for this patch?

Thanks
Jincan

On Wed, May 07, 2014 at 05:53:44PM -0400, Zhuang Jin Can wrote:
> A delayed status request may be queued before composite framework returns
> USB_GADGET_DELAYED_STATUS, because the thread queueing the request can run
> on a different core in parallel with the control request irq.
> 
> SETUP XferComplete IRQ        		fsg_main_thread
> ----------------------        		---------------
> 	|					|
> spin_lock_irqsave(&dwc->lock)               sleeping
> 	|					|
> 	...					...
> dwc3_ep0_inspect_setup()			|
> 	|					|
> dwc3_ep0_delegate_req() 			|
> 	|					|
> 	...					|
> spin_unlock(&dwc->lock);			|
> 	|					|
> fsg_set_alt()   ======> Signal Wakeup ====>	|
> 	| 					|
> other gadgets->set_alt() 	       handle exception
> 	|					|
> 	|			usb_composite_setup_continue()
> 	|					|
> 	|			spin_lock_irqsave(&dwc->lock)
>   	|			     __dwc3_gadget_ep0_queue()
> 	|				 delay_status is false
> 	|			spin_unlock_irqrestore(&dwc->lock)
> 	|					|
> 	|				     sleeping
> spin_lock(&dwc->lock); 				|
> 	|					|
> delayed_status=true				|
> 	|					|
> 
> 		STATUS XferNotReady IRQ
> 		------------------------
> 			|
> 		dwc3_ep0_xfernotready()
> 			|
> 		   delayed_status is true, return;
> 
> The result is the status packet will never be transferred, and
> delayed_status is not cleared.
> 
> Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
> Reported-by: Zhou Liping <liping.zhou@intel.com>
> ---
>  drivers/usb/dwc3/ep0.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 21a3520..07292c0 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -1020,7 +1020,11 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
>  		if (dwc->delayed_status) {
>  			WARN_ON_ONCE(event->endpoint_number != 1);
>  			dev_vdbg(dwc->dev, "Mass Storage delayed status\n");
> -			return;
> +
> +			if (list_empty(&dwc->eps[0]->request_list))
> +				return;
> +			else
> +				dwc->delayed_status = false;
>  		}
>  
>  		dwc3_ep0_do_control_status(dwc, event);
> -- 

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

Hi,

On Tue, May 13, 2014 at 09:45:51PM -0400, Zhuang Jin Can wrote:
> Hi Balbi,
> 
> Do you have any comment for this patch?

do you have an easy test-case which I can use to validate on my end ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] usb: dwc3: ep0: fix delayed status is queued too early

[Zhuang Jin Can]

On Tue, May 13, 2014 at 10:05:34AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Tue, May 13, 2014 at 09:45:51PM -0400, Zhuang Jin Can wrote:
> > Hi Balbi,
> > 
> > Do you have any comment for this patch?
> 
> do you have an easy test-case which I can use to validate on my end ?
The issue was reproduced on a multi-core platform with f_mass_storage and
adb (f_fs gadget) enabled. The enumeration will fail when it's connected
to PC via USB2 cable.

You may not be able to use adb (I think), but do replacing it with some other
gadgets (e.g.f_rndis). And f_mass_storage gadget should be the first
interface in the composite device (this is important to larger the
chance to reproduce the issue, the delayed status request will be queued
while irq is still enabling other endpoints of other gadgets).

Best Regards
Jincan