Re: cardbus and kldunload issue

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_at_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_at_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_at_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)
_at__at_ -987,11 +987,13 _at__at_ 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...
+ *
  * _at_param dc		the devclass to edit
  * _at_param driver	the driver that was just added
  */
_at__at_ -1085,6 +1087,78 _at__at_ devclass_add_driver(devclass_t dc, driver_t *drive
 }
 
 /**
+ * _at_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...
+ *
+ * _at_param busclass	the devclass of the parent bus
+ * _at_param dc		the devclass of the driver being deleted
+ * _at_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);
+}
+
+/**
  * _at_brief Delete a device driver from a device class
  *
  * Delete a device driver from a devclass. This is normally called
_at__at_ -1103,8 +1177,6 _at__at_ 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)));
_at__at_ -1126,30 +1198,9 _at__at_ 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