On Wed, 8 Feb 2012, Sebastian Andrzej Siewior wrote: > On 02/06/2012 09:14 PM, Alan Stern wrote: > >>>> You should not continue processing setup packets after you received a > >>>> DELAYED_STATUS return code. > >>> > >>> No, that's wrong. The gadget _must_ process SETUP packets, always. > >> > >> But if break the HW or don't ACK the setup packet or just disable the > > > > Well, obviously broken hardware can do anything, but that doesn't make > > it right. And the hardware _must_ ACK every valid SETUP packet (see > > 8.5.3 in the USB-2.0 spec); it is not allowed to reply with NAK. > > I'm not saying to reply with NAK. I just say to delay the ACK after we > done processing it. The hardware isn't allowed to do that either. It _must_ send an ACK response to a SETUP packet within the normal bus turnaround time (a few microseconds at most, depending on the bus speed). From the USB-2.0 spec, section 8.5.3: The function receiving a SETUP must accept the SETUP data and respond with ACK; if the data is corrupted, discard the data and return no handshake. Assuming the data is not corrupted (the normal case), the hardware has no choice but to respond with an ACK handshake. > That means: > - fetch the setup packet > - queue processing of the setup data into a workqueue > - until workqueue is done processing there is no ACK or NAK > - once the workqueue is done, the UDC answers with ACK/NAK > - next setup packet can be fetched. This is not valid. If the host sends a SETUP packet and receives no handshake response, it treats this as an error. It will retry up to a total of three times and then cancel the entire transfer. > >> IRQs you never receive a second one packet untill the first SETUP > >> packet is done. The gadget will process the packet but at a later point > >> in time. After all you could hold a spinlock with irqs for a long time > >> and notice the second setup packet before the gadget was done with the > >> first one. > > > > Doesn't matter. If the hardware is designed properly, it will flush > > the ep0 FIFO whenever a new SETUP packet arrives. The UDC driver must > > be written so that it doesn't queue a response to an old SETUP packet. > > So we receive new setup packet before acking the old one? See above. > >> What is wrong processing one setup packet after the other? > > > > Suppose the host sends a Set-Interface request, and you delay handling > > it. If the delay is too long, the host will time out and cancel the > > request. Then the host sends some completely different request, such > > as Set-Feature. If your delayed response goes back now, the host will > > think you accepted the Set-Feature request and not realize that you > > really carried out the Set-Interface. In addition, the response to the > > Set-Feature request will remain sitting in the ep0 FIFO and never get > > sent, because the host won't ask for it. > > Now I see where you are going with this. So this can happen right now > with threaded_irq, a busy high priority task (higher than the irq) and > we don't need one of the delayed_status user for this. You ask for a > descriptor which may get delayed long enough and by the time you answer > it you may have another setup packet received. Yes, the UDC driver needs to detect such things and avoid them. I don't think the composite gadget driver can do all the necessary work, although it can help. > There is the delayed_setup counter which could be used for all > transfers. Once ->setup() returns (winthin the worked) it decrements > the counter and once it reaches 0 you will send the request. Otherwise > discard it. This will make the race window smaller but it will remain. > You *may* receive another SETUP packet right after you looked at the > counter. A better way would be for USB to have an incrementing sequence > counter so if you answer too late it will see that this is an old > answer and ignore it. But it is too late for this. In fact f_mass_storage does have exactly this kind of incrementing sequence counter. It's called ep0_req_tag. But as you point out, the gadget driver can't solve all the races by itself. That's why the UDC driver needs to synchronize with the hardware. > Receiving another SETUP packet while handling the current one isn't > possible at least on dwc3: Lets say SET_INTERFACE will be delayed. The > controller probably acks it immediately. The only thing the driver does > is to setup the next transfer to receive another SETUP packet. Right > now we delay this step until gadget is done with his SET_INTERFACE. If > we don't, we have another setup packet waiting for us (that one after > SET_INTERFACE). I don't have an USB sniffer to say that this is what > really happens, but it looks like it from the driver perspective. That doesn't sound like quite the right approach. A USB device is always supposed to accept SETUP packets. What the driver needs to do is keep track of them, and not give the response for an earlier SETUP to the hardware after a later SETUP arrives. Cases where the two things happen simultaneously must be handled by the hardware. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
