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>
> > > > > 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.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
>
