Menu
▾
▴
Re: [libusb] [PATCH 9/9] usbfs: Add support for allocating / freeing streams
|
From: Sarah S. <sar...@li...> - 2013-11-14 23:08:44
|
All right, this patchset looks sane other than the one small bug I
mentioned, and the couple comments I have below.
Have you talked to the other libusb or libusbX developers about adding
streams support to libusb? ISTR that you said you needed libusb changes
in order to get the virtualized UAS device to work in qemu.
On Wed, Oct 09, 2013 at 05:19:31PM +0200, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hde...@re...>
You need a commit message here, so we can tell usbfs users what bulk
streams are (reference the documentation), and how to use this new
interface (e.g. they need to allocate streams before using the new
uurb->stream_id field).
> ---
> drivers/usb/core/devio.c | 118 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/usbdevice_fs.h | 7 +++
> 2 files changed, 125 insertions(+)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index bfb2821..4ca7e86 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -778,6 +778,79 @@ static struct usb_host_endpoint *ep_to_host_endpoint(struct usb_device *dev,
> return dev->ep_out[ep & USB_ENDPOINT_NUMBER_MASK];
> }
>
> +static int parse_usbdevfs_streams(struct dev_state *ps,
> + struct usbdevfs_streams __user *streams,
> + unsigned int *num_streams_ret,
> + unsigned int *num_eps_ret,
> + struct usb_host_endpoint ***eps_ret,
> + struct usb_interface **intf_ret)
> +{
> + unsigned int i, num_streams, num_eps;
> + struct usb_host_endpoint **eps;
> + struct usb_interface *intf = NULL;
> + unsigned char ep;
> + int ifnum, ret;
> +
> + if (get_user(num_streams, &streams->num_streams) ||
> + get_user(num_eps, &streams->num_eps))
> + return -EFAULT;
> +
> + if (num_eps < 1 || num_eps > USB_MAXENDPOINTS)
> + return -EINVAL;
> +
> + /* The XHCI controller allows max 1024 streams */
> + if (num_streams_ret && (num_streams < 2 || num_streams > 1024))
> + return -EINVAL;
We really shouldn't hard-code the 1024 streams value. I'm actually not
sure where you got that value. Each xHCI host can indicate the max
primary stream array size (basically the number of streams) it supports,
see MaxPSASize in section 5.3.6 of the xHCI spec. An xHCI host can
choose to indicate it doesn't support streams at all, by setting that
value to zero.
Ideally, we should have something like bus->sg_tablesize that allows the
xHCI host to indicate the number of streams it supports. Perhaps
usb_bus->max_streams?
Sarah Sharp
> +
> + eps = kmalloc(num_eps * sizeof(*eps), GFP_KERNEL);
> + if (!eps)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_eps; i++) {
> + if (get_user(ep, &streams->eps[i])) {
> + ret = -EFAULT;
> + goto error;
> + }
> + eps[i] = ep_to_host_endpoint(ps->dev, ep);
> + if (!eps[i]) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
> + /* usb_alloc/free_streams operate on an usb_interface */
> + ifnum = findintfep(ps->dev, ep);
> + if (ifnum < 0) {
> + ret = ifnum;
> + goto error;
> + }
> +
> + if (i == 0) {
> + ret = checkintf(ps, ifnum);
> + if (ret < 0)
> + goto error;
> + intf = usb_ifnum_to_if(ps->dev, ifnum);
> + } else {
> + /* Verify all eps belong to the same interface */
> + if (ifnum != intf->altsetting->desc.bInterfaceNumber) {
> + ret = -EINVAL;
> + goto error;
> + }
> + }
> + }
> +
> + if (num_streams_ret)
> + *num_streams_ret = num_streams;
> + *num_eps_ret = num_eps;
> + *eps_ret = eps;
> + *intf_ret = intf;
> +
> + return 0;
> +
> +error:
> + kfree(eps);
> + return ret;
> +}
> +
> static int match_devt(struct device *dev, void *data)
> {
> return dev->devt == (dev_t) (unsigned long) data;
> @@ -1992,6 +2065,45 @@ static int proc_disconnect_claim(struct dev_state *ps, void __user *arg)
> return claimintf(ps, dc.interface);
> }
>
> +static int proc_alloc_streams(struct dev_state *ps, void __user *arg)
> +{
> + unsigned num_streams, num_eps;
> + struct usb_host_endpoint **eps;
> + struct usb_interface *intf;
> + int r;
> +
> + r = parse_usbdevfs_streams(ps, arg, &num_streams, &num_eps,
> + &eps, &intf);
> + if (r)
> + return r;
> +
> + destroy_async_on_interface(ps,
> + intf->altsetting[0].desc.bInterfaceNumber);
> +
> + r = usb_alloc_streams(intf, eps, num_eps, num_streams, GFP_KERNEL);
> + kfree(eps);
> + return r;
> +}
> +
> +static int proc_free_streams(struct dev_state *ps, void __user *arg)
> +{
> + unsigned num_eps;
> + struct usb_host_endpoint **eps;
> + struct usb_interface *intf;
> + int r;
> +
> + r = parse_usbdevfs_streams(ps, arg, NULL, &num_eps, &eps, &intf);
> + if (r)
> + return r;
> +
> + destroy_async_on_interface(ps,
> + intf->altsetting[0].desc.bInterfaceNumber);
> +
> + r = usb_free_streams(intf, eps, num_eps, GFP_KERNEL);
> + kfree(eps);
> + return r;
> +}
> +
> /*
> * NOTE: All requests here that have interface numbers as parameters
> * are assuming that somehow the configuration has been prevented from
> @@ -2168,6 +2280,12 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
> case USBDEVFS_DISCONNECT_CLAIM:
> ret = proc_disconnect_claim(ps, p);
> break;
> + case USBDEVFS_ALLOC_STREAMS:
> + ret = proc_alloc_streams(ps, p);
> + break;
> + case USBDEVFS_FREE_STREAMS:
> + ret = proc_free_streams(ps, p);
> + break;
> }
> usb_unlock_device(dev);
> if (ret >= 0)
> diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
> index cbf122d..abe5f4b 100644
> --- a/include/uapi/linux/usbdevice_fs.h
> +++ b/include/uapi/linux/usbdevice_fs.h
> @@ -147,6 +147,11 @@ struct usbdevfs_disconnect_claim {
> char driver[USBDEVFS_MAXDRIVERNAME + 1];
> };
>
> +struct usbdevfs_streams {
> + unsigned int num_streams; /* Not used by USBDEVFS_FREE_STREAMS */
> + unsigned int num_eps;
> + unsigned char eps[0];
> +};
>
> #define USBDEVFS_CONTROL _IOWR('U', 0, struct usbdevfs_ctrltransfer)
> #define USBDEVFS_CONTROL32 _IOWR('U', 0, struct usbdevfs_ctrltransfer32)
> @@ -179,5 +184,7 @@ struct usbdevfs_disconnect_claim {
> #define USBDEVFS_RELEASE_PORT _IOR('U', 25, unsigned int)
> #define USBDEVFS_GET_CAPABILITIES _IOR('U', 26, __u32)
> #define USBDEVFS_DISCONNECT_CLAIM _IOR('U', 27, struct usbdevfs_disconnect_claim)
> +#define USBDEVFS_ALLOC_STREAMS _IOR('U', 28, struct usbdevfs_streams)
> +#define USBDEVFS_FREE_STREAMS _IOR('U', 29, struct usbdevfs_streams)
>
> #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
> --
> 1.8.3.1
>
|
