> Module Name: src
> Committed By: skrll
> Date: Sat Aug 10 12:16:47 UTC 2024
>
> Modified Files:
> src/sys/arch/arm/altera: cycv_gmac.c
> src/sys/arch/arm/amlogic: meson_dwmac.c
> src/sys/arch/arm/rockchip: rk_gmac.c
> src/sys/arch/arm/sunxi: sunxi_gmac.c
> src/sys/dev/ic: dwc_gmac.c dwc_gmac_var.h
>
> Log Message:
> awge(4): MP improvements
>
> Remove the non-MP-safe scaffolding and make the locking less
> coarse.
It's great to see more MP-safety and removal of conditional-NET_MPSAFE
scaffolding -- but please, no more all-encompassing `core locks'!
The usage model for a network interface is:
1. (under IFNET_LOCK) if_init:
(a) resets hardware, while nothing else is touching registers
(b) programs multicast filter
(c) starts Tx/Rx and tick for mii/watchdog
(d) sets IFF_RUNNING
2. concurrently:
- (mii lock) periodic mii ticks or (with IFNET_LOCK too) ifmedia ioctls
- (tx lock) Tx submissions
- (rx lock) Rx notifications
- (multicast filter lock) SIOCADDMULTI/SIOCDELMULTI
- (ic lock) 802.11 state machine notifications
- (IFNET_LOCK) maybe other ioctls (some of which return ENETRESET
to cause an if_stop/init cycle in thread context)
3. (under IFNET_LOCK) if_stop:
(a) clears IFF_RUNNING
(b) halts Tx/Rx/tick and waits for completion
(c) disables concurrent multicast updates
(d) calls mii_down to wait for concurrent mii activity to quiesce
(e) resets hardware, now that nothing is touching registers any more
All of the steps in (1) if_init, and all of the steps in (3) if_stop,
are already serialized by IFNET_LOCK, the heavyweight configuration
lock, in low-priority thread context. Sometimes, while (2) running,
ifconfig(8) will reconfigure things, also serialized by IFNET_LOCK.
So there's _no need_ for a `core lock' to cover everything in if_init
and if_stop: IFNET_LOCK already takes care of that.
Any high-priority activity like Tx/Rx paths, callouts, &c., MUST NOT
take IFNET_LOCK or wait for the completion of the heavyweight
configuration changes, which often have to wait for concurrent
high-priority activity to cease -- such waits lead to deadlocks like
<https://mail-index.netbsd.org/tech-net/2022/01/10/msg008170.html>.
So it's _harmful_ to have a `core lock' cover everything in if_init
and if_stop.
In usbnet(9), I broke the deadlock by eliminating the core lock and
replacing it by a narrowly scoped lock covering only the state needed
by multicast filter updates, which happen from high-priority contexts.
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?
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:19:37 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.
@@ -942,11 +925,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 +998,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 +1028,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 +1124,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 +1135,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 +1161,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 +1169,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 +1367,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 +1374,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 +1436,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:19:37 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 */
