From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: bse: mp-safe genet_qstart()
To: tech@openbsd.org, kettenis@openbsd.org
Date: Tue, 6 Jan 2026 01:04:11 +0300

This diff follows the instructions from net/ifq.h The interface queues
layer takes care about serialization. To keep qenet_qstart() path
lockless I do atomic operations on sc_tx.queued shared between
genet_qstart() and genet_intr() to avoid mutex(9). It also rely on
atomic operations, but exclude parallel run of these handlers.

I do re-lock dances around barriers within genet_stop(). In one hand
they are not necessary because the netlock will not be taken in both
genet_qstart() and genet_intr() paths. In other hand, barriers under
exclusive netlock completely stop packets processing. Some mp-safe
drivers follow the first way, some follow the second.

Tested for two days on couple RPI4 under load and continuous interface
up/down sequence.

Index: sys/dev/ic/bcmgenet.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/bcmgenet.c,v
retrieving revision 1.10
diff -u -p -r1.10 bcmgenet.c
--- sys/dev/ic/bcmgenet.c	5 Jan 2026 11:49:01 -0000	1.10
+++ sys/dev/ic/bcmgenet.c	5 Jan 2026 21:33:33 -0000
@@ -195,7 +195,7 @@ genet_setup_txdesc(struct genet_softc *s
 	uint32_t status;
 
 	status = flags | __SHIFTIN(len, GENET_TX_DESC_STATUS_BUFLEN);
-	++sc->sc_tx.queued;
+	atomic_inc_int(&sc->sc_tx.queued);
 
 	WR4(sc, GENET_TX_DESC_ADDRESS_LO(index), (uint32_t)paddr);
 	WR4(sc, GENET_TX_DESC_ADDRESS_HI(index), (uint32_t)(paddr >> 32));
@@ -234,7 +234,8 @@ genet_setup_txbuf(struct genet_softc *sc
 	segs = sc->sc_tx.buf_map[index].map->dm_segs;
 	nsegs = sc->sc_tx.buf_map[index].map->dm_nsegs;
 
-	if (sc->sc_tx.queued >= GENET_DMA_DESC_COUNT - nsegs) {
+	if (atomic_load_int(&sc->sc_tx.queued) >=
+	    GENET_DMA_DESC_COUNT - nsegs) {
 		bus_dmamap_unload(sc->sc_tx.buf_tag,
 		    sc->sc_tx.buf_map[index].map);
 		return -1;
@@ -657,8 +658,13 @@ genet_stop(struct genet_softc *sc)
 	ifp->if_flags &= ~IFF_RUNNING;
 	ifq_clr_oactive(&ifp->if_snd);
 
+	NET_UNLOCK();
+
+	ifq_barrier(&ifp->if_snd);
 	intr_barrier(sc->sc_ih);
 
+	NET_LOCK();
+
 	/* Clean RX ring. */
 	for (i = 0; i < RX_DESC_COUNT; i++) {
 		bmap = &sc->sc_rx.buf_map[i];
@@ -749,14 +755,18 @@ genet_txintr(struct genet_softc *sc, int
 {
 	struct ifnet *ifp = &sc->sc_ac.ac_if;
 	struct genet_bufmap *bmap;
-	uint32_t cidx, total;
+	uint32_t cidx, total, queued;
 	int i;
 
 	cidx = RD4(sc, GENET_TX_DMA_CONS_INDEX(qid)) & 0xffff;
 	total = (cidx - sc->sc_tx.cidx) & 0xffff;
 
-	for (i = sc->sc_tx.next; sc->sc_tx.queued > 0 && total > 0;
-	     i = TX_NEXT(i), total--) {
+	if ((queued = atomic_load_int(&sc->sc_tx.queued)) < total) {
+		total = queued;
+	}
+	atomic_sub_int(&sc->sc_tx.queued, total);
+
+	for (i = sc->sc_tx.next; total > 0; i = TX_NEXT(i), total--) {
 		/* XXX check for errors */
 
 		bmap = &sc->sc_tx.buf_map[i];
@@ -768,8 +778,6 @@ genet_txintr(struct genet_softc *sc, int
 			m_freem(bmap->mbuf);
 			bmap->mbuf = NULL;
 		}
-
-		--sc->sc_tx.queued;
 	}
 
 	if (sc->sc_tx.cidx != cidx) {
@@ -782,13 +790,19 @@ genet_txintr(struct genet_softc *sc, int
 }
 
 void
-genet_start(struct ifnet *ifp)
+genet_qstart(struct ifqueue *ifq)
 {
+	struct ifnet *ifp = ifq->ifq_if;
 	struct genet_softc *sc = ifp->if_softc;
 	struct mbuf *m;
 	const int qid = GENET_DMA_DEFAULT_QUEUE;
 	int nsegs, index, cnt;
 
+	if (!LINK_STATE_IS_UP(ifp->if_link_state)) {
+		ifq_purge(ifq);
+		return;
+	}
+
 	index = sc->sc_tx.pidx & (TX_DESC_COUNT - 1);
 	cnt = 0;
 
@@ -860,6 +874,9 @@ genet_ioctl(struct ifnet *ifp, u_long cm
 		ifp->if_flags |= IFF_UP;
 		/* FALLTHROUGH */
 	case SIOCSIFFLAGS:
+		NET_UNLOCK();
+		rw_enter_write(&sc->sc_lock);
+		NET_LOCK();
 		if (ifp->if_flags & IFF_UP) {
 			if (ifp->if_flags & IFF_RUNNING)
 				error = ENETRESET;
@@ -869,6 +886,7 @@ genet_ioctl(struct ifnet *ifp, u_long cm
 			if (ifp->if_flags & IFF_RUNNING)
 				genet_stop(sc);
 		}
+		rw_exit_write(&sc->sc_lock);
 		break;
 
 	case SIOCGIFMEDIA:
@@ -964,6 +982,8 @@ genet_attach(struct genet_softc *sc)
 		return EINVAL;
 	}
 
+	rw_init(&sc->sc_lock, "genetlk");
+
 	timeout_set(&sc->sc_stat_ch, genet_tick, sc);
 	timeout_set(&sc->sc_rxto, genet_rxtick, sc);
 
@@ -971,7 +991,8 @@ genet_attach(struct genet_softc *sc)
 	ifp->if_softc = sc;
 	snprintf(ifp->if_xname, IFNAMSIZ, "%s", sc->sc_dev.dv_xname);
 	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
-	ifp->if_start = genet_start;
+	ifp->if_xflags = IFXF_MPSAFE;
+	ifp->if_qstart = genet_qstart;
 	ifp->if_ioctl = genet_ioctl;
 	ifq_init_maxlen(&ifp->if_snd, IFQ_MAXLEN);
 
Index: sys/dev/ic/bcmgenetvar.h
===================================================================
RCS file: /cvs/src/sys/dev/ic/bcmgenetvar.h,v
retrieving revision 1.1
diff -u -p -r1.1 bcmgenetvar.h
--- sys/dev/ic/bcmgenetvar.h	14 Apr 2020 21:02:39 -0000	1.1
+++ sys/dev/ic/bcmgenetvar.h	5 Jan 2026 21:33:33 -0000
@@ -69,6 +69,7 @@ struct genet_softc {
 #define sc_lladdr	sc_ac.ac_enaddr
 	struct mii_data		sc_mii;
 	struct timeout		sc_stat_ch;
+	struct rwlock		sc_lock;
 
 	struct genet_ring	sc_tx;
 	struct genet_ring	sc_rx;