Date: Thu, 08 Jan 2009 10:33:06 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: hselasky@FreeBSD.org Cc: perforce@FreeBSD.org Subject: Re: PERFORCE change 155820 for review Message-ID: <20090108.103306.1683974155.imp@bsdimp.com> In-Reply-To: <200901081532.n08FWPDx031982@repoman.freebsd.org> References: <200901081532.n08FWPDx031982@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <200901081532.n08FWPDx031982@repoman.freebsd.org>
Hans Petter Selasky <hselasky@FreeBSD.org> writes:
: USB memory usage reduction patch.
This likely needs to be more descriptive.
: ==== //depot/projects/usb/src/sys/dev/usb2/core/usb2_device.c#45 (text+ko) ====
:
: @@ -1290,19 +1290,23 @@
: * Find an unused device index. In USB Host mode this is the
: * same as the device address.
: *
: - * NOTE: Index 1 is reserved for the Root HUB.
: + * Device index zero is not used and device index 1 should
: + * always be the root hub.
: */
: - for (device_index = USB_ROOT_HUB_ADDR; device_index !=
: - USB_MAX_DEVICES; device_index++) {
: + for (device_index = USB_ROOT_HUB_ADDR;; device_index++) {
This looks wrong. ';;' seems wrong to me. While it is acceptable 'C'
code, the fact that you have an if statement at the end means that you
should write this like:
for (device_index = USB_ROOT_HUB_ADDR;
bus->devices[device_index] != NULL; device_index++) {
: +#if (USB_ROOT_HUB_ADDR > USB_MIN_DEVICES)
: +#error "Incorrect device limit."
: +#endif
This likely is the wrong place for this #ifdef.
: + if (device_index == bus->devices_max) {
: + device_printf(bus->bdev,
: + "No free USB device "
: + "index for new device!\n");
: + return (NULL);
: + }
: if (bus->devices[device_index] == NULL)
: break;
See above: likely want to merge this statement into the for loop.
: ==== //depot/projects/usb/src/sys/dev/usb2/core/usb2_hub.c#28 (text+ko) ====
:
: @@ -1520,8 +1520,12 @@
: * The root HUB device is never suspended
: * and we simply skip it.
: */
: - for (x = USB_ROOT_HUB_ADDR + 1;
: - x != USB_MAX_DEVICES; x++) {
: + for (x = USB_ROOT_HUB_ADDR + 1;; x++) {
: +#if ((USB_ROOT_HUB_ADDR + 1) > USB_MIN_DEVICES)
: +#error "Incorrect device limit."
: +#endif
: + if (x == bus->devices_max)
: + break;
Same comments as above. This #if is in the wrong place for a compile
time assert. The for loop is weirdly constructed.
:
: udev = bus->devices[x];
: if (udev == NULL)
: @@ -1564,8 +1568,12 @@
:
: /* Re-loop all the devices to get the actual state */
:
: - for (x = USB_ROOT_HUB_ADDR + 1;
: - x != USB_MAX_DEVICES; x++) {
: + for (x = USB_ROOT_HUB_ADDR + 1;; x++) {
: +#if ((USB_ROOT_HUB_ADDR + 1) > USB_MIN_DEVICES)
: +#error "Incorrect device limit."
: +#endif
: + if (x == bus->devices_max)
: + break;
Same comments as above. This #if is in the wrong place for a compile
time assert. The for loop is weirdly constructed.
: ==== //depot/projects/usb/src/sys/dev/usb2/include/usb2_defs.h#7 (text+ko) ====
:
: @@ -35,6 +35,8 @@
: #define USB_EP_MAX (2*16) /* hardcoded */
: #define USB_FIFO_MAX (4 * USB_EP_MAX)
:
: +#define USB_MIN_DEVICES 2 /* unused + root HUB */
: +
: #define USB_MAX_DEVICES USB_DEV_MAX /* including virtual root HUB and
: * address zero */
: #define USB_MAX_ENDPOINTS USB_EP_MAX /* 2 directions on 16 endpoints */
: @@ -64,5 +66,7 @@
: #if (USB_EP_MAX < (2*16))
: #error "Misconfigured limits #3"
: #endif
: -
: +#if (USB_MAX_DEVICES < USB_MIN_DEVICES)
: +#error "Misconfigured limits #4"
: +#endif
: #endif /* _USB2_DEFS_H_ */
These #error messages are lame. Please make them less lame and more
descriptive.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090108.103306.1683974155.imp>