From owner-freebsd-new-bus@FreeBSD.ORG Tue Jan 17 18:09:33 2006 Return-Path: X-Original-To: freebsd-new-bus@freebsd.org Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2879216A42B; Tue, 17 Jan 2006 18:09:33 +0000 (GMT) (envelope-from john@baldwin.cx) Received: from speedfactory.net (mail6.speedfactory.net [66.23.216.219]) by mx1.FreeBSD.org (Postfix) with ESMTP id 365A943D58; Tue, 17 Jan 2006 18:09:32 +0000 (GMT) (envelope-from john@baldwin.cx) Received: from server.baldwin.cx (unverified [66.23.211.162]) by speedfactory.net (SurgeMail 3.5b3) with ESMTP id 6283689 for multiple; Tue, 17 Jan 2006 13:08:07 -0500 Received: from localhost (john@localhost [127.0.0.1]) by server.baldwin.cx (8.13.4/8.13.4) with ESMTP id k0HI9Ci0039363; Tue, 17 Jan 2006 13:09:25 -0500 (EST) (envelope-from john@baldwin.cx) From: John Baldwin To: freebsd-new-bus@freebsd.org Date: Tue, 17 Jan 2006 13:08:40 -0500 User-Agent: KMail/1.9.1 References: <200601061401.57308.john@baldwin.cx> In-Reply-To: <200601061401.57308.john@baldwin.cx> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200601171308.41539.john@baldwin.cx> X-Virus-Scanned: ClamAV 0.87.1/1244/Tue Jan 17 03:46:07 2006 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-1.4 required=4.2 tests=ALL_TRUSTED autolearn=failed version=3.1.0 X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on server.baldwin.cx X-Server: High Performance Mail Server - http://surgemail.com r=1653887525 Cc: dfr@freebsd.org Subject: Re: Patch to prevent cycles in the devclass tree X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2006 18:09:33 -0000 On Friday 06 January 2006 14:01, John Baldwin wrote: > I have been doing some work recently to make the ACPI and OFW > (OpenFirmware) PCI bus drivers inherit from the PCI bus driver. As part of > this I've run into an issue because the subclass drivers (ACPI PCI and OFW > PCI) use the same name "pci" as the superclass. They do this so that they > can override the superclass when a "pci" device is added as a child of a > "pcib" device. The problem I ran into is that when the subclass was > registered, it called devclass_find_internal() with the parentname set to > "pci". This caused the "pci" devclass to set its dc->parent member to > point to itself. Thus, if in device_probe_child() we didn't find a > suitable driver in the first pass of the for loop, we'd keep looping > forever and hang during boot. > > This is the fix I'm currently using but I was curious about feedback on it. > It only checks to make sure the a devclass doesn't add itself as a parent, > it doesn't walk up hierarchy to avoid a cycle completely: > > --- //depot/vendor/freebsd/src/sys/kern/subr_bus.c 2005/10/04 22:25:30 > +++ //depot/user/jhb/acpipci/kern/subr_bus.c 2006/01/04 21:32:26 > @@ -781,7 +781,17 @@ > > bus_data_generation_update(); > } > - if (parentname && dc && !dc->parent) { > + > + /* > + * If a parent class is specified, then set that as our parent so > + * that this devclass will support drivers for the parent class as > + * well. If the parent class has the same name don't do this though > + * as it creates a cycle that can trigger an infinite loop in > + * device_probe_child() if a device exists for which there is no > + * suitable driver. > + */ > + if (parentname && dc && !dc->parent && > + strcmp(classname, parentname) != 0) { > dc->parent = devclass_find_internal(parentname, 0, FALSE); > } > > @@ -1240,6 +1250,7 @@ > void > devclass_set_parent(devclass_t dc, devclass_t pdc) > { > + KASSERT(dc != pdc, ("%s: loop", __func__)); > dc->parent = pdc; > } > > The other possible direction is to rename the subclasses to not use the > same name ("pci"), but doing that actually involves a fair bit of work as > it means teaching the various pcib drivers to create acpi_pci child devices > rather than pci child devices, etc. Comments, flames, etc.? -- John Baldwin <>< http://www.baldwin.cx/~john/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org From owner-freebsd-new-bus@FreeBSD.ORG Tue Jan 17 19:27:10 2006 Return-Path: X-Original-To: freebsd-new-bus@freebsd.org Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 9FCC116A41F; Tue, 17 Jan 2006 19:27:10 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 041D843D46; Tue, 17 Jan 2006 19:27:09 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (localhost.village.org [127.0.0.1] (may be forged)) by harmony.bsdimp.com (8.13.3/8.13.3) with ESMTP id k0HJPkna007324; Tue, 17 Jan 2006 12:25:46 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Tue, 17 Jan 2006 12:25:46 -0700 (MST) Message-Id: <20060117.122546.26361238.imp@bsdimp.com> To: john@baldwin.cx From: Warner Losh In-Reply-To: <200601171308.41539.john@baldwin.cx> References: <200601061401.57308.john@baldwin.cx> <200601171308.41539.john@baldwin.cx> X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Tue, 17 Jan 2006 12:25:46 -0700 (MST) Cc: dfr@freebsd.org, freebsd-new-bus@freebsd.org Subject: Re: Patch to prevent cycles in the devclass tree X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2006 19:27:10 -0000 From: John Baldwin Subject: Re: Patch to prevent cycles in the devclass tree Date: Tue, 17 Jan 2006 13:08:40 -0500 > On Friday 06 January 2006 14:01, John Baldwin wrote: > > I have been doing some work recently to make the ACPI and OFW > > (OpenFirmware) PCI bus drivers inherit from the PCI bus driver. As part of > > this I've run into an issue because the subclass drivers (ACPI PCI and OFW > > PCI) use the same name "pci" as the superclass. They do this so that they > > can override the superclass when a "pci" device is added as a child of a > > "pcib" device. The problem I ran into is that when the subclass was > > registered, it called devclass_find_internal() with the parentname set to > > "pci". This caused the "pci" devclass to set its dc->parent member to > > point to itself. Thus, if in device_probe_child() we didn't find a > > suitable driver in the first pass of the for loop, we'd keep looping > > forever and hang during boot. > > > > This is the fix I'm currently using but I was curious about feedback on it. > > It only checks to make sure the a devclass doesn't add itself as a parent, > > it doesn't walk up hierarchy to avoid a cycle completely: > > > > --- //depot/vendor/freebsd/src/sys/kern/subr_bus.c 2005/10/04 22:25:30 > > +++ //depot/user/jhb/acpipci/kern/subr_bus.c 2006/01/04 21:32:26 > > @@ -781,7 +781,17 @@ > > > > bus_data_generation_update(); > > } > > - if (parentname && dc && !dc->parent) { > > + > > + /* > > + * If a parent class is specified, then set that as our parent so > > + * that this devclass will support drivers for the parent class as > > + * well. If the parent class has the same name don't do this though > > + * as it creates a cycle that can trigger an infinite loop in > > + * device_probe_child() if a device exists for which there is no > > + * suitable driver. > > + */ > > + if (parentname && dc && !dc->parent && > > + strcmp(classname, parentname) != 0) { > > dc->parent = devclass_find_internal(parentname, 0, FALSE); > > } > > > > @@ -1240,6 +1250,7 @@ > > void > > devclass_set_parent(devclass_t dc, devclass_t pdc) > > { > > + KASSERT(dc != pdc, ("%s: loop", __func__)); > > dc->parent = pdc; > > } > > > > The other possible direction is to rename the subclasses to not use the > > same name ("pci"), but doing that actually involves a fair bit of work as > > it means teaching the various pcib drivers to create acpi_pci child devices > > rather than pci child devices, etc. > > Comments, flames, etc.? I think this is right. Warner From owner-freebsd-new-bus@FreeBSD.ORG Tue Jan 17 20:17:27 2006 Return-Path: X-Original-To: freebsd-new-bus@freebsd.org Delivered-To: freebsd-new-bus@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 719B616A41F; Tue, 17 Jan 2006 20:17:27 +0000 (GMT) (envelope-from dfr@rabson.org) Received: from itchy.rabson.org (mailgate.nlsystems.com [80.177.232.242]) by mx1.FreeBSD.org (Postfix) with ESMTP id 76B4E43D45; Tue, 17 Jan 2006 20:17:24 +0000 (GMT) (envelope-from dfr@rabson.org) Received: from herring.rabson.org (herring.rabson.org [80.177.232.250]) by itchy.rabson.org (8.13.3/8.13.3) with ESMTP id k0HKH6e5095906; Tue, 17 Jan 2006 20:17:06 GMT (envelope-from dfr@rabson.org) From: Doug Rabson To: Warner Losh Date: Tue, 17 Jan 2006 20:17:04 +0000 User-Agent: KMail/1.8.3 References: <200601061401.57308.john@baldwin.cx> <200601171308.41539.john@baldwin.cx> <20060117.122546.26361238.imp@bsdimp.com> In-Reply-To: <20060117.122546.26361238.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200601172017.05525.dfr@rabson.org> X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=failed version=3.1.0 X-Spam-Checker-Version: SpamAssassin 3.1.0 (2005-09-13) on itchy.rabson.org X-Virus-Scanned: ClamAV 0.87.1/1244/Tue Jan 17 08:46:07 2006 on itchy.rabson.org X-Virus-Status: Clean Cc: john@baldwin.cx, dfr@freebsd.org, freebsd-new-bus@freebsd.org Subject: Re: Patch to prevent cycles in the devclass tree X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2006 20:17:27 -0000 On Tuesday 17 January 2006 19:25, Warner Losh wrote: > From: John Baldwin > Subject: Re: Patch to prevent cycles in the devclass tree > Date: Tue, 17 Jan 2006 13:08:40 -0500 > > > On Friday 06 January 2006 14:01, John Baldwin wrote: > > > I have been doing some work recently to make the ACPI and OFW > > > (OpenFirmware) PCI bus drivers inherit from the PCI bus driver. > > > As part of this I've run into an issue because the subclass > > > drivers (ACPI PCI and OFW PCI) use the same name "pci" as the > > > superclass. They do this so that they can override the > > > superclass when a "pci" device is added as a child of a "pcib" > > > device. The problem I ran into is that when the subclass was > > > registered, it called devclass_find_internal() with the > > > parentname set to "pci". This caused the "pci" devclass to set > > > its dc->parent member to point to itself. Thus, if in > > > device_probe_child() we didn't find a suitable driver in the > > > first pass of the for loop, we'd keep looping forever and hang > > > during boot. > > > > > > This is the fix I'm currently using but I was curious about > > > feedback on it. It only checks to make sure the a devclass > > > doesn't add itself as a parent, it doesn't walk up hierarchy to > > > avoid a cycle completely: > > > > > > --- //depot/vendor/freebsd/src/sys/kern/subr_bus.c 2005/10/04 > > > 22:25:30 +++ //depot/user/jhb/acpipci/kern/subr_bus.c 2006/01/04 > > > 21:32:26 @@ -781,7 +781,17 @@ > > > > > > bus_data_generation_update(); > > > } > > > - if (parentname && dc && !dc->parent) { > > > + > > > + /* > > > + * If a parent class is specified, then set that as our parent > > > so + * that this devclass will support drivers for the parent > > > class as + * well. If the parent class has the same name don't > > > do this though + * as it creates a cycle that can trigger an > > > infinite loop in + * device_probe_child() if a device exists for > > > which there is no + * suitable driver. > > > + */ > > > + if (parentname && dc && !dc->parent && > > > + strcmp(classname, parentname) != 0) { > > > dc->parent = devclass_find_internal(parentname, 0, FALSE); > > > } > > > > > > @@ -1240,6 +1250,7 @@ > > > void > > > devclass_set_parent(devclass_t dc, devclass_t pdc) > > > { > > > + KASSERT(dc != pdc, ("%s: loop", __func__)); > > > dc->parent = pdc; > > > } > > > > > > The other possible direction is to rename the subclasses to not > > > use the same name ("pci"), but doing that actually involves a > > > fair bit of work as it means teaching the various pcib drivers to > > > create acpi_pci child devices rather than pci child devices, etc. > > > > Comments, flames, etc.? > > I think this is right. I think so too.