On Wed, Jun 06, 2012 at 10:10:04AM +0200, Hans de Goede wrote:
> 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:
> >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.
There are two ways to deal with sglists:
1. Set urb->sg and everything else in the URB, and call
usb_submit_urb(). This is what the UAS driver does, and the URB call
back will be asychronous.
2. Call usb_sg_init() with a sglist. This will set up urb->sg. Then
call usb_sg_wait() to wait on all the URBs to be completed. This is
the path the usb-storage driver uses, and this path is synchronous.
The only reason the usb-storage driver uses usb_sg_wait() is because it
is designed to submit each part of the SCSI command and wait on each of
the two to three URBs that make up that command. The UAS driver is
designed to submit all the URBs at once that correspond to the two to
four SCSI command portions, and deal with each individual URB
completion.
I'm not familiar with how usbfs works today, but it seems like you would
want an asynchronous URB completion, so you would want to simply fill in
urb->sg and call usb_submit_urb(). We may want to refactor some of the
sg init code from the uas driver (drivers/usb/storage/uas.c) into the
USB core, so that usbfs can share it.
> >>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 :)
Great!
> > 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
Today, the only reason you would want to use bulk streams is to talk to
a UAS device. That involves larger transfers, so they would need to use
the iovec capable async API.
> I personally think it is fine to only support them with the async API.
Yes, that's fine with me. I think we only really want new userspace
drivers that know what they're doing to be submitting URBs with bulk
streams.
> 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.
Yes, it's a bit ugly, but as long as the types are the same length, I
don't see it as too much of a problem.
> 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 */
In the iovec case, buffer_length should be zero (transfer the whole
iovec) or non-zero (transfer part of the iovec), correct?
> And adding a flag userspace can use to indicate if the buffer is an iovec rather then
> a regular buffer.
Where would the flag be added? Or would it just be another ioctl
number?
> 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.
Sure, makes sense.
> 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!
Yes! Or maybe usbfs should simply return with an error any URBs that
have the BULK_CONTINUATION flag set when it's submitted to a host
controller with a non-zero sg_table length.
You will also need a new ioctl to allow userspace to allocate a specific
number of bulk streams for a set of endpoints under an interface. usbfs
should check that the driver has claimed the interface before calling
into usb_alloc_streams(). Similarly, there needs to be an ioctl to
deallocate streams, which needs to call usb_free_streams().
Perhaps if a driver unclaims an interface, usbfs should automatically
attempt to deallocate streams. That probably means usbfs needs to keep
track of which endpoints have streams. It can't unconditionally
deallocate streams for all endpoints on an interface on the unclaim of
the interface, because the xHCI driver will fail if one of the endpoints
didn't have streams allocated, but there could still be a subset of
endpoints in the interface that do have streams allocated.
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
