PERFORCE change 112957 for review

[M. Warner Losh]

In message: <200701191057.25656.hselasky at c2i.net>
            Hans Petter Selasky <hselasky at c2i.net> writes:
: On Friday 19 January 2007 09:51, M. Warner Losh wrote:
: > In message: <200701152255.l0FMtKId033379 at repoman.freebsd.org>
: >
: >             Hans Petter Selasky <hselasky at 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