Re: cardbus and kldunload issue

On Monday 28 February 2011 14:37:31 John Baldwin wrote:
> 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:

That fixes it, thanks! kldunload now successfully calls the driver's
detach method.

> 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);
 

-- 
Bernhard