NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/57645: bridge does not work on raspberry pi
The following reply was made to PR kern/57645; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: sc.dying%gmail.com@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/57645: bridge does not work on raspberry pi
Date: Mon, 9 Oct 2023 12:08:57 +0000
This is a multi-part message in MIME format.
--=_3GH/d8a4GiEuk6sY511/abKYWvHzDwXu
Can you please try this updated patch series (git am), or single diff
(git apply, or just patch(1))?
--=_3GH/d8a4GiEuk6sY511/abKYWvHzDwXu
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57645-usbreinit-v3"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57645-usbreinit-v3.patch"
From 053b62a76f188a38cfba622781b8a7f9a8782617 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Mon, 9 Oct 2023 11:59:20 +0000
Subject: [PATCH 1/3] usbnet(9): Make sure unp->unp_if_flags is initialized =
on
init.
usbnet_ifflags_cb is only called if the flags change while up and
running. (XXX Maybe it should be called in other circumstances too
so there's only one path here?)
Out of paranoia, clear the cache on stop.
PR kern/57645
XXX pullup-10
---
sys/dev/usb/usbnet.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c
index c77e08e07f7e..76ce9a3f5c61 100644
--- a/sys/dev/usb/usbnet.c
+++ b/sys/dev/usb/usbnet.c
@@ -867,6 +867,8 @@ usbnet_init_rx_tx(struct usbnet * const un)
*/
if (un->un_ops->uno_mcast) {
mutex_enter(&unp->unp_mcastlock);
+ KASSERTMSG(!unp->unp_mcastactive, "%s", ifp->if_xname);
+ unp->unp_if_flags =3D ifp->if_flags;
(*un->un_ops->uno_mcast)(ifp);
unp->unp_mcastactive =3D true;
mutex_exit(&unp->unp_mcastlock);
@@ -1000,6 +1002,13 @@ usbnet_media_upd(struct ifnet *ifp)
=20
/* ioctl */
=20
+/*
+ * usbnet_ifflags_cb(ec)
+ *
+ * Called by if_ethersubr when interface flags change
+ * (SIOCSIFFLAGS), or ethernet capabilities change
+ * (SIOCSETHERCAP), on a running interface.
+ */
static int
usbnet_ifflags_cb(struct ethercom *ec)
{
@@ -1120,7 +1129,9 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int=
disable)
*/
if (un->un_ops->uno_mcast) {
mutex_enter(&unp->unp_mcastlock);
+ KASSERTMSG(unp->unp_mcastactive, "%p", ifp->if_xname);
unp->unp_mcastactive =3D false;
+ unp->unp_if_flags =3D 0;
mutex_exit(&unp->unp_mcastlock);
}
=20
From 761831e5a8234b8197f9feceafcf4bcff539e133 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Mon, 9 Oct 2023 12:04:22 +0000
Subject: [PATCH 2/3] usbnet(9): Fix sense of conditional in usbnet_ifflags_=
cb.
This appears to have been mistranscribed in revision 1.1 of usbnet.c.
PR kern/57645
XXX pullup-10
---
sys/dev/usb/usbnet.c | 54 ++++++++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c
index 76ce9a3f5c61..771b537bda40 100644
--- a/sys/dev/usb/usbnet.c
+++ b/sys/dev/usb/usbnet.c
@@ -1021,29 +1021,39 @@ usbnet_ifflags_cb(struct ethercom *ec)
KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname);
=20
const u_short changed =3D ifp->if_flags ^ unp->unp_if_flags;
- if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) =3D=3D 0) {
- mutex_enter(&unp->unp_mcastlock);
- unp->unp_if_flags =3D ifp->if_flags;
- mutex_exit(&unp->unp_mcastlock);
- /*
- * XXX Can we just do uno_mcast synchronously here
- * instead of resetting the whole interface?
- *
- * Not yet, because some usbnet drivers (e.g., aue(4))
- * initialize the hardware differently in uno_init
- * depending on IFF_PROMISC. But some (again, aue(4))
- * _also_ need to know whether IFF_PROMISC is set in
- * uno_mcast and do something different with it there.
- * Maybe the logic can be unified, but it will require
- * an audit and testing of all the usbnet drivers.
- */
- if (changed & IFF_PROMISC)
- rv =3D ENETRESET;
- } else {
- rv =3D ENETRESET;
- }
=20
- return rv;
+ /*
+ * If any user-settable flags have changed other than
+ * IFF_DEBUG, just reset the interface.
+ */
+ if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) !=3D 0)
+ return ENETRESET;
+
+ /*
+ * Otherwise, cache the flags change so we can read the flags
+ * under uno_mcastlock for multicast updates in SIOCADDMULTI or
+ * SIOCDELMULTI without IFNET_LOCK.
+ */
+ mutex_enter(&unp->unp_mcastlock);
+ unp->unp_if_flags =3D ifp->if_flags;
+ mutex_exit(&unp->unp_mcastlock);
+
+ /*
+ * If we're switching on or off promiscuous mode, reprogram the
+ * hardware multicast filter now.
+ *
+ * XXX Actually, reset the interface, because some usbnet
+ * drivers (e.g., aue(4)) initialize the hardware differently
+ * in uno_init depending on IFF_PROMISC. But some (again,
+ * aue(4)) _also_ need to know whether IFF_PROMISC is set in
+ * uno_mcast and do something different with it there. Maybe
+ * the logic can be unified, but it will require an audit and
+ * testing of all the usbnet drivers.
+ */
+ if (changed & IFF_PROMISC)
+ return ENETRESET;
+
+ return 0;
}
=20
bool
From ce6d546a34f9689b3a06922e1e407c9f14d1e389 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Mon, 9 Oct 2023 12:04:59 +0000
Subject: [PATCH 3/3] usbnet(9): On if_init, stop/init if IFF_RUNNING -- not
noop.
ether_ioctl(9) relies on this to reinitialize an interface when a
flags change returns ENETRESET. We can't just reprogram the hardware
multicast filter because some drivers have logic in if_init that's
conditional on IFF_PROMISC; perhaps we can reduce the cost of this if
we can change those drivers to do it in uno_mcast but that requires
some analysis to determine.
PR kern/57645
XXX pullup-10
---
sys/dev/usb/usbnet.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c
index 771b537bda40..5a3479671110 100644
--- a/sys/dev/usb/usbnet.c
+++ b/sys/dev/usb/usbnet.c
@@ -1131,6 +1131,7 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int=
disable)
USBNETHIST_FUNC(); USBNETHIST_CALLED();
=20
KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname);
+ KASSERTMSG(ifp->if_flags & IFF_RUNNING, "%s", ifp->if_xname);
=20
/*
* For drivers with hardware multicast filter update callbacks:
@@ -1321,14 +1322,17 @@ usbnet_if_init(struct ifnet *ifp)
return EIO;
=20
/*
- * If we're already running, nothing to do.
+ * If we're already running, stop the interface first -- we're
+ * reinitializing it.
*
- * XXX This should be an assertion, but it may require some
- * analysis -- and possibly some tweaking -- of sys/net to
- * ensure.
+ * XXX Grody for sys/net to call if_init to reinitialize. This
+ * should be an assertion, not a branch, but it will require
+ * some tweaking of sys/net to avoid. See also the comment in
+ * usbnet_ifflags_cb about if_init vs uno_mcast on reinitalize.
*/
if (ifp->if_flags & IFF_RUNNING)
- return 0;
+ usbnet_stop(un, ifp, /*disable*/1/*XXX???*/);
+ KASSERTMSG((ifp->if_flags & IFF_RUNNING) =3D=3D 0, "%s", ifp->if_xname);
=20
error =3D uno_init(un, ifp);
if (error)
--=_3GH/d8a4GiEuk6sY511/abKYWvHzDwXu
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57645-usbreinit-v3"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57645-usbreinit-v3.diff"
diff --git a/sys/dev/usb/usbnet.c b/sys/dev/usb/usbnet.c
index c77e08e07f7e..5a3479671110 100644
--- a/sys/dev/usb/usbnet.c
+++ b/sys/dev/usb/usbnet.c
@@ -867,6 +867,8 @@ usbnet_init_rx_tx(struct usbnet * const un)
*/
if (un->un_ops->uno_mcast) {
mutex_enter(&unp->unp_mcastlock);
+ KASSERTMSG(!unp->unp_mcastactive, "%s", ifp->if_xname);
+ unp->unp_if_flags =3D ifp->if_flags;
(*un->un_ops->uno_mcast)(ifp);
unp->unp_mcastactive =3D true;
mutex_exit(&unp->unp_mcastlock);
@@ -1000,6 +1002,13 @@ usbnet_media_upd(struct ifnet *ifp)
=20
/* ioctl */
=20
+/*
+ * usbnet_ifflags_cb(ec)
+ *
+ * Called by if_ethersubr when interface flags change
+ * (SIOCSIFFLAGS), or ethernet capabilities change
+ * (SIOCSETHERCAP), on a running interface.
+ */
static int
usbnet_ifflags_cb(struct ethercom *ec)
{
@@ -1012,29 +1021,39 @@ usbnet_ifflags_cb(struct ethercom *ec)
KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname);
=20
const u_short changed =3D ifp->if_flags ^ unp->unp_if_flags;
- if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) =3D=3D 0) {
- mutex_enter(&unp->unp_mcastlock);
- unp->unp_if_flags =3D ifp->if_flags;
- mutex_exit(&unp->unp_mcastlock);
- /*
- * XXX Can we just do uno_mcast synchronously here
- * instead of resetting the whole interface?
- *
- * Not yet, because some usbnet drivers (e.g., aue(4))
- * initialize the hardware differently in uno_init
- * depending on IFF_PROMISC. But some (again, aue(4))
- * _also_ need to know whether IFF_PROMISC is set in
- * uno_mcast and do something different with it there.
- * Maybe the logic can be unified, but it will require
- * an audit and testing of all the usbnet drivers.
- */
- if (changed & IFF_PROMISC)
- rv =3D ENETRESET;
- } else {
- rv =3D ENETRESET;
- }
=20
- return rv;
+ /*
+ * If any user-settable flags have changed other than
+ * IFF_DEBUG, just reset the interface.
+ */
+ if ((changed & ~(IFF_CANTCHANGE | IFF_DEBUG)) !=3D 0)
+ return ENETRESET;
+
+ /*
+ * Otherwise, cache the flags change so we can read the flags
+ * under uno_mcastlock for multicast updates in SIOCADDMULTI or
+ * SIOCDELMULTI without IFNET_LOCK.
+ */
+ mutex_enter(&unp->unp_mcastlock);
+ unp->unp_if_flags =3D ifp->if_flags;
+ mutex_exit(&unp->unp_mcastlock);
+
+ /*
+ * If we're switching on or off promiscuous mode, reprogram the
+ * hardware multicast filter now.
+ *
+ * XXX Actually, reset the interface, because some usbnet
+ * drivers (e.g., aue(4)) initialize the hardware differently
+ * in uno_init depending on IFF_PROMISC. But some (again,
+ * aue(4)) _also_ need to know whether IFF_PROMISC is set in
+ * uno_mcast and do something different with it there. Maybe
+ * the logic can be unified, but it will require an audit and
+ * testing of all the usbnet drivers.
+ */
+ if (changed & IFF_PROMISC)
+ return ENETRESET;
+
+ return 0;
}
=20
bool
@@ -1112,6 +1131,7 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int=
disable)
USBNETHIST_FUNC(); USBNETHIST_CALLED();
=20
KASSERTMSG(IFNET_LOCKED(ifp), "%s", ifp->if_xname);
+ KASSERTMSG(ifp->if_flags & IFF_RUNNING, "%s", ifp->if_xname);
=20
/*
* For drivers with hardware multicast filter update callbacks:
@@ -1120,7 +1140,9 @@ usbnet_stop(struct usbnet *un, struct ifnet *ifp, int=
disable)
*/
if (un->un_ops->uno_mcast) {
mutex_enter(&unp->unp_mcastlock);
+ KASSERTMSG(unp->unp_mcastactive, "%p", ifp->if_xname);
unp->unp_mcastactive =3D false;
+ unp->unp_if_flags =3D 0;
mutex_exit(&unp->unp_mcastlock);
}
=20
@@ -1300,14 +1322,17 @@ usbnet_if_init(struct ifnet *ifp)
return EIO;
=20
/*
- * If we're already running, nothing to do.
+ * If we're already running, stop the interface first -- we're
+ * reinitializing it.
*
- * XXX This should be an assertion, but it may require some
- * analysis -- and possibly some tweaking -- of sys/net to
- * ensure.
+ * XXX Grody for sys/net to call if_init to reinitialize. This
+ * should be an assertion, not a branch, but it will require
+ * some tweaking of sys/net to avoid. See also the comment in
+ * usbnet_ifflags_cb about if_init vs uno_mcast on reinitalize.
*/
if (ifp->if_flags & IFF_RUNNING)
- return 0;
+ usbnet_stop(un, ifp, /*disable*/1/*XXX???*/);
+ KASSERTMSG((ifp->if_flags & IFF_RUNNING) =3D=3D 0, "%s", ifp->if_xname);
=20
error =3D uno_init(un, ifp);
if (error)
--=_3GH/d8a4GiEuk6sY511/abKYWvHzDwXu--
Home |
Main Index |
Thread Index |
Old Index