| Message ID | 20140507215344.GH19925@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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); > -- -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)