On 2020-05-25 13:19, Martin Pieuchot wrote:
On 25/05/20(Mon) 12:56, Gerhard Roth wrote:
On 5/22/20 9:05 PM, Mark Kettenis wrote:
From: Łukasz Lejtkowski <emig...@gmail.com>
Date: Fri, 22 May 2020 20:51:57 +0200
Probably power supply 12 V is broken. Showing 16,87 V(Fluke 179) -
too high. Should be 12,25-12,50 V. I replaced to the new one.
That might be why the device stops responding. The fact that cleaning
up from a failed USB transaction leads to this panic is a bug though.
And somebody just posted a very similar panic with ure(4). Something
in the network stack is holding a mutex when it shouldn't.
I think that holding the mutex is ok. The bug is calling the stop
routine in case of errors.
This is what common foo_start() does:
m_head = ifq_deq_begin(&ifp->if_snd);
if (foo_encap(sc, m_head, 0)) {
ifq_deq_rollback(&ifp->if_snd, m_head);
...
return;
}
ifq_deq_commit(&ifp->if_snd, m_head);
Here, ifq_deq_begin() grabs a mutex and it is held while
calling foo_encap().
For USB network interfaces foo_encap() mostly does this:
err = usbd_transfer(sc->sc_xfer);
if (err != USBD_IN_PROGRESS) {
foo_stop(sc);
return EIO;
}
And foo_stop() calls usbd_abort_pipe() -> xhci_command_submit(),
which might sleep.
How to fix? We could do the foo_encap() after the ifq_deq_commit(),
possibly dropping the current mbuf if encap fails (who cares
for the packets after foo_stop() anyway).
That's the approach taken by drivers using ifq_dequeue(9) instead of
ifq_deq_begin/commit().
Or change all the drivers to follow the path that if_aue.c takes:
err = usbd_transfer(c->aue_xfer);
if (err != USBD_IN_PROGRESS) {
...
/* Stop the interface from process context. */
usb_add_task(sc->aue_udev, &sc->aue_stop_task);
return (EIO);
}
That's just trading the current problem for another one with higher
complexity.
Any ideas, what's better? Or alternative proposals?
Using ifq_dequeue(9) would have the advantage of unifying the code base.
It introduces a behavior change. A simpler fix would be to call
foo_stop() in the error path after ifq_deq_rollback().
Hi,
two weeks passed any nobody objected Martin's proposal. So I thought,
we could try to move on this way.
Gerhard
Index: sys/dev/usb/if_axe.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.139
diff -u -p -u -p -r1.139 if_axe.c
--- sys/dev/usb/if_axe.c 7 Jul 2019 06:40:10 -0000 1.139
+++ sys/dev/usb/if_axe.c 8 Jun 2020 15:13:25 -0000
@@ -1223,6 +1223,7 @@ axe_encap(struct axe_softc *sc, struct m
/* Transmit */
err = usbd_transfer(c->axe_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->axe_mbuf = NULL;
axe_stop(sc);
return(EIO);
}
@@ -1246,16 +1247,15 @@ axe_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (axe_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
/*
* If there's a BPF listener, bounce a copy of this frame
Index: sys/dev/usb/if_axen.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.27
diff -u -p -u -p -r1.27 if_axen.c
--- sys/dev/usb/if_axen.c 7 Jul 2019 06:40:10 -0000 1.27
+++ sys/dev/usb/if_axen.c 8 Jun 2020 14:49:26 -0000
@@ -1186,6 +1186,7 @@ axen_encap(struct axen_softc *sc, struct
/* Transmit */
err = usbd_transfer(c->axen_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->axen_mbuf = NULL;
axen_stop(sc);
return EIO;
}
@@ -1209,16 +1210,15 @@ axen_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (axen_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
/*
* If there's a BPF listener, bounce a copy of this frame
Index: sys/dev/usb/if_cdce.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_cdce.c,v
retrieving revision 1.76
diff -u -p -u -p -r1.76 if_cdce.c
--- sys/dev/usb/if_cdce.c 14 Nov 2019 13:50:55 -0000 1.76
+++ sys/dev/usb/if_cdce.c 8 Jun 2020 14:50:40 -0000
@@ -375,18 +375,16 @@ cdce_start(struct ifnet *ifp)
if (usbd_is_dying(sc->cdce_udev) || ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (cdce_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
-
#if NBPFILTER > 0
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, m_head, BPF_DIRECTION_OUT);
@@ -422,6 +420,7 @@ cdce_encap(struct cdce_softc *sc, struct
10000, cdce_txeof);
err = usbd_transfer(c->cdce_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->cdce_mbuf = NULL;
cdce_stop(sc);
return (EIO);
}
Index: sys/dev/usb/if_kue.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_kue.c,v
retrieving revision 1.89
diff -u -p -u -p -r1.89 if_kue.c
--- sys/dev/usb/if_kue.c 2 Oct 2018 19:49:10 -0000 1.89
+++ sys/dev/usb/if_kue.c 8 Jun 2020 14:52:27 -0000
@@ -829,6 +829,7 @@ kue_send(struct kue_softc *sc, struct mb
if (err != USBD_IN_PROGRESS) {
printf("%s: kue_send error=%s\n", sc->kue_dev.dv_xname,
usbd_errstr(err));
+ c->kue_mbuf = NULL;
kue_stop(sc);
return (EIO);
}
@@ -852,17 +853,15 @@ kue_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (kue_send(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
-
- ifq_deq_commit(&ifp->if_snd, m_head);
#if NBPFILTER > 0
/*
Index: sys/dev/usb/if_mos.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_mos.c,v
retrieving revision 1.40
diff -u -p -u -p -r1.40 if_mos.c
--- sys/dev/usb/if_mos.c 7 Jul 2019 06:40:10 -0000 1.40
+++ sys/dev/usb/if_mos.c 8 Jun 2020 15:15:42 -0000
@@ -1097,6 +1097,7 @@ mos_encap(struct mos_softc *sc, struct m
/* Transmit */
err = usbd_transfer(c->mos_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->mos_mbuf = NULL;
mos_stop(sc);
return(EIO);
}
@@ -1120,16 +1121,15 @@ mos_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (mos_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
/*
* If there's a BPF listener, bounce a copy of this frame
Index: sys/dev/usb/if_mue.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_mue.c,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 if_mue.c
--- sys/dev/usb/if_mue.c 7 Jul 2019 06:40:10 -0000 1.7
+++ sys/dev/usb/if_mue.c 8 Jun 2020 14:55:07 -0000
@@ -987,6 +987,7 @@ mue_encap(struct mue_softc *sc, struct m
/* Transmit */
err = usbd_transfer(c->mue_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->mue_mbuf = NULL;
mue_stop(sc);
return(EIO);
}
@@ -1308,16 +1309,15 @@ mue_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (mue_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
/* If there's a BPF listener, bounce a copy of this frame to him. */
#if NBPFILTER > 0
Index: sys/dev/usb/if_smsc.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_smsc.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 if_smsc.c
--- sys/dev/usb/if_smsc.c 8 Apr 2020 09:49:32 -0000 1.34
+++ sys/dev/usb/if_smsc.c 8 Jun 2020 15:02:20 -0000
@@ -649,16 +649,15 @@ smsc_start(struct ifnet *ifp)
return;
}
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (smsc_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
#if NBPFILTER > 0
if (ifp->if_bpf)
@@ -1400,6 +1399,7 @@ smsc_encap(struct smsc_softc *sc, struct
err = usbd_transfer(c->sc_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->sc_mbuf = NULL;
smsc_stop(sc);
return (EIO);
}
Index: sys/dev/usb/if_ugl.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_ugl.c,v
retrieving revision 1.23
diff -u -p -u -p -r1.23 if_ugl.c
--- sys/dev/usb/if_ugl.c 2 Oct 2018 19:49:10 -0000 1.23
+++ sys/dev/usb/if_ugl.c 8 Jun 2020 15:16:09 -0000
@@ -566,6 +566,7 @@ ugl_send(struct ugl_softc *sc, struct mb
if (err != USBD_IN_PROGRESS) {
printf("%s: ugl_send error=%s\n", sc->sc_dev.dv_xname,
usbd_errstr(err));
+ c->ugl_mbuf = NULL;
ugl_stop(sc);
return (EIO);
}
@@ -589,17 +590,15 @@ ugl_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (ugl_send(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
-
- ifq_deq_commit(&ifp->if_snd, m_head);
#if NBPFILTER > 0
/*
Index: sys/dev/usb/if_upl.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_upl.c,v
retrieving revision 1.75
diff -u -p -u -p -r1.75 if_upl.c
--- sys/dev/usb/if_upl.c 2 Oct 2018 19:49:10 -0000 1.75
+++ sys/dev/usb/if_upl.c 8 Jun 2020 15:06:43 -0000
@@ -546,6 +546,7 @@ upl_send(struct upl_softc *sc, struct mb
if (err != USBD_IN_PROGRESS) {
printf("%s: upl_send error=%s\n", sc->sc_dev.dv_xname,
usbd_errstr(err));
+ c->upl_mbuf = NULL;
upl_stop(sc);
return (EIO);
}
@@ -569,17 +570,15 @@ upl_start(struct ifnet *ifp)
if (ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (upl_send(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
-
- ifq_deq_commit(&ifp->if_snd, m_head);
#if NBPFILTER > 0
/*
Index: sys/dev/usb/if_ure.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_ure.c,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 if_ure.c
--- sys/dev/usb/if_ure.c 10 Mar 2020 01:11:30 -0000 1.14
+++ sys/dev/usb/if_ure.c 8 Jun 2020 15:12:53 -0000
@@ -775,16 +775,15 @@ ure_start(struct ifnet *ifp)
break;
}
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
break;
if (ure_encap(sc, m_head)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
break;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
#if NBPFILTER > 0
if (ifp->if_bpf)
@@ -1933,6 +1932,7 @@ ure_encap(struct ure_softc *sc, struct m
err = usbd_transfer(c->uc_xfer);
if (err != 0 && err != USBD_IN_PROGRESS) {
+ c->uc_mbuf = NULL;
ure_stop(sc);
return (EIO);
}
Index: sys/dev/usb/if_urndis.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_urndis.c,v
retrieving revision 1.69
diff -u -p -u -p -r1.69 if_urndis.c
--- sys/dev/usb/if_urndis.c 22 Jan 2019 18:06:05 -0000 1.69
+++ sys/dev/usb/if_urndis.c 8 Jun 2020 15:14:57 -0000
@@ -804,6 +804,7 @@ urndis_encap(struct urndis_softc *sc, st
/* Transmit */
err = usbd_transfer(c->sc_xfer);
if (err != USBD_IN_PROGRESS) {
+ c->sc_mbuf = NULL;
urndis_stop(sc);
return(EIO);
}
@@ -1190,16 +1191,15 @@ urndis_start(struct ifnet *ifp)
if (usbd_is_dying(sc->sc_udev) || ifq_is_oactive(&ifp->if_snd))
return;
- m_head = ifq_deq_begin(&ifp->if_snd);
+ IFQ_DEQUEUE(&ifp->if_snd, m_head);
if (m_head == NULL)
return;
if (urndis_encap(sc, m_head, 0)) {
- ifq_deq_rollback(&ifp->if_snd, m_head);
+ m_freem(m_head);
ifq_set_oactive(&ifp->if_snd);
return;
}
- ifq_deq_commit(&ifp->if_snd, m_head);
/*
* If there's a BPF listener, bounce a copy of this frame
