Re: USB devfs patch

On Thursday 26 February 2009, Andrew Thompson wrote:
> http://people.freebsd.org/~thompsa/usb-cdevs.diff

Hi,

Here is a list of comments and bugs.
The comments follow the diff from top to bottom.

1) There is a redundant setting of "is_uref = 1" ?

-       if (ploc->is_uref) {
+       if (cpd->is_uref) {
                /*
                 * We are about to alter the bus-state. Apply the
                 * required locks.
                 */
-               sx_xlock(ploc->udev->default_sx + 1);
+               sx_xlock(cpd->udev->default_sx + 1);
                mtx_lock(&Giant);       /* XXX */
+               cpd->is_uref = 1;
        }


2) Should gid_t uid_t and mode_t be used?

+    uint8_t iface_index, uint32_t uid, uint32_t gid, uint16_t mode)


3) This comment should be:

/* Now, create the device itself and an alias */

/* Now, create the device */

Because the alias is not needed - right.


4) Probably you can just remove the src_path stuff:

+       /* XXX no longer needed */
+       strlcpy(ps->src_path, target, sizeof(ps->src_path));
+       ps->src_len = strlen(ps->src_path);

5) There is no reason to use 32-bit int, except for fflags?

uint16_t bus_index;
uint8_t dev_index;
uint8_t iface_index;
uint8_t ep_addr;
#define	EP_NO_ADDR 0xff /* instead of ep_addr = -1 */

 /*
+ * Private per-device information.
+ */
+struct usb2_cdev_privdata {
+       struct usb2_bus         *bus;
+       struct usb2_device      *udev;
+       struct usb2_interface   *iface;
+       struct usb2_fifo        *rxfifo;
+       struct usb2_fifo        *txfifo;
+       int                     bus_index;      /* bus index */
+       int                     dev_index;      /* device index */
+       int                     iface_index;    /* interface index */
+       int                     ep_addr;        /* endpoint address */
+       uint8_t                 fifo_index;     /* FIFO index */
+       uint8_t                 is_read;        /* location has read access */
+       uint8_t                 is_write;       /* location has write access 
*/
+       uint8_t                 is_uref;        /* USB refcount decr. needed 
*/
+       uint8_t                 is_usbfs;       /* USB-FS is active */
+       int                     fflags;
+};

For usb2_fs_privdata probably you are justified. I would have used 
"unsigned int" for the fields that are unsigned. It has something to do with
range checks in the code not checking for values less than zero. Like:

if (fifo_index < max)
ok;
else
failure;

With int's we have to think more:

if (fifo_index >= 0 && fifo_index < max)
ok;
else
failure;

6) Some non-gcc compilers will complain unless you do 0-1 when the
destination variable is unsigned.

-       usb2_free_pipe_data(udev, iface_index, 0 - 1);
+       usb2_free_pipe_data(udev, iface_index, -1);


7) Watch out!! Maybe using the word "in" and "out" is bad idea. If
this means "read" and "write", then use "rd" and "wr", because in USB
device mode the endpoint direction is exactly the opposide like in USB
host mode:

+               /* Fill in the endpoint bitmasks */
+               if (ed->bEndpointAddress & UE_DIR_IN)
+                       iface->ep_in_mask |=
+                           1 << UE_GET_ADDR(ed->bEndpointAddress);
+               else
+                       iface->ep_out_mask |=
+                           1 << UE_GET_ADDR(ed->bEndpointAddress);

Use the the following macro for reference when computing if an
endpoint is read or write!

#define USB_GET_DATA_ISREAD(xfer) (((xfer)->flags_int.usb2_mode == \
        USB_MODE_DEVICE) ? ((xfer->endpoint & UE_DIR_IN) ? 0 : 1) : \
        ((xfer->endpoint & UE_DIR_IN) ? 1 : 0))

Change:

+       uint16_t ep_in_mask;            /* bitmask of IN endpoints */
+       uint16_t ep_out_mask;           /* bitmask of OUT endpoints */

Into:

+       uint16_t ep_rx_mask;            /* bitmask of RX endpoints */
+       uint16_t ep_tx_mask;           /* bitmask of TX endpoints */


8) The following won't work?? Freeing the cdevs have to be done in multiple
steps. Please keep the semantics of the "usb2_fifo_free_wrap" function.

When setting the configuration we have a cdev handle on EP0. If that
is closed during set_config we are recursivly killing our own handle!
Therefore there are some extra checks so that at some points we only
free non-EP0 handles, at some point only a specific interface and so
on.

+       usb2_cdev_free(udev);
        usb2_fifo_free_wrap(udev, iface_index, 0);

9) Regarding the device nodes, how about organising into sub-folders?

Before:

+                       /* Now, create the device itself */
+                       snprintf(devname, sizeof(devname), USB_DEVICE_NAME
+                           "%u.%u.%u.%u", pd->bus_index, pd->dev_index,
+                           pd->iface_index, pd->ep_addr);
+

After:

+                       /* Now, create the device itself */
+                       snprintf(devname, sizeof(devname), USB_DEVICE_NAME
+                           "%u/%u/%u/%u", pd->bus_index, pd->dev_index,
+                           pd->iface_index, pd->ep_addr);
+

10) Can you show me the implementation of the following function:

+       devfs_set_cdevpriv(cpd, usb2_close);

11) Use plain open method instead of fdopen: usb2_fdopen ?

--HPS