Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 21 Jan 2010 03:06:52 +0000 (UTC)
From:      Navdeep Parhar <np@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r202733 - in stable/8/sys/dev/cxgb: . common
Message-ID:  <201001210306.o0L36qNH098712@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: np
Date: Thu Jan 21 03:06:52 2010
New Revision: 202733
URL: http://svn.freebsd.org/changeset/base/202733

Log:
  MFC r201907,202671,202678
  
  r201907:
  Extra parantheses to keep certain compilers happy.
  
  r202671:
  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.
  
  r202678:
  Complain if freelist queue sizes are significantly less than desired.

Modified:
  stable/8/sys/dev/cxgb/common/cxgb_t3_hw.c
  stable/8/sys/dev/cxgb/cxgb_adapter.h
  stable/8/sys/dev/cxgb/cxgb_main.c
  stable/8/sys/dev/cxgb/cxgb_sge.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)

Modified: stable/8/sys/dev/cxgb/common/cxgb_t3_hw.c
==============================================================================
--- stable/8/sys/dev/cxgb/common/cxgb_t3_hw.c	Thu Jan 21 02:21:31 2010	(r202732)
+++ stable/8/sys/dev/cxgb/common/cxgb_t3_hw.c	Thu Jan 21 03:06:52 2010	(r202733)
@@ -4443,7 +4443,7 @@ int __devinit t3_prep_adapter(adapter_t 
 
 	adapter->params.info = ai;
 	adapter->params.nports = ai->nports0 + ai->nports1;
-	adapter->params.chan_map = !!ai->nports0 | (!!ai->nports1 << 1);
+	adapter->params.chan_map = (!!ai->nports0) | (!!ai->nports1 << 1);
 	adapter->params.rev = t3_read_reg(adapter, A_PL_REV);
 
 	/*

Modified: stable/8/sys/dev/cxgb/cxgb_adapter.h
==============================================================================
--- stable/8/sys/dev/cxgb/cxgb_adapter.h	Thu Jan 21 02:21:31 2010	(r202732)
+++ stable/8/sys/dev/cxgb/cxgb_adapter.h	Thu Jan 21 03:06:52 2010	(r202733)
@@ -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: stable/8/sys/dev/cxgb/cxgb_main.c
==============================================================================
--- stable/8/sys/dev/cxgb/cxgb_main.c	Thu Jan 21 02:21:31 2010	(r202732)
+++ stable/8/sys/dev/cxgb/cxgb_main.c	Thu Jan 21 03:06:52 2010	(r202733)
@@ -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 *);
@@ -1113,7 +1111,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);
@@ -1133,7 +1138,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);
 }
 
@@ -1688,12 +1696,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;
@@ -1755,8 +1764,6 @@ out:
 static void
 cxgb_down(struct adapter *sc)
 {
-	ADAPTER_LOCK_ASSERT_NOTOWNED(sc);
-
 	t3_sge_stop(sc);
 	t3_intr_disable(sc);
 }
@@ -1767,8 +1774,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);
@@ -1803,120 +1808,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,
@@ -1924,6 +1864,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);
@@ -1947,7 +1892,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);
 }
 
 /*
@@ -1960,6 +1946,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
@@ -2036,19 +2027,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;
@@ -2058,35 +2051,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) {
@@ -2136,34 +2151,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);

Modified: stable/8/sys/dev/cxgb/cxgb_sge.c
==============================================================================
--- stable/8/sys/dev/cxgb/cxgb_sge.c	Thu Jan 21 02:21:31 2010	(r202732)
+++ stable/8/sys/dev/cxgb/cxgb_sge.c	Thu Jan 21 03:06:52 2010	(r202733)
@@ -541,8 +541,12 @@ t3_sge_prep(adapter_t *adap, struct sge_
 	jumbo_q_size = min(nmbjumbo4/(3*nqsets), JUMBO_Q_SIZE);
 #endif
 	while (!powerof2(jumbo_q_size))
-		jumbo_q_size--;		
-	
+		jumbo_q_size--;
+
+	if (fl_q_size < (FL_Q_SIZE / 4) || jumbo_q_size < (JUMBO_Q_SIZE / 2))
+		device_printf(adap->dev,
+		    "Insufficient clusters and/or jumbo buffers.\n");
+
 	/* XXX Does ETHER_ALIGN need to be accounted for here? */
 	p->max_pkt_size = adap->sge.qs[0].fl[1].buf_size - sizeof(struct cpl_rx_data);
 



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