Date: Tue, 17 Jan 2006 20:17:04 +0000 From: Doug Rabson <dfr@rabson.org> To: Warner Losh <imp@bsdimp.com> Cc: john@baldwin.cx, dfr@freebsd.org, freebsd-new-bus@freebsd.org Subject: Re: Patch to prevent cycles in the devclass tree Message-ID: <200601172017.05525.dfr@rabson.org> In-Reply-To: <20060117.122546.26361238.imp@bsdimp.com> References: <200601061401.57308.john@baldwin.cx> <200601171308.41539.john@baldwin.cx> <20060117.122546.26361238.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 17 January 2006 19:25, Warner Losh wrote: > From: John Baldwin <john@baldwin.cx> > 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200601172017.05525.dfr>