Module Name: src
Committed By: mrg
Date: Fri Jun 28 01:57:43 UTC 2019
Modified Files:
src/sys/dev/usb: if_axen.c if_cdce.c if_ure.c
Log Message:
more smp cleanup for ure(4)/axen(4)/cdce(4):
- convert IFF_ALLMULTI to ETHER_F_ALLMULTI, using ETHER_LOCK()
- remove IFF_OACTIVE use, and simply check the ring count in start
- assert/take more locks
- XXX: IFF_RUNNING is not properly protected (all driver problem)
- fix axen_timer setting so it actually runs
- document a locking issue in stop callback:
stop is called with the softc lock held, but the lock order
in all other places is ifnet -> softc -> rx -> tx, so taking
ifnet lock when softc lock is held would be problematic
- in rxeof check for stopping/dying more often. i managed to
trigger a pagefault in cdce_rxeof() when yanking an active
device as it attempted to usbd_setup_xfer() on closed pipes.
- add missing USBD_MPSAFE and cdce_stopping resetting for cdce(4)
between this and other recent clean ups increase performance of
these drivers mostly. some numbers (in mbit/sec):
old: new:
driver in out in+out in out in+out
---- -- --- ------ -- --- ------
cdce 39 32 44 38 33 54
axen 44 34 45 48 37 42
ure 36 34 35 36 38 38
i'm not sure why axen drops a little with in+out. cdce is
helped quite a lot, and ure a little. it is disappointing that
ure does not outperform cdce -- it's the same actual hardware,
and the device-specific (ure) should outperform the generic
cdce driver...
To generate a diff of this commit:
cvs rdiff -u -r1.48 -r1.49 src/sys/dev/usb/if_axen.c
cvs rdiff -u -r1.50 -r1.51 src/sys/dev/usb/if_cdce.c
cvs rdiff -u -r1.12 -r1.13 src/sys/dev/usb/if_ure.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/usb/if_axen.c
diff -u src/sys/dev/usb/if_axen.c:1.48 src/sys/dev/usb/if_axen.c:1.49
--- src/sys/dev/usb/if_axen.c:1.48 Sat Jun 22 10:58:39 2019
+++ src/sys/dev/usb/if_axen.c Fri Jun 28 01:57:43 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: if_axen.c,v 1.48 2019/06/22 10:58:39 mrg Exp $ */
+/* $NetBSD: if_axen.c,v 1.49 2019/06/28 01:57:43 mrg Exp $ */
/* $OpenBSD: if_axen.c,v 1.3 2013/10/21 10:10:22 yuo Exp $ */
/*
@@ -23,7 +23,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_axen.c,v 1.48 2019/06/22 10:58:39 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_axen.c,v 1.49 2019/06/28 01:57:43 mrg Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -455,12 +455,14 @@ axen_iff_locked(struct axen_softc *sc)
rxmode = le16toh(wval);
rxmode &= ~(AXEN_RXCTL_ACPT_ALL_MCAST | AXEN_RXCTL_PROMISC |
AXEN_RXCTL_ACPT_MCAST);
- ifp->if_flags &= ~IFF_ALLMULTI;
if (ifp->if_flags & IFF_PROMISC) {
DPRINTF(("%s: promisc\n", device_xname(sc->axen_dev)));
rxmode |= AXEN_RXCTL_PROMISC;
-allmulti: ifp->if_flags |= IFF_ALLMULTI;
+allmulti:
+ ETHER_LOCK(ec);
+ ec->ec_flags |= ETHER_F_ALLMULTI;
+ ETHER_UNLOCK(ec);
rxmode |= AXEN_RXCTL_ACPT_ALL_MCAST
/* | AXEN_RXCTL_ACPT_PHY_MCAST */;
} else {
@@ -468,6 +470,8 @@ allmulti: ifp->if_flags |= IFF_ALLMULTI;
DPRINTF(("%s: initializing hash table\n",
device_xname(sc->axen_dev)));
ETHER_LOCK(ec);
+ ec->ec_flags &= ~ETHER_F_ALLMULTI;
+
ETHER_FIRST_MULTI(step, ec, enm);
while (enm != NULL) {
if (memcmp(enm->enm_addrlo, enm->enm_addrhi,
@@ -979,8 +983,11 @@ axen_detach(device_t self, int flags)
usb_rem_task_wait(sc->axen_udev, &sc->axen_tick_task,
USB_TASKQ_DRIVER, NULL);
- if (ifp->if_flags & IFF_RUNNING)
+ if (ifp->if_flags & IFF_RUNNING) {
+ IFNET_LOCK(ifp);
axen_stop(ifp, 1);
+ IFNET_UNLOCK(ifp);
+ }
mutex_enter(&sc->axen_lock);
sc->axen_refcnt--;
@@ -1256,7 +1263,7 @@ axen_rxeof(struct usbd_xfer *xfer, void
if_percpuq_enqueue((ifp)->if_percpuq, (m));
mutex_enter(&sc->axen_rxlock);
- if (sc->axen_stopping) {
+ if (sc->axen_dying || sc->axen_stopping) {
mutex_exit(&sc->axen_rxlock);
return;
}
@@ -1274,6 +1281,11 @@ nextpkt:
} while (pkt_count > 0);
done:
+ if (sc->axen_dying || sc->axen_stopping) {
+ mutex_exit(&sc->axen_rxlock);
+ return;
+ }
+
mutex_exit(&sc->axen_rxlock);
/* Setup new transfer. */
@@ -1351,7 +1363,6 @@ axen_txeof(struct usbd_xfer *xfer, void
cd->axen_tx_cnt--;
sc->axen_timer = 0;
- ifp->if_flags &= ~IFF_OACTIVE;
switch (status) {
case USBD_NOT_STARTED:
@@ -1489,6 +1500,7 @@ axen_encap(struct axen_softc *sc, struct
/* Transmit */
err = usbd_transfer(c->axen_xfer);
if (err != USBD_IN_PROGRESS) {
+ /* XXXSMP IFNET_LOCK */
axen_stop(ifp, 0);
return EIO;
}
@@ -1505,11 +1517,9 @@ axen_start_locked(struct ifnet *ifp)
int idx;
KASSERT(mutex_owned(&sc->axen_txlock));
+ KASSERT(cd->axen_tx_cnt <= AXEN_TX_LIST_CNT);
- if (sc->axen_link == 0)
- return;
-
- if ((ifp->if_flags & (IFF_OACTIVE | IFF_RUNNING)) != IFF_RUNNING)
+ if (sc->axen_link == 0 || (ifp->if_flags & IFF_RUNNING) == 0)
return;
idx = cd->axen_tx_prod;
@@ -1519,7 +1529,6 @@ axen_start_locked(struct ifnet *ifp)
break;
if (axen_encap(sc, m, idx)) {
- ifp->if_flags |= IFF_OACTIVE; /* XXX */
ifp->if_oerrors++;
break;
}
@@ -1537,9 +1546,6 @@ axen_start_locked(struct ifnet *ifp)
}
cd->axen_tx_prod = idx;
- if (cd->axen_tx_cnt >= AXEN_TX_LIST_CNT)
- ifp->if_flags |= IFF_OACTIVE;
-
/*
* Set a timeout in case the chip goes out to lunch.
*/
@@ -1646,8 +1652,8 @@ axen_init_locked(struct ifnet *ifp)
mutex_exit(&sc->axen_rxlock);
/* Indicate we are up and running. */
+ KASSERT(IFNET_LOCKED(ifp));
ifp->if_flags |= IFF_RUNNING;
- ifp->if_flags &= ~IFF_OACTIVE;
callout_schedule(&sc->axen_stat_ch, hz);
return 0;
@@ -1771,8 +1777,15 @@ axen_stop_locked(struct ifnet *ifp, int
axen_cmd(sc, AXEN_CMD_MAC_WRITE2, 2, AXEN_MAC_RXCTL, &wval);
axen_unlock_mii_sc_locked(sc);
+ /*
+ * XXXSMP Would like to
+ * KASSERT(IFNET_LOCKED(ifp))
+ * here but the locking order is:
+ * ifnet -> sc lock -> rxlock -> txlock
+ * and sc lock is already held.
+ */
+ ifp->if_flags &= ~IFF_RUNNING;
sc->axen_timer = 0;
- ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
callout_stop(&sc->axen_stat_ch);
sc->axen_link = 0;
Index: src/sys/dev/usb/if_cdce.c
diff -u src/sys/dev/usb/if_cdce.c:1.50 src/sys/dev/usb/if_cdce.c:1.51
--- src/sys/dev/usb/if_cdce.c:1.50 Sun Jun 23 02:14:14 2019
+++ src/sys/dev/usb/if_cdce.c Fri Jun 28 01:57:43 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: if_cdce.c,v 1.50 2019/06/23 02:14:14 mrg Exp $ */
+/* $NetBSD: if_cdce.c,v 1.51 2019/06/28 01:57:43 mrg Exp $ */
/*
* Copyright (c) 1997, 1998, 1999, 2000-2003 Bill Paul <[email protected]>
@@ -41,7 +41,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_cdce.c,v 1.50 2019/06/23 02:14:14 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_cdce.c,v 1.51 2019/06/28 01:57:43 mrg Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -146,7 +146,6 @@ static void cdce_txeof(struct usbd_xfer
static void cdce_start(struct ifnet *);
static int cdce_ioctl(struct ifnet *, u_long, void *);
static void cdce_init(void *);
-static void cdce_watchdog(struct ifnet *);
static void cdce_stop(struct cdce_softc *);
static void cdce_tick(void *);
static void cdce_tick_task(void *);
@@ -384,8 +383,11 @@ cdce_detach(device_t self, int flags)
usb_rem_task_wait(sc->cdce_udev, &sc->cdce_tick_task,
USB_TASKQ_DRIVER, NULL);
- if (ifp->if_flags & IFF_RUNNING)
+ if (ifp->if_flags & IFF_RUNNING) {
+ IFNET_LOCK(ifp);
cdce_stop(sc);
+ IFNET_UNLOCK(ifp);
+ }
callout_destroy(&sc->cdce_stat_ch);
ether_ifdetach(ifp);
@@ -408,8 +410,8 @@ cdce_start_locked(struct ifnet *ifp)
struct mbuf *m_head = NULL;
KASSERT(mutex_owned(&sc->cdce_txlock));
- if (sc->cdce_dying || sc->cdce_stopping ||
- (ifp->if_flags & IFF_OACTIVE))
+
+ if (sc->cdce_dying || sc->cdce_cdata.cdce_tx_cnt == CDCE_TX_LIST_CNT)
return;
IFQ_POLL(&ifp->if_snd, m_head);
@@ -425,12 +427,10 @@ cdce_start_locked(struct ifnet *ifp)
bpf_mtap(ifp, m_head, BPF_D_OUT);
- ifp->if_flags |= IFF_OACTIVE;
-
/*
* Set a timeout in case the chip goes out to lunch.
*/
- ifp->if_timer = 6;
+ sc->cdce_timer = 6;
}
static void
@@ -469,30 +469,42 @@ cdce_encap(struct cdce_softc *sc, struct
USBD_FORCE_SHORT_XFER, 10000, cdce_txeof);
err = usbd_transfer(c->cdce_xfer);
if (err != USBD_IN_PROGRESS) {
+ /* XXXSMP IFNET_LOCK */
cdce_stop(sc);
return EIO;
}
sc->cdce_cdata.cdce_tx_cnt++;
+ KASSERT(sc->cdce_cdata.cdce_tx_cnt <= CDCE_TX_LIST_CNT);
return 0;
}
static void
-cdce_stop(struct cdce_softc *sc)
+cdce_stop_locked(struct cdce_softc *sc)
{
usbd_status err;
struct ifnet *ifp = GET_IFP(sc);
int i;
+ /* XXXSMP can't KASSERT(IFNET_LOCKED(ifp)); */
+ KASSERT(mutex_owned(&sc->cdce_lock));
+
mutex_enter(&sc->cdce_rxlock);
mutex_enter(&sc->cdce_txlock);
sc->cdce_stopping = true;
mutex_exit(&sc->cdce_txlock);
mutex_exit(&sc->cdce_rxlock);
- ifp->if_timer = 0;
- ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+ /*
+ * XXXSMP Would like to
+ * KASSERT(IFNET_LOCKED(ifp))
+ * here but the locking order is:
+ * ifnet -> sc lock -> rxlock -> txlock
+ * and sc lock is already held.
+ */
+ ifp->if_flags &= ~IFF_RUNNING;
+ sc->cdce_timer = 0;
callout_stop(&sc->cdce_stat_ch);
@@ -549,7 +561,15 @@ cdce_stop(struct cdce_softc *sc)
device_xname(sc->cdce_dev), usbd_errstr(err));
sc->cdce_bulkout_pipe = NULL;
}
- mutex_exit(&sc->cdce_txlock);
+}
+
+static void
+cdce_stop(struct cdce_softc *sc)
+{
+
+ mutex_enter(&sc->cdce_lock);
+ cdce_stop_locked(sc);
+ mutex_exit(&sc->cdce_lock);
}
static int
@@ -613,7 +633,7 @@ cdce_ioctl(struct ifnet *ifp, u_long com
static void
cdce_watchdog(struct ifnet *ifp)
{
- struct cdce_softc *sc = ifp->if_softc;
+ struct cdce_softc *sc = ifp->if_softc;
struct cdce_chain *c;
usbd_status stat;
@@ -625,12 +645,16 @@ cdce_watchdog(struct ifnet *ifp)
ifp->if_oerrors++;
aprint_error_dev(sc->cdce_dev, "watchdog timeout\n");
+ mutex_enter(&sc->cdce_txlock);
+
c = &sc->cdce_cdata.cdce_rx_chain[0];
usbd_get_xfer_status(c->cdce_xfer, NULL, NULL, NULL, &stat);
cdce_txeof(c->cdce_xfer, c, stat);
if (!IFQ_IS_EMPTY(&ifp->if_snd))
cdce_start_locked(ifp);
+
+ mutex_exit(&sc->cdce_txlock);
}
static void
@@ -649,7 +673,7 @@ cdce_init(void *xsc)
/* Maybe set multicast / broadcast here??? */
err = usbd_open_pipe(sc->cdce_data_iface, sc->cdce_bulkin_no,
- USBD_EXCLUSIVE_USE, &sc->cdce_bulkin_pipe);
+ USBD_EXCLUSIVE_USE | USBD_MPSAFE, &sc->cdce_bulkin_pipe);
if (err) {
printf("%s: open rx pipe failed: %s\n", device_xname(sc->cdce_dev),
usbd_errstr(err));
@@ -657,7 +681,7 @@ cdce_init(void *xsc)
}
err = usbd_open_pipe(sc->cdce_data_iface, sc->cdce_bulkout_no,
- USBD_EXCLUSIVE_USE, &sc->cdce_bulkout_pipe);
+ USBD_EXCLUSIVE_USE | USBD_MPSAFE, &sc->cdce_bulkout_pipe);
if (err) {
printf("%s: open tx pipe failed: %s\n",
device_xname(sc->cdce_dev), usbd_errstr(err));
@@ -674,6 +698,10 @@ cdce_init(void *xsc)
goto out;
}
+ mutex_enter(&sc->cdce_rxlock);
+ mutex_enter(&sc->cdce_txlock);
+ sc->cdce_stopping = false;
+
for (i = 0; i < CDCE_RX_LIST_CNT; i++) {
c = &sc->cdce_cdata.cdce_rx_chain[i];
@@ -682,8 +710,10 @@ cdce_init(void *xsc)
usbd_transfer(c->cdce_xfer);
}
+ mutex_exit(&sc->cdce_txlock);
+ mutex_exit(&sc->cdce_rxlock);
+
ifp->if_flags |= IFF_RUNNING;
- ifp->if_flags &= ~IFF_OACTIVE;
callout_schedule(&sc->cdce_stat_ch, hz);
@@ -831,12 +861,12 @@ cdce_rxeof(struct usbd_xfer *xfer, void
if_percpuq_enqueue(ifp->if_percpuq, m);
mutex_enter(&sc->cdce_rxlock);
+done:
if (sc->cdce_stopping || sc->cdce_dying) {
mutex_exit(&sc->cdce_rxlock);
return;
}
-done:
mutex_exit(&sc->cdce_rxlock);
/* Setup new transfer. */
@@ -858,8 +888,10 @@ cdce_txeof(struct usbd_xfer *xfer, void
if (sc->cdce_dying)
goto out;
+ KASSERT(sc->cdce_cdata.cdce_tx_cnt > 0);
+ sc->cdce_cdata.cdce_tx_cnt--;
+
sc->cdce_timer = 0;
- ifp->if_flags &= ~IFF_OACTIVE;
switch (status) {
case USBD_NOT_STARTED:
Index: src/sys/dev/usb/if_ure.c
diff -u src/sys/dev/usb/if_ure.c:1.12 src/sys/dev/usb/if_ure.c:1.13
--- src/sys/dev/usb/if_ure.c:1.12 Mon Jun 24 04:42:06 2019
+++ src/sys/dev/usb/if_ure.c Fri Jun 28 01:57:43 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: if_ure.c,v 1.12 2019/06/24 04:42:06 mrg Exp $ */
+/* $NetBSD: if_ure.c,v 1.13 2019/06/28 01:57:43 mrg Exp $ */
/* $OpenBSD: if_ure.c,v 1.10 2018/11/02 21:32:30 jcs Exp $ */
/*-
@@ -30,7 +30,7 @@
/* RealTek RTL8152/RTL8153 10/100/Gigabit USB Ethernet device */
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_ure.c,v 1.12 2019/06/24 04:42:06 mrg Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_ure.c,v 1.13 2019/06/28 01:57:43 mrg Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -436,7 +436,6 @@ ure_iff_locked(struct ure_softc *sc)
rxmode = ure_read_4(sc, URE_PLA_RCR, URE_MCU_TYPE_PLA);
rxmode &= ~URE_RCR_ACPT_ALL;
- ifp->if_flags &= ~IFF_ALLMULTI;
/*
* Always accept frames destined to our station address.
@@ -446,13 +445,18 @@ ure_iff_locked(struct ure_softc *sc)
if (ifp->if_flags & IFF_PROMISC) {
rxmode |= URE_RCR_AAP;
-allmulti: ifp->if_flags |= IFF_ALLMULTI;
+allmulti:
+ ETHER_LOCK(ec);
+ ec->ec_flags |= ETHER_F_ALLMULTI;
+ ETHER_UNLOCK(ec);
rxmode |= URE_RCR_AM;
hashes[0] = hashes[1] = 0xffffffff;
} else {
rxmode |= URE_RCR_AM;
ETHER_LOCK(ec);
+ ec->ec_flags &= ~ETHER_F_ALLMULTI;
+
ETHER_FIRST_MULTI(step, ec, enm);
while (enm != NULL) {
if (memcmp(enm->enm_addrlo, enm->enm_addrhi,
@@ -597,8 +601,8 @@ ure_init_locked(struct ifnet *ifp)
mutex_exit(&sc->ure_rxlock);
/* Indicate we are up and running. */
+ KASSERT(IFNET_LOCKED(ifp));
ifp->if_flags |= IFF_RUNNING;
- ifp->if_flags &= ~IFF_OACTIVE;
callout_reset(&sc->ure_stat_ch, hz, ure_tick, sc);
@@ -625,9 +629,12 @@ ure_start_locked(struct ifnet *ifp)
struct ure_cdata *cd = &sc->ure_cdata;
int idx;
+ KASSERT(cd->tx_cnt <= URE_TX_LIST_CNT);
+
if (sc->ure_dying || sc->ure_stopping ||
(sc->ure_flags & URE_FLAG_LINK) == 0 ||
- (ifp->if_flags & (IFF_OACTIVE | IFF_RUNNING)) != IFF_RUNNING) {
+ (ifp->if_flags & IFF_RUNNING) == 0 ||
+ cd->tx_cnt == URE_TX_LIST_CNT) {
return;
}
@@ -650,9 +657,6 @@ ure_start_locked(struct ifnet *ifp)
cd->tx_cnt++;
}
cd->tx_prod = idx;
-
- if (cd->tx_cnt >= URE_TX_LIST_CNT)
- ifp->if_flags |= IFF_OACTIVE;
}
static void
@@ -698,7 +702,14 @@ ure_stop_locked(struct ifnet *ifp, int d
ure_reset(sc);
- ifp->if_flags &= ~(IFF_RUNNING | IFF_OACTIVE);
+ /*
+ * XXXSMP Would like to
+ * KASSERT(IFNET_LOCKED(ifp))
+ * here but the locking order is:
+ * ifnet -> sc lock -> rxlock -> txlock
+ * and sc lock is already held.
+ */
+ ifp->if_flags &= ~IFF_RUNNING;
callout_stop(&sc->ure_stat_ch);
@@ -1350,8 +1361,11 @@ ure_detach(device_t self, int flags)
/* partial-attach, below items weren't configured. */
if (sc->ure_phyno != -1) {
- if (ifp->if_flags & IFF_RUNNING)
+ if (ifp->if_flags & IFF_RUNNING) {
+ IFNET_LOCK(ifp);
ure_stop(ifp, 1);
+ IFNET_UNLOCK(ifp);
+ }
rnd_detach_source(&sc->ure_rnd_source);
mii_detach(&sc->ure_mii, MII_PHY_ANY, MII_OFFSET_ANY);
@@ -1535,7 +1549,7 @@ ure_rxeof(struct usbd_xfer *xfer, void *
if_percpuq_enqueue(ifp->if_percpuq, m);
mutex_enter(&sc->ure_rxlock);
- if (sc->ure_stopping) {
+ if (sc->ure_dying || sc->ure_stopping) {
mutex_exit(&sc->ure_rxlock);
return;
}
@@ -1543,6 +1557,10 @@ ure_rxeof(struct usbd_xfer *xfer, void *
} while (total_len > 0);
done:
+ if (sc->ure_dying || sc->ure_stopping) {
+ mutex_exit(&sc->ure_rxlock);
+ return;
+ }
mutex_exit(&sc->ure_rxlock);
/* Setup new transfer. */
@@ -1609,8 +1627,6 @@ ure_txeof(struct usbd_xfer *xfer, void *
KASSERT(cd->tx_cnt > 0);
cd->tx_cnt--;
- ifp->if_flags &= ~IFF_OACTIVE;
-
switch (status) {
case USBD_NOT_STARTED:
case USBD_CANCELLED:
@@ -1721,6 +1737,7 @@ ure_encap(struct ure_softc *sc, struct m
err = usbd_transfer(c->uc_xfer);
if (err != USBD_IN_PROGRESS) {
+ /* XXXSMP IFNET_LOCK */
ure_stop(ifp, 0);
return EIO;
}
