Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 20 Jan 2010 03:40:43 +0000 (UTC)
From:      Navdeep Parhar <np@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r202671 - head/sys/dev/cxgb
Message-ID:  <201001200340.o0K3ehsG095007@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
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);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001200340.o0K3ehsG095007>