new if_sk locking patch...

John-Mark Gurney gurney_j at resnet.uoregon.edu
Tue Aug 17 00:24:40 PDT 2004


Ok, I just happen to pick up a Belkin F5D5005 Gige card.  (I couldn't
resist at $25.)  They were nice enough to include Linux drivers on the
CD (yeh!) which made it much easier for me to find out what driver it
was compatible with.  It didn't take much time to get the card working.

Of course, once I had the card working I was getting soooo many LOR's
that my serial console was overloaded.  I took Doug White's patch and
modified it a little.  I left in the recursion since I wanted to get
it working first.

So, here it is attached, and availabe at:
http://people.freebsd.org/~jmg/if_sk.diff

I don't get any LOR's w/ this patch and debug.mpsafenet=1.  I didn't
try using it as a module though.  (Hence I don't know if kldload/kldunload
works.)

Test it out and let me know how it works for you.

Thanks!

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."
-------------- next part --------------
Index: if_sk.c
===================================================================
RCS file: /home/ncvs/src/sys/pci/if_sk.c,v
retrieving revision 1.83
diff -u -r1.83 if_sk.c
--- if_sk.c	2004/06/28 20:07:03	1.83
+++ if_sk.c	2004/08/17 07:13:23
@@ -156,6 +156,11 @@
 		"Marvell Gigabit Ethernet"
 	},
 	{
+		VENDORID_MARVELL,
+		DEVICEID_BELKIN_5005,
+		"Belkin F5D5005 Gigabit Ethernet"
+	},
+	{
 		VENDORID_3COM,
 		DEVICEID_3COM_3C940,
 		"3Com 3C940 Gigabit Ethernet"
@@ -1332,7 +1337,6 @@
 	error = 0;
 	sc_if = device_get_softc(dev);
 	sc = device_get_softc(device_get_parent(dev));
-	SK_LOCK(sc);
 	port = *(int *)device_get_ivars(dev);
 	free(device_get_ivars(dev), M_DEVBUF);
 	device_set_ivars(dev, NULL);
@@ -1347,6 +1351,40 @@
 	if (port == SK_PORT_B)
 		sc_if->sk_tx_bmu = SK_BMU_TXS_CSR1;
 
+	/* Allocate the descriptor queues. */
+	sc_if->sk_rdata = contigmalloc(sizeof(struct sk_ring_data), M_DEVBUF,
+	    M_NOWAIT, 0, 0xffffffff, PAGE_SIZE, 0);
+
+	if (sc_if->sk_rdata == NULL) {
+		printf("sk%d: no memory for list buffers!\n", sc_if->sk_unit);
+		error = ENOMEM;
+		goto fail;
+	}
+
+	bzero(sc_if->sk_rdata, sizeof(struct sk_ring_data));
+
+	/* Try to allocate memory for jumbo buffers. */
+	if (sk_alloc_jumbo_mem(sc_if)) {
+		printf("sk%d: jumbo buffer allocation failed\n",
+		    sc_if->sk_unit);
+		error = ENOMEM;
+		goto fail;
+	}
+
+	ifp = &sc_if->arpcom.ac_if;
+	ifp->if_softc = sc_if;
+	if_initname(ifp, device_get_name(dev), device_get_unit(dev));
+	ifp->if_mtu = ETHERMTU;
+	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
+	ifp->if_ioctl = sk_ioctl;
+	ifp->if_start = sk_start;
+	ifp->if_watchdog = sk_watchdog;
+	ifp->if_init = sk_init;
+	ifp->if_baudrate = 1000000000;
+	ifp->if_snd.ifq_maxlen = SK_TX_RING_CNT - 1;
+
+	callout_handle_init(&sc_if->sk_tick_ch);
+
 	/*
 	 * Get station address for this interface. Note that
 	 * dual port cards actually come with three station
@@ -1356,6 +1394,7 @@
 	 * are operating in failover mode. Currently we don't
 	 * use this extra address.
 	 */
+	SK_LOCK(sc);
 	for (i = 0; i < ETHER_ADDR_LEN; i++)
 		sc_if->arpcom.ac_enaddr[i] =
 		    sk_win_read_1(sc, SK_MAC0_0 + (port * 8) + i);
@@ -1409,48 +1448,26 @@
 		printf("skc%d: unsupported PHY type: %d\n",
 		    sc->sk_unit, sc_if->sk_phytype);
 		error = ENODEV;
-		goto fail;
-	}
-
-	/* Allocate the descriptor queues. */
-	sc_if->sk_rdata = contigmalloc(sizeof(struct sk_ring_data), M_DEVBUF,
-	    M_NOWAIT, 0, 0xffffffff, PAGE_SIZE, 0);
-
-	if (sc_if->sk_rdata == NULL) {
-		printf("sk%d: no memory for list buffers!\n", sc_if->sk_unit);
-		error = ENOMEM;
-		goto fail;
-	}
-
-	bzero(sc_if->sk_rdata, sizeof(struct sk_ring_data));
-
-	/* Try to allocate memory for jumbo buffers. */
-	if (sk_alloc_jumbo_mem(sc_if)) {
-		printf("sk%d: jumbo buffer allocation failed\n",
-		    sc_if->sk_unit);
-		error = ENOMEM;
+		SK_UNLOCK(sc);
 		goto fail;
 	}
 
