very rough if_sk fixes

[Doug White]

Hey all,

I ran out and bought a Linksys EG1032 today and fixed up some of the
witness warnings and a detach panic.  I'm not 100% sure my lock relocation
is totally correct, though, so if someone wants to look at this and tell
me I have no idea what I'm doing, that'd be great. :-)  Suggestions
appreciated.  In particular I may need to tighten up the locking and
ordering in skc_detach() since I'm not sure if we can take an interrupt in
that context, and it'd be kinda bad to.

Most of the changes were trivial... too trivial maybe :-)

I'm also hoarding a copy of this patch at
http://people.freebsd.org/~dwhite/if_sk.c.diff if the attachment doesn't
come through.

Thanks!

-- 
Doug White                    |  FreeBSD: The Power to Serve
dwhite at gumbysoft.com          |  www.FreeBSD.org
-------------- next part --------------
--- if_sk.c.orig	Sun Jun 13 16:50:15 2004
+++ if_sk.c	Sun Jun 13 18:11:16 2004
@@ -1446,11 +1446,22 @@
 
 	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.
 	 */
@@ -1647,7 +1658,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) {
@@ -1685,10 +1696,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);
@@ -1709,7 +1732,12 @@
 
 	sc = device_get_softc(dev);
 	KASSERT(mtx_initialized(&sc->sk_mtx), ("sk mutex not initialized"));
+	/* XXX dwhite
+	 * Don't need the locks here?
+	 */
+	/*
 	SK_LOCK(sc);
+	*/
 
 	if (device_is_alive(dev)) {
 		if (sc->sk_devs[SK_PORT_A] != NULL)
@@ -1726,7 +1754,12 @@
 	if (sc->sk_res)
 		bus_release_resource(dev, SK_RES, SK_RID, sc->sk_res);
 
+	/* XXX dwhite
+	 * Don't need this?
+	 */
+	/*
 	SK_UNLOCK(sc);
+	*/
 	mtx_destroy(&sc->sk_mtx);
 
 	return(0);