Re: USB devfs patch

On Fri, Feb 27, 2009 at 08:43:11AM +0100, Hans Petter Selasky wrote:
> Hi Andrew,
> 
> > The patch has been updated thanks to feedback
> > http://people.freebsd.org/~thompsa/usb-cdevs2.diff
> 
> I cannot see that you have looked at the 11 issues I repored on the last 
> patch. Can you have a look at the following issues again:

Yes I have, 1, 2, 3, 6, 7 & 8 have all been fixed. For #10 use
http://fxr.watson.org


Andrew 
> 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);
> +
> 
> You would also have to update libusb20.
> 
> 10) Can you show me the implementation of the following function:
> 
> + ? ? ? devfs_set_cdevpriv(cpd, usb2_close);
> 
> --HPS