From owner-svn-src-all@FreeBSD.ORG Wed Jan 20 03:40:44 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 09EA31065679; Wed, 20 Jan 2010 03:40:44 +0000 (UTC) (envelope-from np@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id ECF5C8FC12; Wed, 20 Jan 2010 03:40:43 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id o0K3ehcK095009; Wed, 20 Jan 2010 03:40:43 GMT (envelope-from np@svn.freebsd.org) Received: (from np@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id o0K3ehsG095007; Wed, 20 Jan 2010 03:40:43 GMT (envelope-from np@svn.freebsd.org) Message-Id: <201001200340.o0K3ehsG095007@svn.freebsd.org> From: Navdeep Parhar Date: Wed, 20 Jan 2010 03:40:43 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r202671 - head/sys/dev/cxgb X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jan 2010 03:40:44 -0000 Author: np Date: Wed Jan 20 03:40:43 2010 New Revision: 202671 URL: http://svn.freebsd.org/changeset/base/202671 Log: Fix for a cxgb(4) panic. cxgb_ioctl can be called by the IP and IPv6 layers with non-sleepable locks held. Don't (potentially) sleep in those situations. Modified: head/sys/dev/cxgb/cxgb_adapter.h head/sys/dev/cxgb/cxgb_main.c Modified: head/sys/dev/cxgb/cxgb_adapter.h ============================================================================== --- head/sys/dev/cxgb/cxgb_adapter.h Wed Jan 20 01:14:54 2010 (r202670) +++ head/sys/dev/cxgb/cxgb_adapter.h Wed Jan 20 03:40:43 2010 (r202671) @@ -136,7 +136,6 @@ enum { }; #define IS_DOOMED(p) (p->flags & DOOMED) #define SET_DOOMED(p) do {p->flags |= DOOMED;} while (0) -#define DOOMED(p) (p->flags & DOOMED) #define IS_BUSY(sc) (sc->flags & CXGB_BUSY) #define SET_BUSY(sc) do {sc->flags |= CXGB_BUSY;} while (0) #define CLR_BUSY(sc) do {sc->flags &= ~CXGB_BUSY;} while (0) Modified: head/sys/dev/cxgb/cxgb_main.c ============================================================================== --- head/sys/dev/cxgb/cxgb_main.c Wed Jan 20 01:14:54 2010 (r202670) +++ head/sys/dev/cxgb/cxgb_main.c Wed Jan 20 03:40:43 2010 (r202671) @@ -84,11 +84,9 @@ __FBSDID("$FreeBSD$"); static int cxgb_setup_interrupts(adapter_t *); static void cxgb_teardown_interrupts(adapter_t *); -static int cxgb_begin_op(struct port_info *, const char *); -static int cxgb_begin_detach(struct port_info *); -static int cxgb_end_op(struct port_info *); static void cxgb_init(void *); -static int cxgb_init_synchronized(struct port_info *); +static int cxgb_init_locked(struct port_info *); +static int cxgb_uninit_locked(struct port_info *); static int cxgb_uninit_synchronized(struct port_info *); static int cxgb_ioctl(struct ifnet *, unsigned long, caddr_t); static int cxgb_media_change(struct ifnet *); @@ -1109,7 +1107,14 @@ cxgb_port_detach(device_t dev) p = device_get_softc(dev); sc = p->adapter; - cxgb_begin_detach(p); + /* Tell cxgb_ioctl and if_init that the port is going away */ + ADAPTER_LOCK(sc); + SET_DOOMED(p); + wakeup(&sc->flags); + while (IS_BUSY(sc)) + mtx_sleep(&sc->flags, &sc->lock, 0, "cxgbdtch", 0); + SET_BUSY(sc); + ADAPTER_UNLOCK(sc); if (p->port_cdev != NULL) destroy_dev(p->port_cdev); @@ -1129,7 +1134,10 @@ cxgb_port_detach(device_t dev) if_free(p->ifp); p->ifp = NULL; - cxgb_end_op(p); + ADAPTER_LOCK(sc); + CLR_BUSY(sc); + wakeup_one(&sc->flags); + ADAPTER_UNLOCK(sc); return (0); } @@ -1684,12 +1692,13 @@ cxgb_up(struct adapter *sc) { int err = 0; - ADAPTER_LOCK_ASSERT_NOTOWNED(sc); KASSERT(sc->open_device_map == 0, ("%s: device(s) already open (%x)", __func__, sc->open_device_map)); if ((sc->flags & FULL_INIT_DONE) == 0) { + ADAPTER_LOCK_ASSERT_NOTOWNED(sc); + if ((sc->flags & FW_UPTODATE) == 0) if ((err = upgrade_fw(sc))) goto out; @@ -1751,8 +1760,6 @@ out: static void cxgb_down(struct adapter *sc) { - ADAPTER_LOCK_ASSERT_NOTOWNED(sc); - t3_sge_stop(sc); t3_intr_disable(sc); } @@ -1763,8 +1770,6 @@ offload_open(struct port_info *pi) struct adapter *sc = pi->adapter; struct t3cdev *tdev = &sc->tdev; - ADAPTER_LOCK_ASSERT_NOTOWNED(sc); - setbit(&sc->open_device_map, OFFLOAD_DEVMAP_BIT); t3_tp_set_offload_mode(sc, 1); @@ -1799,120 +1804,55 @@ offload_close(struct t3cdev *tdev) } /* - * Begin a synchronized operation. If this call succeeds, it is guaranteed that - * no one will remove the port or its ifp from underneath the caller. Caller is - * also granted exclusive access to open_device_map. - * - * operation here means init, uninit, detach, and ioctl service. - * - * May fail. - * EINTR (ctrl-c pressed during ifconfig for example). - * ENXIO (port is about to detach - due to kldunload for example). + * if_init for cxgb ports. */ -int -cxgb_begin_op(struct port_info *p, const char *wmsg) +static void +cxgb_init(void *arg) { - int rc = 0; + struct port_info *p = arg; struct adapter *sc = p->adapter; ADAPTER_LOCK(sc); + cxgb_init_locked(p); /* releases adapter lock */ + ADAPTER_LOCK_ASSERT_NOTOWNED(sc); +} + +static int +cxgb_init_locked(struct port_info *p) +{ + struct adapter *sc = p->adapter; + struct ifnet *ifp = p->ifp; + struct cmac *mac = &p->mac; + int i, rc = 0, may_sleep = 0; + + ADAPTER_LOCK_ASSERT_OWNED(sc); while (!IS_DOOMED(p) && IS_BUSY(sc)) { - if (mtx_sleep(&sc->flags, &sc->lock, PCATCH, wmsg, 0)) { + if (mtx_sleep(&sc->flags, &sc->lock, PCATCH, "cxgbinit", 0)) { rc = EINTR; goto done; } } - - if (IS_DOOMED(p)) + if (IS_DOOMED(p)) { rc = ENXIO; - else if (!IS_BUSY(sc)) - SET_BUSY(sc); - else { - KASSERT(0, ("%s: port %d, p->flags = %x , sc->flags = %x", - __func__, p->port_id, p->flags, sc->flags)); - rc = EDOOFUS; + goto done; } - -done: - ADAPTER_UNLOCK(sc); - return (rc); -} - -/* - * End a synchronized operation. Read comment block above cxgb_begin_op. - */ -int -cxgb_end_op(struct port_info *p) -{ - struct adapter *sc = p->adapter; - - ADAPTER_LOCK(sc); - KASSERT(IS_BUSY(sc), ("%s: not busy.", __func__)); - CLR_BUSY(sc); - wakeup_one(&sc->flags); - ADAPTER_UNLOCK(sc); - - return (0); -} - -/* - * Prepare for port detachment. Detach is a special kind of synchronized - * operation. Also read comment before cxgb_begin_op. - */ -static int -cxgb_begin_detach(struct port_info *p) -{ - struct adapter *sc = p->adapter; + KASSERT(!IS_BUSY(sc), ("%s: controller busy.", __func__)); /* - * Inform those waiting for this port that it is going to be destroyed - * and they should not continue further. (They'll return with ENXIO). + * The code that runs during one-time adapter initialization can sleep + * so it's important not to hold any locks across it. */ - ADAPTER_LOCK(sc); - SET_DOOMED(p); - wakeup(&sc->flags); - ADAPTER_UNLOCK(sc); + may_sleep = sc->flags & FULL_INIT_DONE ? 0 : 1; - /* - * Wait for in-progress operations. - */ - ADAPTER_LOCK(sc); - while (IS_BUSY(sc)) { - mtx_sleep(&sc->flags, &sc->lock, 0, "cxgbdtch", 0); + if (may_sleep) { + SET_BUSY(sc); + ADAPTER_UNLOCK(sc); } - SET_BUSY(sc); - ADAPTER_UNLOCK(sc); - - return (0); -} - -/* - * if_init for cxgb ports. - */ -static void -cxgb_init(void *arg) -{ - struct port_info *p = arg; - - if (cxgb_begin_op(p, "cxgbinit")) - return; - - cxgb_init_synchronized(p); - cxgb_end_op(p); -} - -static int -cxgb_init_synchronized(struct port_info *p) -{ - struct adapter *sc = p->adapter; - struct ifnet *ifp = p->ifp; - struct cmac *mac = &p->mac; - int i, rc; if (sc->open_device_map == 0) { if ((rc = cxgb_up(sc)) != 0) - return (rc); + goto done; if (is_offload(sc) && !ofld_disable && offload_open(p)) log(LOG_WARNING, @@ -1920,6 +1860,11 @@ cxgb_init_synchronized(struct port_info } PORT_LOCK(p); + if (isset(&sc->open_device_map, p->port_id) && + (ifp->if_drv_flags & IFF_DRV_RUNNING)) { + PORT_UNLOCK(p); + goto done; + } t3_port_intr_enable(sc, p->port_id); if (!mac->multiport) t3_mac_init(mac); @@ -1943,7 +1888,48 @@ cxgb_init_synchronized(struct port_info /* all ok */ setbit(&sc->open_device_map, p->port_id); - return (0); +done: + if (may_sleep) { + ADAPTER_LOCK(sc); + KASSERT(IS_BUSY(sc), ("%s: controller not busy.", __func__)); + CLR_BUSY(sc); + wakeup_one(&sc->flags); + } + ADAPTER_UNLOCK(sc); + return (rc); +} + +static int +cxgb_uninit_locked(struct port_info *p) +{ + struct adapter *sc = p->adapter; + int rc; + + ADAPTER_LOCK_ASSERT_OWNED(sc); + + while (!IS_DOOMED(p) && IS_BUSY(sc)) { + if (mtx_sleep(&sc->flags, &sc->lock, PCATCH, "cxgbunin", 0)) { + rc = EINTR; + goto done; + } + } + if (IS_DOOMED(p)) { + rc = ENXIO; + goto done; + } + KASSERT(!IS_BUSY(sc), ("%s: controller busy.", __func__)); + SET_BUSY(sc); + ADAPTER_UNLOCK(sc); + + rc = cxgb_uninit_synchronized(p); + + ADAPTER_LOCK(sc); + KASSERT(IS_BUSY(sc), ("%s: controller not busy.", __func__)); + CLR_BUSY(sc); + wakeup_one(&sc->flags); +done: + ADAPTER_UNLOCK(sc); + return (rc); } /* @@ -1956,6 +1942,11 @@ cxgb_uninit_synchronized(struct port_inf struct ifnet *ifp = pi->ifp; /* + * taskqueue_drain may cause a deadlock if the adapter lock is held. + */ + ADAPTER_LOCK_ASSERT_NOTOWNED(sc); + + /* * Clear this port's bit from the open device map, and then drain all * the tasks that can access/manipulate this port's port_info or ifp. * We disable this port's interrupts here and so the the slow/ext @@ -2032,19 +2023,21 @@ static int cxgb_ioctl(struct ifnet *ifp, unsigned long command, caddr_t data) { struct port_info *p = ifp->if_softc; + struct adapter *sc = p->adapter; struct ifreq *ifr = (struct ifreq *)data; - int flags, error = 0, mtu, handle_unsynchronized = 0; + int flags, error = 0, mtu; uint32_t mask; - if ((error = cxgb_begin_op(p, "cxgbioct")) != 0) - return (error); - - /* - * Only commands that should be handled within begin-op/end-op are - * serviced in this switch statement. See handle_unsynchronized. - */ switch (command) { case SIOCSIFMTU: + ADAPTER_LOCK(sc); + error = IS_DOOMED(p) ? ENXIO : (IS_BUSY(sc) ? EBUSY : 0); + if (error) { +fail: + ADAPTER_UNLOCK(sc); + return (error); + } + mtu = ifr->ifr_mtu; if ((mtu < ETHERMIN) || (mtu > ETHERMTU_JUMBO)) { error = EINVAL; @@ -2054,35 +2047,57 @@ cxgb_ioctl(struct ifnet *ifp, unsigned l cxgb_update_mac_settings(p); PORT_UNLOCK(p); } - + ADAPTER_UNLOCK(sc); break; case SIOCSIFFLAGS: + ADAPTER_LOCK(sc); + if (IS_DOOMED(p)) { + error = ENXIO; + goto fail; + } if (ifp->if_flags & IFF_UP) { if (ifp->if_drv_flags & IFF_DRV_RUNNING) { flags = p->if_flags; if (((ifp->if_flags ^ flags) & IFF_PROMISC) || ((ifp->if_flags ^ flags) & IFF_ALLMULTI)) { + if (IS_BUSY(sc)) { + error = EBUSY; + goto fail; + } PORT_LOCK(p); cxgb_update_mac_settings(p); PORT_UNLOCK(p); } + ADAPTER_UNLOCK(sc); } else - error = cxgb_init_synchronized(p); + error = cxgb_init_locked(p); p->if_flags = ifp->if_flags; } else if (ifp->if_drv_flags & IFF_DRV_RUNNING) - error = cxgb_uninit_synchronized(p); - + error = cxgb_uninit_locked(p); + + ADAPTER_LOCK_ASSERT_NOTOWNED(sc); break; case SIOCADDMULTI: case SIOCDELMULTI: + ADAPTER_LOCK(sc); + error = IS_DOOMED(p) ? ENXIO : (IS_BUSY(sc) ? EBUSY : 0); + if (error) + goto fail; + if (ifp->if_drv_flags & IFF_DRV_RUNNING) { PORT_LOCK(p); cxgb_update_mac_settings(p); PORT_UNLOCK(p); } + ADAPTER_UNLOCK(sc); break; case SIOCSIFCAP: + ADAPTER_LOCK(sc); + error = IS_DOOMED(p) ? ENXIO : (IS_BUSY(sc) ? EBUSY : 0); + if (error) + goto fail; + mask = ifr->ifr_reqcap ^ ifp->if_capenable; if (mask & IFCAP_TXCSUM) { if (IFCAP_TXCSUM & ifp->if_capenable) { @@ -2132,34 +2147,20 @@ cxgb_ioctl(struct ifnet *ifp, unsigned l PORT_UNLOCK(p); } } - if (mask & IFCAP_VLAN_HWCSUM) { + if (mask & IFCAP_VLAN_HWCSUM) ifp->if_capenable ^= IFCAP_VLAN_HWCSUM; - } #ifdef VLAN_CAPABILITIES VLAN_CAPABILITIES(ifp); #endif + ADAPTER_UNLOCK(sc); break; - default: - handle_unsynchronized = 1; + case SIOCSIFMEDIA: + case SIOCGIFMEDIA: + error = ifmedia_ioctl(ifp, ifr, &p->media, command); break; - } - - /* - * We don't want to call anything outside the driver while inside a - * begin-op/end-op block. If it calls us back (eg. ether_ioctl may - * call cxgb_init) we may deadlock if the state is already marked busy. - * - * XXX: this probably opens a small race window with kldunload... - */ - cxgb_end_op(p); - - /* The IS_DOOMED check is racy, we're clutching at straws here */ - if (handle_unsynchronized && !IS_DOOMED(p)) { - if (command == SIOCSIFMEDIA || command == SIOCGIFMEDIA) - error = ifmedia_ioctl(ifp, ifr, &p->media, command); - else - error = ether_ioctl(ifp, command, data); + default: + error = ether_ioctl(ifp, command, data); } return (error);