Re: OpenBSD 6.7 crashes on APU2C4 with LTE modem Huawei E3372s-153 HiLink

Mon, 15 Jun 2020 11:10:23 -0700 

On 2020-06-13 01:24, Łukasz Lejtkowski wrote:

Good news - no more kernel panics on USB 3.0(xHCI), it’s fixed.
Bad news - after 2-3h LTE modem lost local network connection via USB 3.0(cdce0). I have to remove modem and put it back to usb port - then local network connection between OpenBSD and modem back for 2-3h, sometimes 30-40 min. It looks like the same problem as kernel panic, but this time there is lost network connection via usb 3.0(xhci). 

root@master[~]ping 192.168.8.1
PING 192.168.8.1 (192.168.8.1): 56 data bytes
ping: sendmsg: Network is down
ping: wrote 192.168.8.1 64 chars, ret=-1
ping: sendmsg: Network is down
ping: wrote 192.168.8.1 64 chars, ret=-1

192.168.8.1 is default static IP on lte modem.

Your changes in if_cdce.c 1.77 not completely fix the problem.


Hi,
yes, my patch just targeted to fix the panic as a reaction to USB problems; not the USB problems themself. 

Does it recover after doing

        # ifconfig cdcef0 down
        # ifconfig cdcef0 up

Gerhard
On 11 Jun 2020, at 11:13, Łukasz Lejtkowski <emig...@gmail.com <mailto:emig...@gmail.com>> wrote: 

Hi Gerhard,
Today I added Your patches to 6.7-stable and moved back LTE modem to USB 3.0. So, just waiting for… nothing or kernel panic. I’ll let you know.
On 8 Jun 2020, at 19:13, Patrick Wildt <patr...@blueri.se <mailto:patr...@blueri.se>> wrote: 

On Mon, Jun 08, 2020 at 05:31:44PM +0200, Gerhard Roth wrote:

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 <mailto: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



From what I remember from various discussions, the goal should be to
check if there's a buffer free in the ring, then dequeue and send, and
it it can't be sent out, then drop it.  With USB apparently those
drivers "always" have an open buffer, so we can just dequeue and send,
like you do in this diff.  And if it gets dropped, that's fine.

That said, I think IFQ_DEQUEUE() is old compat code, and we actually
nowadays prefer:

m_head = ifq_dequeue(&ifp->if_snd);

If you look at the define for IFQ_DEQUEUE() you'll see it's marked
as compat code.  If you look at a new driver, like ixl(4), you'll
see that it also uses ifq_dequeue().

Sorry to to give you some work, but with that fixed: ok patrick@

Patrick



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.c7 Jul 2019 06:40:10 -00001.139
+++ sys/dev/usb/if_axe.c8 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.c7 Jul 2019 06:40:10 -00001.27
+++ sys/dev/usb/if_axen.c8 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.c14 Nov 2019 13:50:55 -00001.76
+++ sys/dev/usb/if_cdce.c8 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.c2 Oct 2018 19:49:10 -00001.89
+++ sys/dev/usb/if_kue.c8 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.c7 Jul 2019 06:40:10 -00001.40
+++ sys/dev/usb/if_mos.c8 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.c7 Jul 2019 06:40:10 -00001.7
+++ sys/dev/usb/if_mue.c8 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.c8 Apr 2020 09:49:32 -00001.34
+++ sys/dev/usb/if_smsc.c8 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.c2 Oct 2018 19:49:10 -00001.23
+++ sys/dev/usb/if_ugl.c8 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.c2 Oct 2018 19:49:10 -00001.75
+++ sys/dev/usb/if_upl.c8 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.c10 Mar 2020 01:11:30 -00001.14
+++ sys/dev/usb/if_ure.c8 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.c22 Jan 2019 18:06:05 -00001.69
+++ sys/dev/usb/if_urndis.c8 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

Reply via email to