-	ifp = &sc_if->arpcom.ac_if;
-	ifp->if_softc = sc_if;
-	if_initname(ifp, device_get_name(dev), device_get_unit(dev));
-	ifp->if_mtu = ETHERMTU;
-	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
-	ifp->if_ioctl = sk_ioctl;
-	ifp->if_start = sk_start;
-	ifp->if_watchdog = sk_watchdog;
-	ifp->if_init = sk_init;
-	ifp->if_baudrate = 1000000000;
-	ifp->if_snd.ifq_maxlen = SK_TX_RING_CNT - 1;
-
-	callout_handle_init(&sc_if->sk_tick_ch);
+	/* XXX dwhite
+	 * Drop interface lock around ether_ifattach, which
+	 * cannot be called with locks held.
+	 */
+	SK_UNLOCK(sc);
 
 	/*
 	 * Call MI attach routine.
 	 */
 	ether_ifattach(ifp, sc_if->arpcom.ac_enaddr);
 
+	/* XXX dwhite
+	 * Pick the lock back up.
+	 */
+	SK_LOCK(sc);
+
 	/*
 	 * Do miibus setup.
 	 */
@@ -1463,16 +1480,15 @@
 		break;
 	}
 
+	SK_UNLOCK(sc);
 	if (mii_phy_probe(dev, &sc_if->sk_miibus,
 	    sk_ifmedia_upd, sk_ifmedia_sts)) {
 		printf("skc%d: no PHY found!\n", sc_if->sk_unit);
 		ether_ifdetach(ifp);
 		error = ENXIO;
-		goto fail;
 	}
 
 fail:
-	SK_UNLOCK(sc);
 	if (error) {
 		/* Access should be ok even though lock has been dropped */
 		sc->sk_if[port] = NULL;
@@ -1532,6 +1548,7 @@
 		sc->sk_type = SK_GENESIS;
 		break;
 	case DEVICEID_SK_V2:
+	case DEVICEID_BELKIN_5005:
 	case DEVICEID_3COM_3C940:
 	case DEVICEID_LINKSYS_EG1032:
 	case DEVICEID_DLINK_DGE530T:
@@ -1623,7 +1640,7 @@
 	bus_generic_attach(dev);
 
 	/* Hook interrupt last to avoid having to lock softc */
-	error = bus_setup_intr(dev, sc->sk_irq, INTR_TYPE_NET,
+	error = bus_setup_intr(dev, sc->sk_irq, INTR_TYPE_NET|INTR_MPSAFE,
 	    sk_intr, sc, &sc->sk_intrhand);
 
 	if (error) {
@@ -1661,10 +1678,22 @@
 	/* These should only be active if attach_xmac succeeded */
 	if (device_is_attached(dev)) {
 		sk_stop(sc_if);
+		/* XXX dwhite
+		 * Can't hold locks while calling detach 
+		 */
+		SK_IF_UNLOCK(sc_if);
 		ether_ifdetach(ifp);
+		SK_IF_LOCK(sc_if);
 	}
+	/* XXX dwhite
+	 * We're generally called from skc_detach() which is using
+	 * device_delete_child() to get to here. It's already trashed
+	 * miibus for us, so don't do it here or we'll panic.
+	 */
+	/*
 	if (sc_if->sk_miibus)
 		device_delete_child(dev, sc_if->sk_miibus);
+	*/
 	bus_generic_detach(dev);
 	if (sc_if->sk_cdata.sk_jumbo_buf)
 		contigfree(sc_if->sk_cdata.sk_jumbo_buf, SK_JMEM, M_DEVBUF);
@@ -1685,7 +1714,6 @@
 
 	sc = device_get_softc(dev);
 	KASSERT(mtx_initialized(&sc->sk_mtx), ("sk mutex not initialized"));
-	SK_LOCK(sc);
 
 	if (device_is_alive(dev)) {
 		if (sc->sk_devs[SK_PORT_A] != NULL)
@@ -1702,7 +1730,6 @@
 	if (sc->sk_res)
 		bus_release_resource(dev, SK_RES, SK_RID, sc->sk_res);
 
-	SK_UNLOCK(sc);
 	mtx_destroy(&sc->sk_mtx);
 
 	return(0);
@@ -2357,7 +2384,8 @@
 	return;
 }
 
-static void sk_init_yukon(sc_if)
+static void
+sk_init_yukon(sc_if)
 	struct sk_if_softc	*sc_if;
 {
 	u_int32_t		phy;
Index: if_skreg.h
===================================================================
RCS file: /home/ncvs/src/sys/pci/if_skreg.h,v
retrieving revision 1.20
diff -u -r1.20 if_skreg.h
--- if_skreg.h	2004/03/31 12:35:51	1.20
+++ if_skreg.h	2004/08/17 07:13:24
@@ -71,6 +71,11 @@
 #define DEVICEID_SK_V2		0x4320
 
 /*
+ * Belkin F5D5005
+ */
+#define DEVICEID_BELKIN_5005	0x5005
+
+/*
  * 3Com PCI vendor ID
  */
 #define VENDORID_3COM		0x10b7
@@ -1436,6 +1441,7 @@
 #define	SK_LOCK_ASSERT(_sc)	mtx_assert(&(_sc)->sk_mtx, MA_OWNED)
 #define	SK_IF_LOCK(_sc)		mtx_lock(&(_sc)->sk_softc->sk_mtx)
 #define	SK_IF_UNLOCK(_sc)	mtx_unlock(&(_sc)->sk_softc->sk_mtx)
+#define	SK_IF_LOCK_ASSERT(_sc)	mtx_assert(&(_sc)->sk_mtx, MA_OWNED)
 
 /* Softc for each logical interface */
 struct sk_if_softc {


More information about the freebsd-current mailing list