Hi,
On 06/05/2012 05:56 PM, Sarah Sharp wrote:
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. :(
No problem, it was my own choice to dive deeper into the problem, rather then
bounce it back to you. I had the feeling that you're fix was close and I hoped
that I could make it work, but alas ...
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.
Right, that is the alternative discussed below :)
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().
Well I just checked and for drivers calling usb_sg_init() on a sg capable hc,
usb_sg_init() will only submit a single urb with urb->sg set, so usb_sg_init()
takes care of this for the drivers, which IMHO is the right place to do this,
rather then needing to have 2 paths for this in each driver dealing with sg
lists.
./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?
Since usb_sg_init does the right thing, I guess we don't need to worry
about this :)
./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.
It only possibly uses the SHORT_NOT_OK flag through usb_sg_init and that
does not use the flag for a sg capable hc.
<snip>
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.
Great I did not know about urb->sg, so indeed it looks like this bit
is already in place.
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.
An iovec is still breaking up a large transfer, although not into
multiple syscalls, but still into multiple memory blocks (which
is exactly what we need). With that said, I'm 100% in favor of this
solution :)
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.
I'm a libusb developer these days, and you can count me as being on board :)
However if we're going to make some serious changes to the usbfs API I would
really like to see us tackle usb 3 bulk streams at the same time! We don't
necessarily need to implement both the iovecs and the bulkstreams at the same
time, but I believe we should keep both in mind when making API changes.
Before writing up a proposal on this, let me start with some questions:
1) Do we want to support bulk streams in the synchronous API, iow with the
USBDEVFS_BULK ioctl, or just with the async API, iow with USBDEVFS_SUBMITURB
and friends? (as well as with the new iovec capable async API
I personally think it is fine to only support them with the async API.
2) We either need to change the size of struct usbdevfs_urb to make place for
a stream-id which means having 2 sets of ioctls, or we could change it from:
struct usbdevfs_urb {
unsigned char type;
unsigned char endpoint;
int status;
unsigned int flags;
void *buffer;
int buffer_length;
int actual_length;
int start_frame;
int number_of_packets;
int error_count;
unsigned int signr; /* signal to be sent on completion,
or 0 if none should be sent. */
void *usercontext;
struct usbdevfs_iso_packet_desc iso_frame_desc[0];
};
struct usbdevfs_urb {
unsigned char type;
unsigned char endpoint;
int status;
unsigned int flags;
void *buffer;
int buffer_length;
int actual_length;
int start_frame;
union {
int number_of_packets; /* Valid only for ISO transfers */
unsigned int stream_id; /* Valid only for BULK transfers */
};
int error_count;
unsigned int signr; /* signal to be sent on completion,
or 0 if none should be sent. */
void *usercontext;
struct usbdevfs_iso_packet_desc iso_frame_desc[0];
};
So overloading number_of_packets with the stream_id for bulk transfers,
note that currently for bulk transfers devio.c will always set
number_of_packets to 0 independent of what userspace provided there,
so some userspace apps may be sending garbage there. Which means that
if we go this way, we should only look at stream_id if bulk streams
have been allocated for the ep in question (which only new apps which
know to properly fill stream_id will do).
I personally like this solution, I know it is not very pretty, but the
alternative is growing the struct which means adding a whole new set
of ioctls, as we need to keep the old ones for compatibility, IMHO
overloading the number_of_packets field is the lesser evil.
3) Akin to 2). We also need some way to pass iovecs, rather then
a regular buffer for urbs. I suggest making yet another struct usbdevfs_urb
change for this, replacing:
void *buffer;
int buffer_length;
With:
union {
void *buffer;
struct iovec *buffer_iov;
};
int buffer_length; /* buffer length in bytes, or number of buffer_iov elements */
And adding a flag userspace can use to indicate if the buffer is an iovec rather then
a regular buffer.
4) Userspace needs to know if it can use bulk streams and / or iovecs, and it would be
good to have a better test then checking the kernel version, esp. as we expect some of
this to get backported to older kernels. Therefor I suggest adding a USBDEVFS_GET_CAP
ioctl which takes an u64 * as arg and stores a capabilities bitmask there. This bitmask
will reflect if the kernel knows about bulkstreams for example, not if the device the
ioctl is happening on happens to support bulkstreams.
Thats all for now. One last remark: We must not forget to document that the
URB_SHORT_NOT_OK flag does not always halt the ep on a short urb!
Regards,
Hans
--
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
