On Wed, Aug 19, 2009 at 03:35:03PM +0200, Oliver Neukum wrote:
>
> > +Driver implications
> > +===================
> > +
> > +int usb_alloc_streams(struct usb_driver *driver,
> > + struct usb_device *dev,
>
> This is very problematic. We never pass the usb_driver. Subsystems
> like usb serial might not even have it available. Is this needed for anything
> but debugging?
I was originally going to check to make sure the driver was
actually bound to the interface it was changing. I can remove that
argument.
> [..]
> > +Clean up
> > +========
> > +
> > +If a driver wishes to stop using streams to communicate with the device,it
> > +should call
> > +
> > +int usb_free_streams(struct usb_driver *driver,
>
> What is a driver supposed to do if this returns an error? This will
> have to be used in disconnect(), which cannot fail.
I see. I can make sure there will be a spot on the command ring for the
command to remove the stream rings. Unfortunately, the host controller
can fail that command if it runs out of bandwidth or resources.
Deallocating streams shouldn't change the bandwidth allocation (unless
the host controller is broken), but I can't prevent the host controller
from running out of internal resources.
What should the xHCI driver do in that case? Wait a while and re-submit
the command? I'm not sure if it's wise to tie up a disconnect()
function with command resubmissions, so perhaps usb_free_streams()
should only guarantee that the streams will be freed sometime in the
future?
On the other hand, if the device has truly disconnected, the USB core
will deallocate the whole device, which will cause the internal xHCI
structures associated with that device to go away. That command won't
fail, although I'll have to reserve a spot on the command ring for that.
> > + struct usb_device *dev,
> > + unsigned int *epaddr,
> > + unsigned int num_eps,
> > + gfp_t mem_flags);
> [..]
> > + */
> > +int usb_alloc_streams(struct usb_driver *driver, struct usb_device *dev,
> > + unsigned int *epaddr, unsigned int num_eps,
> > + unsigned int num_streams, gfp_t mem_flags)
> > +{
> > + struct usb_hcd *hcd;
> > + struct usb_host_endpoint **eps;
> > + int i;
> > +
> > + hcd = bus_to_hcd(dev->bus);
> > + if (!hcd->driver->alloc_streams)
> > + return -EINVAL;
> > + if (dev->speed != USB_SPEED_SUPER)
> > + return -EINVAL;
> > + eps = usb_make_endpoint_array(dev, epaddr, num_eps, mem_flags);
> > + if (!eps)
> > + return -ENOMEM;
> > +
> > + /* Streams only apply to bulk endpoints. */
> > + for (i = 0; i < num_eps; i++) {
> > + if (!usb_endpoint_xfer_bulk(&eps[i]->desc)) {
> > + kfree(eps);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + i = hcd->driver->alloc_streams(hcd, dev, eps, num_eps,
> > + num_streams, mem_flags);
>
> Locking?
Is there something specific I should lock?
The xHCI driver already has internal locking for this command. The
xhci_alloc_streams() function takes xhci->lock, checks that all the
endpoints are valid (e.g. don't already have streams or are
transitioning to streams, or have URBs enqueued for them). Then it
marks all the endpoints as being in transition to having streams and
submits the command to change the host controller's internal endpoint
representation. Then it releases the lock and waits on the command to
finish before returning.
If the driver somehow submitted an URB to the transitioning endpoint
while we were waiting on the command, the xHCI driver would reject it.
If the driver tried to call usb_alloc_streams in two separate threads
with similar endpoints, one of them would fail because the endpoint is
marked as transitioning.
I do think there would be a problem if the driver called
usb_alloc_streams() and then usb_free_streams() was called before the
allocation command finished. Since disconnect can be called at any
time, I had better fix that. :)
>
> > + kfree(eps);
> > + return i;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_alloc_streams);
> > +
> > +/* Reverts a group of bulk endpoints back to not using stream IDs.
> > + * Can fail if we run out of memory, or are given bad arguments.
> > + */
> > +int usb_free_streams(struct usb_driver *driver, struct usb_device *dev,
> > + unsigned int *epaddr, unsigned int num_eps, gfp_t mem_flags)
> > +{
> > + struct usb_hcd *hcd;
> > + struct usb_host_endpoint **eps;
> > + int i;
> > +
> > + hcd = bus_to_hcd(dev->bus);
> > + if (!hcd->driver->free_streams)
> > + return -EINVAL;
>
> Too late. You must check this on allocation, or you'll have streams that
> cannot be freed.
>
> > + eps = usb_make_endpoint_array(dev, epaddr, num_eps, mem_flags);
> > + if (!eps)
> > + return -ENOMEM;
>
> This is very problematic. You cannot deal with -ENOMEM in disconnect().
> It would be better to allocate this in advance, or from a mempool.
Ok, I'll look into correcting these two. Thanks!
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
