Date: Thu, 25 Jan 2007 15:40:04 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: hselasky@c2i.net Cc: perforce@freebsd.org Subject: Re: PERFORCE change 112957 for review Message-ID: <20070125.154004.-957810070.imp@bsdimp.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <200701191057.25656.hselasky@c2i.net>
Hans Petter Selasky <hselasky@c2i.net> writes:
: On Friday 19 January 2007 09:51, M. Warner Losh wrote:
: > In message: <200701152255.l0FMtKId033379@repoman.freebsd.org>
: >
: > Hans Petter Selasky <hselasky@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070125.154004.-957810070.imp>