Thu Mar 3 05:53:23 2022 UTC ()
usbnet: Apply hardware multicast filter updates synchronously again.
To make this work:
1. Do it only under a new lock, unp_mcastlock. This lock lives at
IPL_SOFTCLOCK so it can be taken from network stack callouts. It
is forbidden to acquire the usbnet core lock under unp_mcastlock.
2. Do it only after usbnet_init_rx_tx and before usbnet_stop; if
issued at any other time, drop the update on the floor.
3. Make usbnet_init_rx_tx apply any pending multicast filter updates
under the lock before setting the flag that allows SIOCADDMULTI or
SIOCDELMULTI to apply the updates.
4. Remove core lock asserts from various drivers' register access
routines. This is necessary because the multicast filter updates
are done with register reads/writes, but _cannot_ take the core
lock when the caller holds softnet_lock.
This now programs the hardware multicast filter redundantly in many
drivers which already explicitly call *_uno_mcast from the *_uno_init
routines. This is probably harmless, but it will likely be better to
remove the explicit calls.
(riastradh)
diff -r1.180 -r1.181 src/sys/dev/usb/if_aue.c
diff -r1.141 -r1.142 src/sys/dev/usb/if_axe.c
diff -r1.83 -r1.84 src/sys/dev/usb/if_axen.c
diff -r1.82 -r1.83 src/sys/dev/usb/if_smsc.c
diff -r1.87 -r1.88 src/sys/dev/usb/if_udav.c
diff -r1.87 -r1.88 src/sys/dev/usb/if_url.c
diff -r1.48 -r1.49 src/sys/dev/usb/if_ure.c
diff -r1.81 -r1.82 src/sys/dev/usb/usbnet.c
--- src/sys/dev/usb/if_aue.c 2022/03/03 05:53:14 1.180
+++ src/sys/dev/usb/if_aue.c 2022/03/03 05:53:23 1.181
@@ -1,4 +1,4 @@
-/* $NetBSD: if_aue.c,v 1.180 2022/03/03 05:53:14 riastradh Exp $ */
+/* $NetBSD: if_aue.c,v 1.181 2022/03/03 05:53:23 riastradh Exp $ */
/*
* Copyright (c) 1997, 1998, 1999, 2000
@@ -76,7 +76,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_aue.c,v 1.180 2022/03/03 05:53:14 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_aue.c,v 1.181 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -284,8 +284,6 @@
usbd_status err;
uByte val = 0;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -315,8 +313,6 @@
usbd_status err;
uWord val;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -346,8 +342,6 @@
usbd_status err;
uByte val;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -378,8 +372,6 @@
usbd_status err;
uWord val;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -623,8 +615,6 @@
AUEHIST_FUNC();
AUEHIST_CALLARGSN(5, "aue%jd: enter", device_unit(un->un_dev), 0, 0, 0);
-
- usbnet_isowned_core(un);
if (ifp->if_flags & IFF_PROMISC) {
ETHER_LOCK(ec);
--- src/sys/dev/usb/if_axe.c 2022/03/03 05:53:04 1.141
+++ src/sys/dev/usb/if_axe.c 2022/03/03 05:53:23 1.142
@@ -1,4 +1,4 @@
-/* $NetBSD: if_axe.c,v 1.141 2022/03/03 05:53:04 riastradh Exp $ */
+/* $NetBSD: if_axe.c,v 1.142 2022/03/03 05:53:23 riastradh Exp $ */
/* $OpenBSD: if_axe.c,v 1.137 2016/04/13 11:03:37 mpi Exp $ */
/*
@@ -87,7 +87,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_axe.c,v 1.141 2022/03/03 05:53:04 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_axe.c,v 1.142 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -292,8 +292,6 @@
struct usbnet * const un = &sc->axe_un;
usb_device_request_t req;
usbd_status err;
-
- usbnet_isowned_core(un);
if (usbnet_isdying(un))
return -1;
--- src/sys/dev/usb/if_axen.c 2022/03/03 05:53:04 1.83
+++ src/sys/dev/usb/if_axen.c 2022/03/03 05:53:23 1.84
@@ -1,4 +1,4 @@
-/* $NetBSD: if_axen.c,v 1.83 2022/03/03 05:53:04 riastradh Exp $ */
+/* $NetBSD: if_axen.c,v 1.84 2022/03/03 05:53:23 riastradh 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.83 2022/03/03 05:53:04 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_axen.c,v 1.84 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -108,8 +108,6 @@
usb_device_request_t req;
usbd_status err;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -238,8 +236,6 @@
if (usbnet_isdying(un))
return;
-
- usbnet_isowned_core(un);
rxmode = 0;
--- src/sys/dev/usb/if_smsc.c 2022/03/03 05:53:14 1.82
+++ src/sys/dev/usb/if_smsc.c 2022/03/03 05:53:23 1.83
@@ -1,4 +1,4 @@
-/* $NetBSD: if_smsc.c,v 1.82 2022/03/03 05:53:14 riastradh Exp $ */
+/* $NetBSD: if_smsc.c,v 1.83 2022/03/03 05:53:23 riastradh Exp $ */
/* $OpenBSD: if_smsc.c,v 1.4 2012/09/27 12:38:11 jsg Exp $ */
/* $FreeBSD: src/sys/dev/usb/net/if_smsc.c,v 1.1 2012/08/15 04:03:55 gonzo Exp $ */
@@ -61,7 +61,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_smsc.c,v 1.82 2022/03/03 05:53:14 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_smsc.c,v 1.83 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -211,8 +211,6 @@
uint32_t buf;
usbd_status err;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -238,8 +236,6 @@
uint32_t buf;
usbd_status err;
- usbnet_isowned_core(un);
-
if (usbnet_isdying(un))
return 0;
@@ -421,8 +417,6 @@
struct ether_multistep step;
uint32_t hashtbl[2] = { 0, 0 };
uint32_t hash;
-
- usbnet_isowned_core(un);
if (usbnet_isdying(un))
return;
--- src/sys/dev/usb/if_udav.c 2022/03/03 05:53:14 1.87
+++ src/sys/dev/usb/if_udav.c 2022/03/03 05:53:23 1.88
@@ -1,4 +1,4 @@
-/* $NetBSD: if_udav.c,v 1.87 2022/03/03 05:53:14 riastradh Exp $ */
+/* $NetBSD: if_udav.c,v 1.88 2022/03/03 05:53:23 riastradh Exp $ */
/* $nabe: if_udav.c,v 1.3 2003/08/21 16:57:19 nabe Exp $ */
/*
@@ -45,7 +45,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_udav.c,v 1.87 2022/03/03 05:53:14 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_udav.c,v 1.88 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -364,7 +364,6 @@
usb_device_request_t req;
usbd_status err;
- usbnet_isowned_core(un);
KASSERT(!usbnet_isdying(un));
DPRINTFN(0x200,
@@ -395,7 +394,6 @@
usb_device_request_t req;
usbd_status err;
- usbnet_isowned_core(un);
KASSERT(!usbnet_isdying(un));
DPRINTFN(0x200,
@@ -424,8 +422,6 @@
{
uint8_t val = 0;
- usbnet_isowned_core(un);
-
DPRINTFN(0x200,
("%s: %s: enter\n", device_xname(un->un_dev), __func__));
@@ -442,7 +438,6 @@
usb_device_request_t req;
usbd_status err;
- usbnet_isowned_core(un);
KASSERT(!usbnet_isdying(un));
DPRINTFN(0x200,
@@ -585,8 +580,6 @@
int h = 0;
DPRINTF(("%s: %s: enter\n", device_xname(un->un_dev), __func__));
-
- usbnet_isowned_core(un);
if (usbnet_isdying(un))
return;
--- src/sys/dev/usb/if_url.c 2022/03/03 05:53:04 1.87
+++ src/sys/dev/usb/if_url.c 2022/03/03 05:53:23 1.88
@@ -1,4 +1,4 @@
-/* $NetBSD: if_url.c,v 1.87 2022/03/03 05:53:04 riastradh Exp $ */
+/* $NetBSD: if_url.c,v 1.88 2022/03/03 05:53:23 riastradh Exp $ */
/*
* Copyright (c) 2001, 2002
@@ -44,7 +44,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_url.c,v 1.87 2022/03/03 05:53:04 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_url.c,v 1.88 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_inet.h"
@@ -268,8 +268,6 @@
usb_device_request_t req;
usbd_status err;
- usbnet_isowned_core(un);
-
DPRINTFN(0x200,
("%s: %s: enter\n", device_xname(un->un_dev), __func__));
@@ -434,8 +432,6 @@
int h = 0, rcr;
DPRINTF(("%s: %s: enter\n", device_xname(un->un_dev), __func__));
-
- usbnet_isowned_core(un);
if (usbnet_isdying(un))
return;
--- src/sys/dev/usb/if_ure.c 2022/03/03 05:53:04 1.48
+++ src/sys/dev/usb/if_ure.c 2022/03/03 05:53:23 1.49
@@ -1,4 +1,4 @@
-/* $NetBSD: if_ure.c,v 1.48 2022/03/03 05:53:04 riastradh Exp $ */
+/* $NetBSD: if_ure.c,v 1.49 2022/03/03 05:53:23 riastradh 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.48 2022/03/03 05:53:04 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_ure.c,v 1.49 2022/03/03 05:53:23 riastradh Exp $");
#ifdef _KERNEL_OPT
#include "opt_usb.h"
@@ -339,8 +339,6 @@
struct ether_multistep step;
uint32_t mchash[2] = { 0, 0 };
uint32_t h = 0, rxmode;
-
- usbnet_isowned_core(un);
if (usbnet_isdying(un))
return;
--- src/sys/dev/usb/usbnet.c 2022/03/03 05:52:46 1.81
+++ src/sys/dev/usb/usbnet.c 2022/03/03 05:53:23 1.82
@@ -1,4 +1,4 @@
-/* $NetBSD: usbnet.c,v 1.81 2022/03/03 05:52:46 riastradh Exp $ */
+/* $NetBSD: usbnet.c,v 1.82 2022/03/03 05:53:23 riastradh Exp $ */
/*
* Copyright (c) 2019 Matthew R. Green
@@ -31,7 +31,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: usbnet.c,v 1.81 2022/03/03 05:52:46 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: usbnet.c,v 1.82 2022/03/03 05:53:23 riastradh Exp $");
#include <sys/param.h>
#include <sys/kernel.h>
@@ -59,6 +59,7 @@
*
* the lock ordering is:
* ifnet lock -> unp_core_lock -> unp_rxlock -> unp_txlock
+ * -> unp_mcastlock
* - ifnet lock is not needed for unp_core_lock, but if ifnet lock is
* involved, it must be taken first
*/
@@ -66,11 +67,13 @@
kmutex_t unp_rxlock;
kmutex_t unp_txlock;
+ kmutex_t unp_mcastlock;
+ bool unp_mcastactive;
+
struct usbnet_cdata unp_cdata;
struct ethercom unp_ec;
struct mii_data unp_mii;
- struct usb_task unp_mcasttask;
struct usb_task unp_ticktask;
struct callout unp_stat_ch;
struct usbd_pipe *unp_ep[USBNET_ENDPT_MAX];
@@ -866,6 +869,17 @@
"%s", ifp->if_xname);
ifp->if_flags |= IFF_RUNNING;
+ /*
+ * If the hardware has a multicast filter, program it and then
+ * allow updates to it while we're running.
+ */
+ if (un->un_ops->uno_mcast) {
+ mutex_enter(&unp->unp_mcastlock);
+ (*un->un_ops->uno_mcast)(ifp);
+ unp->unp_mcastactive = true;
+ mutex_exit(&unp->unp_mcastlock);
+ }
+
/* Start up the receive pipe(s). */
usbnet_rx_start_pipes(un);
@@ -1018,8 +1032,20 @@
switch (cmd) {
case SIOCADDMULTI:
case SIOCDELMULTI:
- usb_add_task(un->un_udev, &unp->unp_mcasttask,
- USB_TASKQ_DRIVER);
+ /*
+ * If there's a hardware multicast filter, and
+ * it has been programmed by usbnet_init_rx_tx
+ * and is active, update it now. Otherwise,
+ * drop the update on the floor -- it will be
+ * observed by usbnet_init_rx_tx next time we
+ * bring the interface up.
+ */
+ if (un->un_ops->uno_mcast) {
+ mutex_enter(&unp->unp_mcastlock);
+ if (unp->unp_mcastactive)
+ (*un->un_ops->uno_mcast)(ifp);
+ mutex_exit(&unp->unp_mcastlock);
+ }
error = 0;
break;
default:
@@ -1030,45 +1056,6 @@
return error;
}
-static void
-usbnet_mcast_task(void *arg)
-{
- USBNETHIST_FUNC();
- struct usbnet * const un = arg;
- struct ifnet * const ifp = usbnet_ifp(un);
-
- USBNETHIST_CALLARGSN(10, "%jd: enter",
- un->un_pri->unp_number, 0, 0, 0);
-
- /*
- * If we're detaching, we must check usbnet_isdying _before_
- * touching IFNET_LOCK -- the ifnet may have been detached by
- * the time this task runs. This is racy -- unp_dying may be
- * set immediately after we test it -- but nevertheless safe,
- * because usbnet_detach waits for the task to complete before
- * issuing if_detach, and necessary, so that we don't touch
- * IFNET_LOCK after if_detach. See usbnet_detach for details.
- */
- if (usbnet_isdying(un))
- return;
-
- /*
- * If the hardware is running, ask the driver to reprogram the
- * multicast filter. If the hardware is not running, the
- * driver is responsible for programming the multicast filter
- * as part of its uno_init routine to bring the hardware up.
- */
- IFNET_LOCK(ifp);
- if (ifp->if_flags & IFF_RUNNING) {
- if (un->un_ops->uno_mcast) {
- mutex_enter(&un->un_pri->unp_core_lock);
- (*un->un_ops->uno_mcast)(ifp);
- mutex_exit(&un->un_pri->unp_core_lock);
- }
- }
- IFNET_UNLOCK(ifp);
-}
-
/*
* Generic stop network function:
* - mark as stopping
@@ -1094,6 +1081,18 @@
usbnet_isowned_core(un);
/*
+ * For drivers with hardware multicast filter update callbacks:
+ * Prevent concurrent access to the hardware registers by
+ * multicast filter updates, which happens without IFNET_LOCK
+ * or the usbnet core lock.
+ */
+ if (un->un_ops->uno_mcast) {
+ mutex_enter(&unp->unp_mcastlock);
+ unp->unp_mcastactive = false;
+ mutex_exit(&unp->unp_mcastlock);
+ }
+
+ /*
* Prevent new activity (rescheduling ticks, xfers, &c.) and
* clear the watchdog timer.
*/
@@ -1387,8 +1386,6 @@
un->un_pri = kmem_zalloc(sizeof(*un->un_pri), KM_SLEEP);
struct usbnet_private * const unp = un->un_pri;
- usb_init_task(&unp->unp_mcasttask, usbnet_mcast_task, un,
- USB_TASKQ_MPSAFE);
usb_init_task(&unp->unp_ticktask, usbnet_tick_task, un,
USB_TASKQ_MPSAFE);
callout_init(&unp->unp_stat_ch, CALLOUT_MPSAFE);
@@ -1397,6 +1394,7 @@
mutex_init(&unp->unp_txlock, MUTEX_DEFAULT, IPL_SOFTUSB);
mutex_init(&unp->unp_rxlock, MUTEX_DEFAULT, IPL_SOFTUSB);
mutex_init(&unp->unp_core_lock, MUTEX_DEFAULT, IPL_NONE);
+ mutex_init(&unp->unp_mcastlock, MUTEX_DEFAULT, IPL_SOFTCLOCK);
rnd_attach_source(&unp->unp_rndsrc, device_xname(un->un_dev),
RND_TYPE_NET, RND_FLAG_DEFAULT);
@@ -1539,9 +1537,6 @@
KASSERT(!callout_pending(&unp->unp_stat_ch));
KASSERT(!usb_task_pending(un->un_udev, &unp->unp_ticktask));
- usb_rem_task_wait(un->un_udev, &unp->unp_mcasttask, USB_TASKQ_DRIVER,
- NULL);
-
if (mii) {
mii_detach(mii, MII_PHY_ANY, MII_OFFSET_ANY);
ifmedia_fini(&mii->mii_media);
@@ -1555,45 +1550,12 @@
}
usbnet_ec(un)->ec_mii = NULL;
- /*
- * We have already waited for the multicast task to complete.
- * Unfortunately, until if_detach, nothing has prevented it
- * from running again -- another thread might issue if_mcast_op
- * between the time of our first usb_rem_task_wait and the time
- * we actually get around to if_detach.
- *
- * Fortunately, the first usb_rem_task_wait ensures that if the
- * task is scheduled again, it will witness our setting of
- * unp_dying to true[*]. So after that point, if the task is
- * scheduled again, it will decline to touch IFNET_LOCK and do
- * nothing. But we still need to wait for it to complete.
- *
- * It would be nice if we could write
- *
- * if_pleasestopissuingmcastopsthanks(ifp);
- * usb_rem_task_wait(..., &unp->unp_mcasttask, ...);
- * if_detach(ifp);
- *
- * and then we would need only one usb_rem_task_wait.
- *
- * Unfortunately, there is no such operation available in
- * sys/net at the moment, and it would require a bit of
- * coordination with if_mcast_op and doifioctl probably under a
- * new lock. So we'll use this kludge until that mechanism is
- * invented.
- *
- * [*] This is not exactly a documented property of the API,
- * but it is implied by the single lock in the task queue
- * serializing changes to the task state.
- */
- usb_rem_task_wait(un->un_udev, &unp->unp_mcasttask, USB_TASKQ_DRIVER,
- NULL);
-
usbnet_rx_list_free(un);
usbnet_tx_list_free(un);
rnd_detach_source(&unp->unp_rndsrc);
+ mutex_destroy(&unp->unp_mcastlock);
mutex_destroy(&unp->unp_core_lock);
mutex_destroy(&unp->unp_rxlock);
mutex_destroy(&unp->unp_txlock);