Date: Tue, 17 Jan 2006 12:25:46 -0700 (MST) From: Warner Losh <imp@bsdimp.com> To: john@baldwin.cx Cc: dfr@freebsd.org, freebsd-new-bus@freebsd.org Subject: Re: Patch to prevent cycles in the devclass tree Message-ID: <20060117.122546.26361238.imp@bsdimp.com> In-Reply-To: <200601171308.41539.john@baldwin.cx> References: <200601061401.57308.john@baldwin.cx> <200601171308.41539.john@baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060117.122546.26361238.imp>