On Tue, Jun 05, 2012 at 01:09:17PM +0200, Hans de Goede wrote: > Which has resulted in me doing a good 2 hours of reading the XHCI spec, and > xhci_requires_manual_halt_cleanup is 100% correct here, not only does a > short packet currently not stop the ep ring, there is no way to make a short > packet stop the ep ring. A (normal) trb does have the Interrupt-on Short Packet > (ISP) flag, but that only causes the controller to generate a Transfer Event, > not to stop the ep ring. I'm sorry you had to take so much time to read the xHCI spec for something I should have caught in the first place. :( > So it looks like, if we want to preserve the usb-core / drivers expected behavior > of a short packet in an urb with the URB_SHORT_NOT_OK flag set causing the ep > to halt until the completion handler for that ep has run, that the xHCI code > would need to not submit later urbs for the same ep to the controller until the > urb with the URB_SHORT_NOT_OK flag set has completed. Or the driver needs to hold onto URBs internally until each URB with the URB_SHORT_NOT_OK flag set completes. > An alternative which I've quickly explored is auditing all users of the > URB_SHORT_NOT_OK flag and see if they do any pipe-lining, iow submit more then > one urb at a time. And if they do fix them to somehow not do that (and then > document that URB_SHORT_NOT_OK does not guarantee to stop the ep). > > A quick grep shows that that seems to be quite doable, only the following .c > files contain URB_SHORT_NOT_OK: > ./drivers/usb/host/imx21-hcd.c > ./drivers/usb/host/ohci-q.c > ./drivers/usb/host/whci/qset.c > ./drivers/usb/host/isp1362-hcd.c > ./drivers/usb/host/xhci-ring.c > ./drivers/usb/host/isp116x-hcd.c > ./drivers/usb/host/oxu210hp-hcd.c > ./drivers/usb/host/fhci-q.c > ./drivers/usb/host/u132-hcd.c > ./drivers/usb/host/ehci-q.c > ./drivers/usb/host/isp1760-hcd.c > ./drivers/usb/host/uhci-q.c So that's all host controllers dealing with the URB_SHORT_NOT_OK flag. > ./drivers/usb/misc/usbtest.c That's the host controller testing program. > ./drivers/usb/musb/cppi_dma.c > ./drivers/usb/musb/musb_host.c That's the musb host controller dealing with URB_SHORT_NOT_OK. > ./drivers/usb/core/urb.c That's just the USB core making sure that URB_SHORT_NOT_OK isn't set for an OUT endpoint. > ./drivers/usb/core/devio.c > ./drivers/usb/core/message.c That's the USB core setting URB_SHORT_NOT_OK for each sg list URB. But we'll never hit that path under xHCI because the xHCI driver supports native scatter gather, so the USB core doesn't need to break each sglist entry into separate URBs. Drivers can still *use* usb_sg_init(), but it's not required, and as you said, they really need to be changed to just add the sglist to the URB and call usb_submit_urb() instead of calling usb_sg_init(). > ./drivers/usb/core/hcd.c > ./drivers/staging/rts5139/rts51x_transport.c > ./drivers/staging/usbip/stub_rx.c > > But an important user of the flag is usb_sg_init and friends, which gets used in: > ./drivers/usb/storage/transport.c > ./drivers/usb/misc/usbtest.c > ./drivers/usb/core/message.c > ./drivers/staging/rts5139/rts51x_transport.c > ./drivers/staging/usbip/vhci_hcd.c > ./drivers/staging/keucr/transport.c Should these staging drivers really be using usb_sg_init()? Also, are they really submitting sglists with multiple vectors, or just one vector? > ./drivers/mmc/host/vub300.c Why is a *host* driver using usb_sg_init()? > Which also means that the issues of XHCI ep rings not > stopping on a short transfer is bigger then we thought, as > under the right circumstances it can impact the linux > usb-storage driver too .. :| But short transfers should happen all the time with the usb-storage driver, so I don't understand why it wouldn't be effected by this bug. Oh, I think I know why. Short packets can only happen on IN transfers. OUT transfers won't ever see a short packet because the host is sending the transfer. An IN data transfer means the usb-storage is issuing a Read command (or some other SCSI command that requires the device to send data). I don't think Read SCSI commands can ever be short. Possibly other SCSI IN commands can be short, but I don't think it would use usb_sg_init() for those commands. Perhaps Alan can provide some insight into what SCSI commands the usb-storage driver would use usb_sg_init() for? > And this means that simply serializing urbs with the > URB_SHORT_NOT_OK flag set is not really an option either as this > will seriously hurt performance. It will hurt SCSI Read performance, yes. > All in all it looks like > 1) We need a way to submit sg lists (or some such) directly to > the hcd driver, so the the xHCI driver can put all the parts into 1 td. That's already in place. You just populate urb->sg with the sglist you want to send, and call usb_submit_urb(). Just bypass the call to usb_sg_init(). In fact, that's what the usb-storage should do in usb_stor_bulk_transfer_sglist(), if it detects the host controller natively supports sglists (when xhci.c: hcd->self.sg_tablesize != 0). There's no reason it should use usb_sg_init() under xHCI or EHCI, which both have native scatter gather support. > 2) Change usbdevfs to use an sg like interface for the BULK_CONTINUATION > flag, which also means changing the userspace API since userspace now > also needs a way to signal that the first urb is part of a multi urb > transfer Or just change libusb to not break up large transfers, and get rid of the BULK_CONTINUATION flag all together. libusb should submit the whole transfer to usbfs with an iovec. usbfs can then: a) verify each iovec entry is not too long b) verify the total length of the iovec isn't too big c) translate each iovec entry into one sglist vector d) submit one URB with a populated sglist to the USB core > 3) audit all the other users of URB_SHORT_NOT_OK > > So a lot of work I'm afraid, but I see no other way to really fix this :( I see a lot of work in usbfs and libusb, but I believe that the USB core and xHCI driver infrastructure is already there. So the job is half done, although arguably the userspace half is the harder half, since it requires getting libusb developers on board. Sarah Sharp -- 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
