devclass_find_free_unit

[John Baldwin]

On Wednesday 10 June 2009 1:28:13 pm M. Warner Losh wrote:
> In message: <200906101302.03211.jhb at freebsd.org>
>             John Baldwin <jhb at freebsd.org> writes:
> : On Wednesday 10 June 2009 12:21:44 pm M. Warner Losh wrote:
> : > In message: <200906100822.15516.jhb at freebsd.org>
> : >             John Baldwin <jhb at freebsd.org> writes:
> : > : On Tuesday 09 June 2009 7:42:49 pm M. Warner Losh wrote:
> : > : > What purpose does devclass_find_free_unit serve?  I think it can 
safely 
> : be
> : > : > eliminated from the tree.  The current design is racy.
> : > : > 
> : > : > Comments?
> : > : > 
> : > : > It is currently used:
> : > : > 
> : > : > ./arm/xscale/ixp425/.svn/text-base/avila_ata.c.svn-base:        
> : > : device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 
0));
> : > : > ./arm/xscale/ixp425/avila_ata.c:        device_add_child(dev, "ata", 
> : > : devclass_find_free_unit(ata_devclass, 0));
> : > : > ./arm/at91/.svn/text-base/at91_cfata.c.svn-base:        
> : > : device_add_child(dev, "ata", devclass_find_free_unit(ata_devclass, 
0));
> : > : > ./arm/at91/at91_cfata.c:        device_add_child(dev, "ata", 
> : > : devclass_find_free_unit(ata_devclass, 0));
> : > : > ./powerpc/psim/.svn/text-base/ata_iobus.c.svn-base:                      
> : > : devclass_find_free_unit(ata_devclass, 0));
> : > : > 
> : > : > # All the above can be replaced with a simple '-1'.
> : > : > 
> : > : > ata/ata-pci.c:      unit : devclass_find_free_unit(ata_devclass, 
2));
> : > : > ata/ata-usb.c:              devclass_find_free_unit(ata_devclass, 
2))) 
> : == 
> : > : NULL) {
> : > : > 
> : > : > These can likely be replaced by '2', but that may result in a 
warning
> : > : > message being printed that likely can be eliminated...
> : > : 
> : > : ata does this so it can reserve ata0 and ata1 for the "legacy" ATA 
> : channels on 
> : > : legacy ATA PCI adapters.  That is, if you have both SATA controllers 
and a 
> : > : PATA controller, this allows the two PATA channels to always be ata0 
and 
> : ata1 
> : > : and the PATA drivers to always be ad0 - ad3.  You could perhaps 
implement 
> : > : this in 8.x now by a really horrendous hack of having ISA hints for 
ata0 
> : and 
> : > : ata1 and letting bus_hint_device_unit() in the atapci driver claim 
those 
> : > : hints for the channels on PATA controllers.
> : > 
> : > I think it already does something akin to this:
> : > 
> : >     /* attach all channels on this controller */
> : >     for (unit = 0; unit < ctlr->channels; unit++) {
> : >         if ((ctlr->ichannels & (1 << unit)) == 0)
> : >             continue;
> : >         child = device_add_child(dev, "ata",
> : >             ((unit == 0 || unit == 1) && ctlr->legacy) ?
> : >             unit : devclass_find_free_unit(ata_devclass, 2));
> : >         if (child == NULL)
> : >             device_printf(dev, "failed to add ata child device\n");
> : >         else
> : >             device_set_ivars(child, (void *)(intptr_t)unit);
> : >     }
> : > 
> : > Why not just replace devclass_find_free_unit with '2'?
> : 
> : Because if you add 'ata2', and 'ata2' exists it will fail, it won't rename 
it 
> : to ata3.  And that is what ata is trying to do.  It basically wants 
> : to "reserve" ata0 and ata1 and then use device_add_child(..., -1).  
However, 
> : device_add_child(..., -1) will not "reserve" ata0 and ata1.
> 
> Ah yes.  It does just fail.  However, setting the unit here is racy.
> If we were to make the device tree probe more parallel, then we may
> have a case where devclass_find_free_unit gets called from two
> different threads, returning the same number, then the
> device_child_add works for only one of these threads...

Yes, it is quite racey.

> : > All the other users in the tree aer bogus and should be replaced by
> : > -1.  Well, I'm not 100% sure about the ata-usb.c patch, since that
> : > would also be necessary to avoid collision.  And the above code really
> : > only applies to x86-based machine, right?  There's no need to do that
> : > for non-intel boxes.  Or is the assumption on those boxes the
> : > controller would never be in legacy.
> : 
> : Any machine that can have a PCI PATA controller or a PCI SATA controller 
> : operating in "legacy" mode.  That said, the compatability bits probably 
don't 
> : matter as much on non-x86 as there are not older releases to be compatible 
> : with (or the impact would be less severe if we renumber people's drives at 
> : least).
> 
> Yes.  I guess I was asking if we need an ifdef for this behavior or
> not...  I guess not..
> 
> I think we need to have a better way to 'reserve' a unit than we have
> today.  I think this will be better to do that and retire
> devclass_find_free_unit.  I think that only one or two uses in the
> tree are legit...

Well, that was why I suggested possibly depending on the (already-existing) 
ata[01] ISA hints and having a bus_hint_device_unit() method for the atapci 
driver that let PATA channels claim ata[01].  Then the ata driver could 
always use device_add_unit(..., -1) to add "ata" devices.  It is sort of odd, 
but it actually maps what the code is trying to do: let the PATA ATA 
channels "look like" the old ISA channels.

-- 
John Baldwin