From owner-freebsd-arch@FreeBSD.ORG Wed Jun 10 17:09:14 2009 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 28084106564A for ; Wed, 10 Jun 2009 17:09:14 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id D68CB8FC12 for ; Wed, 10 Jun 2009 17:09:13 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 03CC946B06; Wed, 10 Jun 2009 13:09:13 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id CFAD28A06A; Wed, 10 Jun 2009 13:09:11 -0400 (EDT) From: John Baldwin To: "M. Warner Losh" Date: Wed, 10 Jun 2009 13:02:02 -0400 User-Agent: KMail/1.9.7 References: <20090609.174249.-1435625969.imp@bsdimp.com> <200906100822.15516.jhb@freebsd.org> <20090610.102144.324381338.imp@bsdimp.com> In-Reply-To: <20090610.102144.324381338.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906101302.03211.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Wed, 10 Jun 2009 13:09:11 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: freebsd-arch@freebsd.org Subject: Re: devclass_find_free_unit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Jun 2009 17:09:14 -0000 On Wednesday 10 June 2009 12:21:44 pm M. Warner Losh wrote: > In message: <200906100822.15516.jhb@freebsd.org> > John Baldwin 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. > 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). -- John Baldwin