Date: Mon, 28 Feb 2011 08:37:31 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-current@freebsd.org Cc: Warner Losh <imp@freebsd.org>, Bernhard Schmidt <bschmidt@freebsd.org> Subject: Re: cardbus and kldunload issue Message-ID: <201102280837.31744.jhb@freebsd.org> In-Reply-To: <201102261625.41522.bschmidt@freebsd.org> References: <201102261625.41522.bschmidt@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, February 26, 2011 10:25:41 am Bernhard Schmidt wrote:
> Hi,
>
> while working on a wireless driver for a Cardbus card I stumbled over
> an issue which bugs me quite a bit.
>
> The device:
>
> % none3@pci0:22:0:0: class=0x028000 card=0x107f1043 chip=0x02011814
rev=0x01 hdr=0x00
>
> Loading the module attaches nicely to the device:
>
> # kldload if_ral
> % ral0: <Ralink Technology RT2560> mem 0xf4800000-0xf4801fff irq 16 at
device 0.0 on cardbus0
> % ral0: MAC/BBP RT2560 (rev 0x04), RF RT2525
> % ral0: [ITHREAD]
> # pciconf -l
> % ral0@pci0:22:0:0: class=0x028000 card=0x107f1043 chip=0x02011814
rev=0x01 hdr=0x00
>
> Though, kldunload doesn't detach the device, it doesn't even call the
> module's detach function.
>
> # kldunload if_ral
> # pciconf -l
> % ral0@pci0:22:0:0: class=0x028000 card=0x107f1043 chip=0x02011814
rev=0x01 hdr=0x00
> # kldstat
> % Id Refs Address Size Name
> % 1 27 0xffffffff80100000 e640a0 kernel
> # ifconfig ral0
> % ral0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 2290
> % ether 00:0e:a6:a6:1b:70
> % media: IEEE 802.11 Wireless Ethernet autoselect (autoselect)
> % status: no carrier
>
> And of course trying to use the device at that point will result in
> instant panics. Playing around a bit I've noticed that changing the bus
> name in:
>
> % DRIVER_MODULE(ral, pci, ral_pci_driver, ral_devclass, 0, 0);
>
> from pci to cardbus makes a big difference. On module unload the detach
> function is then called as expected. So, question is, are we doing some
> too strict checks on module unload while matching the bus? Or is this
> expected behavior and the drivers are supposed to indiciated that they
> support both pci and cardbus? I don't see the later anywhere in tree.
This sounds like a bug in how the inheritance stuff in new-bus drivers works.
The DRIVER_MODULE() line for cardbus is implicit because the 'cardbus' driver
inherits from the generic 'pci' bus driver. That is how kldload works
correctly. It seems we need to do some extra plumbing for the kldunload case.
The bug is that 189574 only patched the devclass add driver path, not the
delete driver path. Try this:
Index: subr_bus.c
===================================================================
--- subr_bus.c (revision 219096)
+++ subr_bus.c (working copy)
@@ -987,11 +987,13 @@ devclass_find(const char *classname)
* is called by devclass_add_driver to accomplish the recursive
* notification of all the children classes of dc, as well as dc.
* Each layer will have BUS_DRIVER_ADDED() called for all instances of
- * the devclass. We do a full search here of the devclass list at
- * each iteration level to save storing children-lists in the devclass
- * structure. If we ever move beyond a few dozen devices doing this,
- * we may need to reevaluate...
+ * the devclass.
*
+ * We do a full search here of the devclass list at each iteration
+ * level to save storing children-lists in the devclass structure. If
+ * we ever move beyond a few dozen devices doing this, we may need to
+ * reevaluate...
+ *
* @param dc the devclass to edit
* @param driver the driver that was just added
*/
@@ -1085,6 +1087,78 @@ devclass_add_driver(devclass_t dc, driver_t *drive
}
/**
+ * @brief Register that a device driver has been deleted from a devclass
+ *
+ * Register that a device driver has been removed from a devclass.
+ * This is called by devclass_delete_driver to accomplish the
+ * recursive notification of all the children classes of busclass, as
+ * well as busclass. Each layer will attempt to detach the driver
+ * from any devices that are children of the bus's devclass. The function
+ * will return an error if a device fails to detach.
+ *
+ * We do a full search here of the devclass list at each iteration
+ * level to save storing children-lists in the devclass structure. If
+ * we ever move beyond a few dozen devices doing this, we may need to
+ * reevaluate...
+ *
+ * @param busclass the devclass of the parent bus
+ * @param dc the devclass of the driver being deleted
+ * @param driver the driver being deleted
+ */
+static int
+devclass_driver_deleted(devclass_t busclass, devclass_t dc, driver_t *driver)
+{
+ devclass_t parent;
+ device_t dev;
+ int error, i;
+
+ /*
+ * Disassociate from any devices. We iterate through all the
+ * devices in the devclass of the driver and detach any which are
+ * using the driver and which have a parent in the devclass which
+ * we are deleting from.
+ *
+ * Note that since a driver can be in multiple devclasses, we
+ * should not detach devices which are not children of devices in
+ * the affected devclass.
+ */
+ for (i = 0; i < dc->maxunit; i++) {
+ if (dc->devices[i]) {
+ dev = dc->devices[i];
+ if (dev->driver == driver && dev->parent &&
+ dev->parent->devclass == busclass) {
+ if ((error = device_detach(dev)) != 0)
+ return (error);
+ device_set_driver(dev, NULL);
+ BUS_PROBE_NOMATCH(dev->parent, dev);
+ devnomatch(dev);
+ dev->flags |= DF_DONENOMATCH;
+ }
+ }
+ }
+
+ /*
+ * Walk through the children classes. Since we only keep a
+ * single parent pointer around, we walk the entire list of
+ * devclasses looking for children. We set the
+ * DC_HAS_CHILDREN flag when a child devclass is created on
+ * the parent, so we only walk the list for those devclasses
+ * that have children.
+ */
+ if (!(busclass->flags & DC_HAS_CHILDREN))
+ return (0);
+ parent = busclass;
+ TAILQ_FOREACH(busclass, &devclasses, link) {
+ if (busclass->parent == parent) {
+ error = devclass_driver_deleted(busclass, dc, driver);
+ if (error)
+ return (error);
+ }
+ }
+ return (0);
+}
+
+/**
* @brief Delete a device driver from a device class
*
* Delete a device driver from a devclass. This is normally called
@@ -1103,8 +1177,6 @@ devclass_delete_driver(devclass_t busclass, driver
{
devclass_t dc = devclass_find(driver->name);
driverlink_t dl;
- device_t dev;
- int i;
int error;
PDEBUG(("%s from devclass %s", driver->name, DEVCLANAME(busclass)));
@@ -1126,30 +1198,9 @@ devclass_delete_driver(devclass_t busclass, driver
return (ENOENT);
}
- /*
- * Disassociate from any devices. We iterate through all the
- * devices in the devclass of the driver and detach any which are
- * using the driver and which have a parent in the devclass which
- * we are deleting from.
- *
- * Note that since a driver can be in multiple devclasses, we
- * should not detach devices which are not children of devices in
- * the affected devclass.
- */
- for (i = 0; i < dc->maxunit; i++) {
- if (dc->devices[i]) {
- dev = dc->devices[i];
- if (dev->driver == driver && dev->parent &&
- dev->parent->devclass == busclass) {
- if ((error = device_detach(dev)) != 0)
- return (error);
- device_set_driver(dev, NULL);
- BUS_PROBE_NOMATCH(dev->parent, dev);
- devnomatch(dev);
- dev->flags |= DF_DONENOMATCH;
- }
- }
- }
+ error = devclass_driver_deleted(busclass, dc, driver);
+ if (error != 0)
+ return (error);
TAILQ_REMOVE(&busclass->drivers, dl, link);
free(dl, M_BUS);
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201102280837.31744.jhb>