Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 Feb 2009 08:43:11 +0100
From:      Hans Petter Selasky <hselasky@c2i.net>
To:        Andrew Thompson <thompsa@freebsd.org>
Cc:        usb@freebsd.org, freebsd-current@freebsd.org
Subject:   Re: USB devfs patch
Message-ID:  <200902270843.12291.hselasky@c2i.net>
In-Reply-To: <20090227011434.GA52500@citylink.fud.org.nz>
References:  <20090226020330.GC25211@citylink.fud.org.nz> <20090227011434.GA52500@citylink.fud.org.nz>

next in thread | previous in thread | raw e-mail | index | archive | help
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=20
patch. Can you have a look at the following issues again:

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 =3D 1" ?

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


2) Should gid_t uid_t and mode_t be used?

+ =A0 =A0uint8_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:

+ =A0 =A0 =A0 /* XXX no longer needed */
+ =A0 =A0 =A0 strlcpy(ps->src_path, target, sizeof(ps->src_path));
+ =A0 =A0 =A0 ps->src_len =3D 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=A0EP_NO_ADDR 0xff /* instead of ep_addr =3D -1 */

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

=46or usb2_fs_privdata probably you are justified. I would have used=20
"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 >=3D 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.

=2D =A0 =A0 =A0 usb2_free_pipe_data(udev, iface_index, 0 - 1);
+ =A0 =A0 =A0 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:

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Fill in the endpoint bitmasks */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ed->bEndpointAddress & UE_DIR_IN)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 iface->ep_in_mask |=3D
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 1 << UE_GET_ADDR(ed->=
bEndpointAddress);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 iface->ep_out_mask |=3D
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 =3D=3D \
=A0 =A0 =A0 =A0 USB_MODE_DEVICE) ? ((xfer->endpoint & UE_DIR_IN) ? 0 : 1) :=
 \
=A0 =A0 =A0 =A0 ((xfer->endpoint & UE_DIR_IN) ? 1 : 0))

Change:

+ =A0 =A0 =A0 uint16_t ep_in_mask; =A0 =A0 =A0 =A0 =A0 =A0/* bitmask of IN =
endpoints */
+ =A0 =A0 =A0 uint16_t ep_out_mask; =A0 =A0 =A0 =A0 =A0 /* bitmask of OUT e=
ndpoints */

Into:

+ =A0 =A0 =A0 uint16_t ep_rx_mask; =A0 =A0 =A0 =A0 =A0 =A0/* bitmask of RX =
endpoints */
+ =A0 =A0 =A0 uint16_t ep_tx_mask; =A0 =A0 =A0 =A0 =A0 /* bitmask of TX end=
points */


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.

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

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

Before:

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Now, create the device its=
elf */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 snprintf(devname, sizeof(devn=
ame), USB_DEVICE_NAME
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "%u.%u.%u.%u", pd->bu=
s_index, pd->dev_index,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pd->iface_index, pd->=
ep_addr);
+

After:

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Now, create the device its=
elf */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 snprintf(devname, sizeof(devn=
ame), USB_DEVICE_NAME
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "%u/%u/%u/%u", pd->bu=
s_index, pd->dev_index,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pd->iface_index, pd->=
ep_addr);
+

You would also have to update libusb20.

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

+ =A0 =A0 =A0 devfs_set_cdevpriv(cpd, usb2_close);

=2D-HPS



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200902270843.12291.hselasky>