Patch to prevent cycles in the devclass tree

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch to prevent cycles in the devclass tree

John Baldwin-2
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.

--
John Baldwin <[hidden email]>  <><  http://www.baldwin.cx/~john/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Patch to prevent cycles in the devclass tree

John Baldwin-2
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 <[hidden email]>  <><  http://www.baldwin.cx/~john/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Patch to prevent cycles in the devclass tree

Warner Losh
From: John Baldwin <[hidden email]>
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
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Patch to prevent cycles in the devclass tree

Doug Rabson-2
On Tuesday 17 January 2006 19:25, Warner Losh wrote:

> From: John Baldwin <[hidden email]>
> 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.
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-new-bus
To unsubscribe, send any mail to "[hidden email]"