From owner-p4-projects@FreeBSD.ORG Thu Jan 8 17:35:48 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 07EF61065859; Thu, 8 Jan 2009 17:35:48 +0000 (UTC) Delivered-To: perforce@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B409B1065856; Thu, 8 Jan 2009 17:35:47 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 6447D8FC17; Thu, 8 Jan 2009 17:35:47 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.2/8.14.1) with ESMTP id n08HWpjq080136; Thu, 8 Jan 2009 10:32:51 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Thu, 08 Jan 2009 10:33:06 -0700 (MST) Message-Id: <20090108.103306.1683974155.imp@bsdimp.com> To: hselasky@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <200901081532.n08FWPDx031982@repoman.freebsd.org> References: <200901081532.n08FWPDx031982@repoman.freebsd.org> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: perforce@FreeBSD.org Subject: Re: PERFORCE change 155820 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jan 2009 17:35:49 -0000 In message: <200901081532.n08FWPDx031982@repoman.freebsd.org> Hans Petter Selasky 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