Date: Mon, 28 Feb 2011 21:06:30 +0100 From: Bernhard Schmidt <bschmidt@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-current@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: cardbus and kldunload issue Message-ID: <201102282106.30656.bschmidt@freebsd.org> In-Reply-To: <201102280837.31744.jhb@freebsd.org> References: <201102261625.41522.bschmidt@freebsd.org> <201102280837.31744.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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:
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)
> @@ -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);
--
Bernhard
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201102282106.30656.bschmidt>