From owner-p4-projects@FreeBSD.ORG Thu Jan 25 22:39:51 2007 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id C1E8216A40A; Thu, 25 Jan 2007 22:39:51 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 846FF16A402 for ; Thu, 25 Jan 2007 22:39:51 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 22C2113C459 for ; Thu, 25 Jan 2007 22:39:51 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.13.4/8.13.4) with ESMTP id l0PMdYod032570; Thu, 25 Jan 2007 15:39:35 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Thu, 25 Jan 2007 15:40:04 -0700 (MST) Message-Id: <20070125.154004.-957810070.imp@bsdimp.com> To: hselasky@c2i.net From: "M. Warner Losh" In-Reply-To: <200701191057.25656.hselasky@c2i.net> References: <200701152255.l0FMtKId033379@repoman.freebsd.org> <20070119.015122.514366110.imp@bsdimp.com> <200701191057.25656.hselasky@c2i.net> X-Mailer: Mew version 4.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Thu, 25 Jan 2007 15:39:36 -0700 (MST) Cc: perforce@freebsd.org Subject: Re: PERFORCE change 112957 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, 25 Jan 2007 22:39:52 -0000 In message: <200701191057.25656.hselasky@c2i.net> Hans Petter Selasky writes: : On Friday 19 January 2007 09:51, M. Warner Losh wrote: : > In message: <200701152255.l0FMtKId033379@repoman.freebsd.org> : > : > Hans Petter Selasky writes: : > : ==== //depot/projects/usb/src/sys/dev/pccbb/pccbb.c#3 (text+ko) ==== : > : : > : @@ -302,10 +302,11 @@ : > : * for the kldload/unload case to work. If we failed to do that, then : > : * we'd get duplicate devices when cbb.ko was reloaded. : > : */ : > : - device_get_children(brdev, &devlist, &numdevs); : > : - for (tmp = 0; tmp < numdevs; tmp++) : > : - device_delete_child(brdev, devlist[tmp]); : > : - free(devlist, M_TEMP); : > : + if (!device_get_children(brdev, &devlist, &numdevs)) { : > : + for (tmp = 0; tmp < numdevs; tmp++) : > : > Actually this is a problem. While the old code ignores errors, I : > don't think that's really the right thing to do here. It is critical : > that the children be deleted. : > : : Then maybe you should implement a "device_for_each_safe()" function that : iterates the devices? : : Or something like "device_get_first_child()" ? If we had a topology lock for newbus, like we do for geom, then we could return the child's memory directly w/o worrying that it will change out from under the caller. : BTW: I see that several places "device_get_children()" is used in combination : with "device_delete_child()". How about factoring that out. I currently put : the following in "usb_subr.c": : : /*---------------------------------------------------------------------------* : * device_delete_all_children - delete all children of a device : *---------------------------------------------------------------------------*/ : int32_t int : device_delete_all_children(device_t dev) : { : device_t *devlist; : int32_t devcount; : int32_t error; int devcount, error; : : error = device_get_children(dev, &devlist, &devcount); : if (error == 0) { : : while (devcount-- > 0) { : error = device_delete_child(dev, devlist[devcount]); : if (error) { : break; : } : } : free(devlist, M_TEMP); : } : return error; : } : : This function still uses "device_get_children()", but if it is implemented in : "kern/subr_bus.c", then it can access the child list directly, and we are : saved the memory allocation/deallocation, which is the weak point. : : Also the memory is allocated with M_NOWAIT. Yes. That's a weak point. Warner