Re: [PATCH/RFC 5/5] usb: Add support for streams alloc/dealloc to devio.c

[Hans Petter Selasky <hselasky@c2i.net>]

Hi,

> 
> On Wed, Aug 17, 2011 at 09:06:03AM +0200, Hans Petter Selasky wrote:
> > I'm looking into implementing USB 3.0 streams support for FreeBSD and
> > would like to have a solution in Linux which is not too far apart, also
> > regarding API's for userspace.
> > 
> > I would suggest overloading the "unsigned int pipe", instead of breaking
> > existing API's by adding a new stream ID value. Also for LibUSB.
> > 
> > ./linux/usb.h:  unsigned int pipe;              /* (in) pipe information
> > */
> 
> I think this gets very close to breaking current API for usbfs.  What
> happens if some application is putting garbage in the upper bits of the
> pipe? 

I think this will never happen, because all USB applications I know about are 
taught to use a set of macros to create the "pipe" value, which by default 
zero these bits.

> A libusb application that worked before will break because the
> USB core will reject the endpoint as not having streams enabled.  So I
> think it's better to have a new IOCTL for submitting URBs to endpoints
> with streams enabled, as Alan suggested:
> 
> http://marc.info/?l=linux-kernel&m=130832352124932&w=2
> 
> > As per definition there are 15 bits available for "pipe". I think it is
> > not important to support more than 255 streams in the first go, hence I
> > see no real applications that would benefit from that many streams yet.
> 
> I don't see this as a strong argument why we should arbitrarily limit a
> new API.  It's very hard to deprecate kernel to userspace API, so I
> think we should do it right the first time.  There are current
> applications (like an SSD behind a UAS device) that need as many
> concurrent streams in flight as possible, so I don't buy the argument
> that there aren't current applications that need that many streams.
> 
> I don't want to limit future applications of streams without a very good
> reason.  I would like to see the new API allow userspace to submit URBs
> for stream IDs up to the maximum limit specified by the USB 3.0 spec
> (65,533 streams).
> 
> Why do you think reusing the pipe variable would be a better solution?
> You're going to need a new IOCTL to enable streams anyway, so why not
> also have a new IOCTL for submitting an URB to an endpoint with streams
> enabled?

What I'm thinking is that you should try to integrate the streams 
functionality as much as possible, with existing API's, so that API's that do 
roughly the same are not duplicated. In FreeBSD 8/9 we have more free bits in 
the pipe variable, and I'll probably use that for the stream ID. It's like 
IPv4 and IPv6, that you extend fields when required and not duplicate them. 
I'm just worried that intel will come up with yet-another way to transfer data 
in a few years, and then we'll have to wreck API's again. So why not foresee 
that now and make 128-bits available for endpoint/stream/XXX addressing?

> 
> > #define usb_pipeendpoint(pipe)  (((pipe) >> 15) & 0xf)
> > 
> > Then I suggest a new function/IOCTL in libusb which can be used to switch
> > on/off streams on a given endpoint. This is something which would need to
> > be done before submitting any URB's on that endpoint. And would be
> > similar to the clear-stall case.
> 
> Amit was discussing such an IOCTL.  The problem is you just can't turn
> streams "on and off".  The xHCI driver has to have an indication of how
> many streams your application will need.  That's why Amit's proposal
> included a way for the application to pass in the number of streams to
> allocate.

Ok.

> 
> > If an URB is submitted on a stream when streams are disabled then it
> > should just fail and vice versa.
> 
> That code is already included in the xHCI driver, and usbfs shouldn't do
> any checking of the stream ID for the URB, since it's already in the
> lower level driver.

Ok.

--HPS