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

[Juergen Gross <jgross@xxxxxxxx>]

On 06/12/2015 06:23 PM, Greg KH wrote:
> 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.
Okay.

> > +/*
> > + *  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?
This interface is a stable ABI to be used outside of Linux by other
Xen guests as well. As long as those Linux internal definitions are
not guaranteed to remain unchanged forever, I'd rather have my own
defines.

> > +
> > +#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?
Correct. :-)

> > +
> > +#define USBIF_MAX_SEGMENTS_PER_REQUEST (16)
> > +#define USBIF_MAX_PORTNR       31
> Why does userspace have to know this?
Not userspace, but Xen guests. The number of segments per request and
the maximum port number are part of the interface between frontend and
backend, so they are defined here.

> > +
> > +/*
> > + * 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...
Okay.

> > +struct usbif_urb_request {
> Again, very generic structure name for a xen specific thing :(
I'll change all names to use "xenusb_" instead of "usbif_" as prefix.


Juergen

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