On Wed, Aug 19, 2009 at 11:38:39AM -0400, Alan Stern wrote:
> On Tue, 18 Aug 2009, Sarah Sharp wrote:
>
> > +Device drivers will call this API to request that the host controller driver
> > +allocate memory so the driver can use up to num_requested_streams stream IDs.
> > +They must pass an array of endpoint addresses that need to be setup with similar
> > +stream IDs. This is to insure that a UASP driver will be able to use the same
> > +stream ID for the bulk IN and OUT endpoints used in a Bi-directional command
> > +sequence.
>
> I don't understand this. Let's suppose the driver made two different
> calls to usb_alloc_streams(), one for the bulk IN and one for the bulk
> OUT endpoint, and let's say that each call allocated 8 streams. Why
> wouldn't the driver then be able to use the same stream ID (between 1
> and 8) for both endpoints in a bi-directional command?
If the driver got 8 stream IDs for both calls, then it would be fine for
them to make two calls. However, different endpoints can support
different numbers of streams. So a driver could ask for 255 streams for
both endpoints, and get allocated 63 and 31 streams. It would be up to
the driver to use streams 1-31 for both endpoints if it wanted to
coordinate between the two endpoints. It might be easier for the driver
to make one call instead.
But it's unlikely that a UASP device will have endpoints with different
numbers of supported streams. The real reason that paragraph is in
there is because of secondary stream context arrays. They're sort of
like second-level page tables, and the xHCI driver needs to reserve some
stream IDs to point to these second-level entries. If the xHCI driver
doesn't set up all the endpoints at the same time, there's a chance the
xHCI driver will reserve different stream IDs in the paired endpoints if
it has to use secondary stream arrays.
I was going to deal with secondary stream arrays and reserved streams by
making the drivers ask for the next available stream ID. In the end, I
found out that the first host controllers probably aren't going to
support secondary stream arrays, and decided it was too much work.
The code changed but the documentation didn't.
> > +static struct usb_host_endpoint **usb_make_endpoint_array(
> > + struct usb_device *dev, unsigned int *epaddr,
> > + unsigned int num_eps, gfp_t mem_flags)
> > +{
> > + struct usb_host_endpoint **eps;
> > + unsigned int epnum;
> > + int i;
> > +
> > + eps = kmalloc(sizeof(struct usb_host_endpoint *)*num_eps, mem_flags);
> > + if (!eps)
> > + return NULL;
> > +
> > + for (i = 0; i < num_eps; i++) {
> > + epnum = epaddr[i] & USB_ENDPOINT_NUMBER_MASK;
> > + if (usb_endpoint_out(epaddr[i]))
> > + eps[i] = dev->ep_out[epnum];
> > + else
> > + eps[i] = dev->ep_out[epnum];
> > + if (!eps[i]) {
> > + kfree(eps);
> > + return NULL;
> > + }
> > + }
> > + return eps;
> > +}
>
> This seems like unnecessary overhead. Why not ask drivers to pass an
> array of pointers in the first place? That's just as natural as an
> array of endpoint addresses.
I thought drivers didn't have access to the usb_host_endpoints, or
weren't supposed to access them anyway. Functions I looked at, like
usb_reset_endpoint(), all took endpoint addresses.
> > @@ -1199,6 +1208,7 @@ struct urb {
> > struct usb_device *dev; /* (in) pointer to associated device */
> > struct usb_host_endpoint *ep; /* (internal) pointer to endpoint */
> > unsigned int pipe; /* (in) pipe information */
> > + unsigned int stream_id; /* (in) stream ID */
> > int status; /* (return) non-ISO status */
> > unsigned int transfer_flags; /* (in) URB_SHORT_NOT_OK | ...*/
> > void *transfer_buffer; /* (in) associated data buffer */
>
> Keeping track of the number of streams in the usb_host_endpoint
> structure seems like a reasonable thing to do. Then automatically
> re-enabling the streams after a device reset would be fairly
> straightforward.
Sure. Should we re-enable streams for a driver that doesn't have
pre_reset() and post_reset() methods? If the driver is going to be
unbound and rebound, then won't it try to allocate the streams again?
> You could then add some error checking to usb_submit_urb(). Verify
> that urb->stream_id <= the number of streams allocated for urb->ep.
Yes, that would take lots of checking out of the xHCI driver and put it
into the USB core.
Sarah
--
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
