Re: [Xen-devel] [Patch V2 1/3] usb: Add Xen pvUSB protocol description

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Fri, Jun 12, 2015 at 04:09:59PM +0200, Juergen Gross wrote:
> +enum usb_spec_version {
> +     USB_VER_UNKNOWN = 0,
> +     USB_VER_USB11,
> +     USB_VER_USB20,
> +     USB_VER_USB30,  /* not supported yet */
> +};
> +

You are defining a bunch of things in this .h file that start with
"usb" yet are not part of the USB core at all, but rather are xen
specific.  Please don't polute the namespace with such things.

> +/*
> + *  USB pipe in usbif_request
> + *
> + *  - port number:   bits 0-4
> + *                           (USB_MAXCHILDREN is 31)
> + *
> + *  - operation flag:        bit 5
> + *                           (0 = submit urb,
> + *                            1 = unlink urb)
> + *
> + *  - direction:     bit 7
> + *                           (0 = Host-to-Device [Out]
> + *                            1 = Device-to-Host [In])
> + *
> + *  - device address:        bits 8-14
> + *
> + *  - endpoint:              bits 15-18
> + *
> + *  - pipe type:     bits 30-31
> + *                           (00 = isochronous, 01 = interrupt,
> + *                            10 = control, 11 = bulk)
> + */
> +
> +#define USBIF_PIPE_PORT_MASK 0x0000001f
> +#define USBIF_PIPE_UNLINK    0x00000020
> +#define USBIF_PIPE_DIR               0x00000080
> +#define USBIF_PIPE_DEV_MASK  0x0000007f
> +#define USBIF_PIPE_DEV_SHIFT 8
> +#define USBIF_PIPE_EP_MASK   0x0000000f
> +#define USBIF_PIPE_EP_SHIFT  15
> +#define USBIF_PIPE_TYPE_MASK 0x00000003
> +#define USBIF_PIPE_TYPE_SHIFT        30
> +#define USBIF_PIPE_TYPE_ISOC 0
> +#define USBIF_PIPE_TYPE_INT  1
> +#define USBIF_PIPE_TYPE_CTRL 2
> +#define USBIF_PIPE_TYPE_BULK 3

Why can't you just use the defines we have for this already?

> +
> +#define usbif_pipeportnum(pipe)                      ((pipe) & 
> USBIF_PIPE_PORT_MASK)
> +#define usbif_setportnum_pipe(pipe, portnum) ((pipe) | (portnum))
> +
> +#define usbif_pipeunlink(pipe)                       ((pipe) & 
> USBIF_PIPE_UNLINK)
> +#define usbif_pipesubmit(pipe)                       
> (!usbif_pipeunlink(pipe))
> +#define usbif_setunlink_pipe(pipe)           ((pipe) | USBIF_PIPE_UNLINK)
> +
> +#define usbif_pipein(pipe)                   ((pipe) & USBIF_PIPE_DIR)
> +#define usbif_pipeout(pipe)                  (!usbif_pipein(pipe))
> +
> +#define usbif_pipedevice(pipe)                       \
> +             (((pipe) >> USBIF_PIPE_DEV_SHIFT) & USBIF_PIPE_DEV_MASK)
> +
> +#define usbif_pipeendpoint(pipe)             \
> +             (((pipe) >> USBIF_PIPE_EP_SHIFT) & USBIF_PIPE_EP_MASK)
> +
> +#define usbif_pipetype(pipe)                 \
> +             (((pipe) >> USBIF_PIPE_TYPE_SHIFT) & USBIF_PIPE_TYPE_MASK)
> +#define usbif_pipeisoc(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_ISOC)
> +#define usbif_pipeint(pipe)  (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_INT)
> +#define usbif_pipectrl(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_CTRL)
> +#define usbif_pipebulk(pipe) (usbif_pipetype(pipe) == USBIF_PIPE_TYPE_BULK)


Same for all of these?

> +
> +#define USBIF_MAX_SEGMENTS_PER_REQUEST (16)
> +#define USBIF_MAX_PORTNR     31

Why does userspace have to know this?

> +
> +/*
> + * RING for transferring urbs.
> + */
> +struct usbif_request_segment {
> +     grant_ref_t gref;
> +     uint16_t offset;
> +     uint16_t length;
> +};

Please use proper kernel types for things that go outside of the kernel
(__u16 and the like).  There is no "uint16_t" as a valid type in the
kernel, sorry.  Well, ok, we have it, just because it snuck in somehow,
but it should be removed one of these days...

> +struct usbif_urb_request {

Again, very generic structure name for a xen specific thing :(

I stopped reading here.

greg k-h

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel