From owner-freebsd-current@FreeBSD.ORG Mon Feb 28 13:38:43 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 408161065679; Mon, 28 Feb 2011 13:38:43 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id DD8148FC17; Mon, 28 Feb 2011 13:38:42 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 8D6F946B2C; Mon, 28 Feb 2011 08:38:42 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.10]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 299018A02A; Mon, 28 Feb 2011 08:38:42 -0500 (EST) From: John Baldwin To: freebsd-current@freebsd.org Date: Mon, 28 Feb 2011 08:37:31 -0500 User-Agent: KMail/1.13.5 (FreeBSD/7.4-CBSD-20110107; KDE/4.4.5; amd64; ; ) References: <201102261625.41522.bschmidt@freebsd.org> In-Reply-To: <201102261625.41522.bschmidt@freebsd.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201102280837.31744.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Mon, 28 Feb 2011 08:38:42 -0500 (EST) Cc: Warner Losh , Bernhard Schmidt Subject: Re: cardbus and kldunload issue X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Feb 2011 13:38:43 -0000 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: 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 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