Re: CVS commit: src/sys

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

> Date: Sat, 10 Aug 2024 21:37:45 +0000
> From: Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>
> 
> I suggest doing the same here (and in bge(4) and in vmx(4) and
> wherever else we have an overbroad `core lock'), with the attached
> patch.  But I don't have any awge(4) hardware to test.  Could I
> trouble you to test this and commit it if it works?

Oops -- missed a spot, need to take sc_mcast_lock around the call to
dwc_gmac_setmulti (and only around that call) in dwc_gmac_init.

diff -r f339ab4bf462 sys/dev/ic/dwc_gmac.c
--- a/sys/dev/ic/dwc_gmac.c	Sat Aug 10 15:20:59 2024 +0000
+++ b/sys/dev/ic/dwc_gmac.c	Sat Aug 10 21:48:14 2024 +0000
@@ -87,9 +87,7 @@ static void dwc_gmac_reset_tx_ring(struc
 static void dwc_gmac_free_tx_ring(struct dwc_gmac_softc *, struct dwc_gmac_tx_ring *);
 static void dwc_gmac_txdesc_sync(struct dwc_gmac_softc *, int, int, int);
 static int dwc_gmac_init(struct ifnet *);
-static int dwc_gmac_init_locked(struct ifnet *);
 static void dwc_gmac_stop(struct ifnet *, int);
-static void dwc_gmac_stop_locked(struct ifnet *, int);
 static void dwc_gmac_start(struct ifnet *);
 static void dwc_gmac_start_locked(struct ifnet *);
 static int dwc_gmac_queue(struct dwc_gmac_softc *, struct mbuf *);
@@ -297,7 +295,7 @@ dwc_gmac_attach(struct dwc_gmac_softc *s
 	sc->sc_stopping = false;
 	sc->sc_txbusy = false;
 
-	sc->sc_core_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NONE);
+	sc->sc_mcast_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_SOFTNET);
 	sc->sc_intr_lock = mutex_obj_alloc(MUTEX_DEFAULT, IPL_NET);
 	mutex_init(&sc->sc_txq.t_mtx, MUTEX_DEFAULT, IPL_NET);
 	mutex_init(&sc->sc_rxq.r_mtx, MUTEX_DEFAULT, IPL_NET);
@@ -863,28 +861,13 @@ static int
 dwc_gmac_init(struct ifnet *ifp)
 {
 	struct dwc_gmac_softc * const sc = ifp->if_softc;
-
-	KASSERT(IFNET_LOCKED(ifp));
-
-	mutex_enter(sc->sc_core_lock);
-	int ret = dwc_gmac_init_locked(ifp);
-	mutex_exit(sc->sc_core_lock);
-
-	return ret;
-}
-
-static int
-dwc_gmac_init_locked(struct ifnet *ifp)
-{
-	struct dwc_gmac_softc * const sc = ifp->if_softc;
 	uint32_t ffilt;
 
 	ASSERT_SLEEPABLE();
 	KASSERT(IFNET_LOCKED(ifp));
-	KASSERT(mutex_owned(sc->sc_core_lock));
 	KASSERT(ifp == &sc->sc_ec.ec_if);
 
-	dwc_gmac_stop_locked(ifp, 0);
+	dwc_gmac_stop(ifp, 0);
 
 	/*
 	 * Configure DMA burst/transfer mode and RX/TX priorities.
@@ -914,7 +897,9 @@ dwc_gmac_init_locked(struct ifnet *ifp)
 	/*
 	 * Set up multicast filter
 	 */
+	mutex_enter(sc->sc_mcast_lock);
 	dwc_gmac_setmulti(sc);
+	mutex_exit(sc->sc_mcast_lock);
 
 	/*
 	 * Set up dma pointer for RX and TX ring
@@ -942,11 +927,11 @@ dwc_gmac_init_locked(struct ifnet *ifp)
 
 	mutex_enter(sc->sc_intr_lock);
 	sc->sc_stopping = false;
+	mutex_exit(sc->sc_intr_lock);
 
 	mutex_enter(&sc->sc_txq.t_mtx);
 	sc->sc_txbusy = false;
 	mutex_exit(&sc->sc_txq.t_mtx);
-	mutex_exit(sc->sc_intr_lock);
 
 	return 0;
 }
@@ -1015,29 +1000,23 @@ dwc_gmac_stop(struct ifnet *ifp, int dis
 {
 	struct dwc_gmac_softc * const sc = ifp->if_softc;
 
-	mutex_enter(sc->sc_core_lock);
-	dwc_gmac_stop_locked(ifp, disable);
-	mutex_exit(sc->sc_core_lock);
-}
-
-static void
-dwc_gmac_stop_locked(struct ifnet *ifp, int disable)
-{
-	struct dwc_gmac_softc * const sc = ifp->if_softc;
-
 	ASSERT_SLEEPABLE();
 	KASSERT(IFNET_LOCKED(ifp));
-	KASSERT(mutex_owned(sc->sc_core_lock));
+
+	ifp->if_flags &= ~IFF_RUNNING;
+
+	mutex_enter(sc->sc_mcast_lock);
+	sc->sc_if_flags = ifp->if_flags;
+	mutex_exit(sc->sc_mcast_lock);
 
 	mutex_enter(sc->sc_intr_lock);
 	sc->sc_stopping = true;
+	mutex_exit(sc->sc_intr_lock);
 
 	mutex_enter(&sc->sc_txq.t_mtx);
 	sc->sc_txbusy = false;
 	mutex_exit(&sc->sc_txq.t_mtx);
 
-	mutex_exit(sc->sc_intr_lock);
-
 	bus_space_write_4(sc->sc_bst, sc->sc_bsh,
 	    AWIN_GMAC_DMA_OPMODE,
 	    bus_space_read_4(sc->sc_bst, sc->sc_bsh,
@@ -1051,9 +1030,6 @@ dwc_gmac_stop_locked(struct ifnet *ifp, 
 	mii_down(&sc->sc_mii);
 	dwc_gmac_reset_tx_ring(sc, &sc->sc_txq);
 	dwc_gmac_reset_rx_ring(sc, &sc->sc_rxq);
-
-	ifp->if_flags &= ~IFF_RUNNING;
-	sc->sc_if_flags = ifp->if_flags;
 }
 
 /*
@@ -1150,7 +1126,7 @@ dwc_gmac_ifflags_cb(struct ethercom *ec)
 	int ret = 0;
 
 	KASSERT(IFNET_LOCKED(ifp));
-	mutex_enter(sc->sc_core_lock);
+	mutex_enter(sc->sc_mcast_lock);
 
 	u_short change = ifp->if_flags ^ sc->sc_if_flags;
 	sc->sc_if_flags = ifp->if_flags;
@@ -1161,7 +1137,7 @@ dwc_gmac_ifflags_cb(struct ethercom *ec)
 		dwc_gmac_setmulti(sc);
 	}
 
-	mutex_exit(sc->sc_core_lock);
+	mutex_exit(sc->sc_mcast_lock);
 
 	return ret;
 }
@@ -1187,7 +1163,7 @@ dwc_gmac_ioctl(struct ifnet *ifp, u_long
 	if (error == ENETRESET) {
 		error = 0;
 		if (cmd == SIOCADDMULTI || cmd == SIOCDELMULTI) {
-			mutex_enter(sc->sc_core_lock);
+			mutex_enter(sc->sc_mcast_lock);
 			if (sc->sc_if_flags & IFF_RUNNING) {
 				/*
 				 * Multicast list has changed; set the hardware
@@ -1195,7 +1171,7 @@ dwc_gmac_ioctl(struct ifnet *ifp, u_long
 				 */
 				dwc_gmac_setmulti(sc);
 			}
-			mutex_exit(sc->sc_core_lock);
+			mutex_exit(sc->sc_mcast_lock);
 		}
 	}
 
@@ -1393,7 +1369,6 @@ skip:
 static void
 dwc_gmac_setmulti(struct dwc_gmac_softc *sc)
 {
-	struct ifnet * const ifp = &sc->sc_ec.ec_if;
 	struct ether_multi *enm;
 	struct ether_multistep step;
 	struct ethercom *ec = &sc->sc_ec;
@@ -1401,7 +1376,7 @@ dwc_gmac_setmulti(struct dwc_gmac_softc 
 	uint32_t ffilt, h;
 	int mcnt;
 
-	KASSERT(mutex_owned(sc->sc_core_lock));
+	KASSERT(mutex_owned(sc->sc_mcast_lock));
 
 	ffilt = bus_space_read_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_FFILT);
 
@@ -1463,7 +1438,6 @@ special_filter:
 	    0xffffffff);
 	bus_space_write_4(sc->sc_bst, sc->sc_bsh, AWIN_GMAC_MAC_HTHIGH,
 	    0xffffffff);
-	sc->sc_if_flags = ifp->if_flags;
 }
 
 int
diff -r f339ab4bf462 sys/dev/ic/dwc_gmac_var.h
--- a/sys/dev/ic/dwc_gmac_var.h	Sat Aug 10 15:20:59 2024 +0000
+++ b/sys/dev/ic/dwc_gmac_var.h	Sat Aug 10 21:48:14 2024 +0000
@@ -102,12 +102,12 @@ struct dwc_gmac_softc {
 	struct dwc_gmac_rx_ring sc_rxq;
 	struct dwc_gmac_tx_ring sc_txq;
 	const struct dwc_gmac_desc_methods *sc_descm;
-	u_short sc_if_flags;			/* shadow of ether flags */
+	u_short sc_if_flags;			/* if_flags copy (for mcast) */
 	uint16_t sc_mii_clk;
 	bool sc_txbusy;
 	bool sc_stopping;
 	krndsource_t rnd_source;
-	kmutex_t *sc_core_lock;			/* lock for softc operations */
+	kmutex_t *sc_mcast_lock;		/* lock for SIOCADD/DELMULTI */
 	kmutex_t *sc_intr_lock;			/* lock for interrupt operations */
 
 	struct if_percpuq *sc_ipq;		/* softint-based input queues */