Date: Wed, 10 Jun 2009 13:43:15 -0400 From: John Baldwin <jhb@freebsd.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: freebsd-arch@freebsd.org Subject: Re: devclass_find_free_unit Message-ID: <200906101343.15311.jhb@freebsd.org> In-Reply-To: <20090610.112813.623117012.imp@bsdimp.com> References: <200906100822.15516.jhb@freebsd.org> <200906101302.03211.jhb@freebsd.org> <20090610.112813.623117012.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 10 June 2009 1:28:13 pm M. Warner Losh wrote:
> In message: <200906101302.03211.jhb@freebsd.org>
> John Baldwin <jhb@freebsd.org> writes:
> : On Wednesday 10 June 2009 12:21:44 pm M. Warner Losh wrote:
> : > In message: <200906100822.15516.jhb@freebsd.org>
> : > John Baldwin <jhb@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906101343.15311.jhb>