Holding NET_LOCK() while invoking a taskq barrier can result in
a deadlock because the lock can prevent the taskq from making progress.
Avoid this problem in cad_down() by serializing the driver's ioctl
handler with an rwlock so that NET_LOCK() can be released temporarily.

The patch additionally makes cad_up() release NET_LOCK() when allocating
the Tx and Rx rings. There is a risk that the allocation blocks.
The code probably should allow allocation failure, but that I leave for
another patch.

OK?

Index: dev/fdt/if_cad.c
===================================================================
RCS file: src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.9
diff -u -p -r1.9 if_cad.c
--- dev/fdt/if_cad.c    27 Jan 2022 17:34:51 -0000      1.9
+++ dev/fdt/if_cad.c    2 Mar 2022 10:28:04 -0000
@@ -30,6 +30,7 @@
 #include <sys/ioctl.h>
 #include <sys/mutex.h>
 #include <sys/kstat.h>
+#include <sys/rwlock.h>
 #include <sys/task.h>
 #include <sys/timeout.h>
 
@@ -297,6 +298,7 @@ struct cad_softc {
        unsigned int            sc_rx_cons;
        uint32_t                sc_netctl;
 
+       struct rwlock           sc_cfg_lock;
        struct task             sc_statchg_task;
        uint32_t                sc_tx_freq;
 
@@ -467,10 +469,13 @@ cad_attach(struct device *parent, struct
        if (OF_is_compatible(faa->fa_node, "cdns,zynq-gem"))
                sc->sc_rxhang_erratum = 1;
 
+       rw_init(&sc->sc_cfg_lock, "cadcfg");
        timeout_set(&sc->sc_tick, cad_tick, sc);
        task_set(&sc->sc_statchg_task, cad_statchg_task, sc);
 
+       rw_enter_write(&sc->sc_cfg_lock);
        cad_reset(sc);
+       rw_exit_write(&sc->sc_cfg_lock);
 
        sc->sc_ih = fdt_intr_establish(faa->fa_node, IPL_NET | IPL_MPSAFE,
            cad_intr, sc, sc->sc_dev.dv_xname);
@@ -548,6 +553,9 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
        int error = 0;
        int s;
 
+       NET_UNLOCK();
+       rw_enter_write(&sc->sc_cfg_lock);
+       NET_LOCK();
        s = splnet();
 
        switch (cmd) {
@@ -585,6 +593,7 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
        }
 
        splx(s);
+       rw_exit_write(&sc->sc_cfg_lock);
 
        return error;
 }
@@ -598,6 +607,8 @@ cad_reset(struct cad_softc *sc)
        unsigned int freq, i;
        uint32_t div, netcfg;
 
+       rw_assert_wrlock(&sc->sc_cfg_lock);
+
        HWRITE4(sc, GEM_NETCTL, 0);
        HWRITE4(sc, GEM_IDR, ~0U);
        HWRITE4(sc, GEM_RXSR, 0);
@@ -655,6 +666,11 @@ cad_up(struct cad_softc *sc)
        unsigned int i, nrxd, ntxd;
        uint32_t val;
 
+       rw_assert_wrlock(&sc->sc_cfg_lock);
+
+       /* Release lock for memory allocation. */
+       NET_UNLOCK();
+
        if (sc->sc_dma64)
                flags |= BUS_DMA_64BIT;
 
@@ -810,6 +826,8 @@ cad_up(struct cad_softc *sc)
                }
        }
 
+       NET_LOCK();
+
        /*
         * Set MAC address filters.
         */
@@ -905,11 +923,16 @@ cad_down(struct cad_softc *sc)
        struct cad_buf *rxb, *txb;
        unsigned int i, timeout;
 
+       rw_assert_wrlock(&sc->sc_cfg_lock);
+
        ifp->if_flags &= ~IFF_RUNNING;
 
        ifq_clr_oactive(&ifp->if_snd);
        ifp->if_timer = 0;
 
+       /* Avoid lock order issues with barriers. */
+       NET_UNLOCK();
+
        timeout_del_barrier(&sc->sc_tick);
 
        /* Disable data transfer. */
@@ -981,6 +1004,8 @@ cad_down(struct cad_softc *sc)
        cad_dmamem_free(sc, sc->sc_rxring);
        sc->sc_rxring = NULL;
        sc->sc_rxdesc = NULL;
+
+       NET_LOCK();
 }
 
 uint8_t
@@ -1010,6 +1035,8 @@ cad_iff(struct cad_softc *sc)
        struct ether_multistep step;
        uint64_t hash;
        uint32_t netcfg;
+
+       rw_assert_wrlock(&sc->sc_cfg_lock);
 
        netcfg = HREAD4(sc, GEM_NETCFG);
        netcfg &= ~GEM_NETCFG_UCASTHASHEN;

Reply via email to