Re: usbd_abort_pipe(); usbd_close_pipe; dance
On 31/07/20(Fri) 11:22, Marcus Glocker wrote:
> Maybe I'm missing something here.
You aren't. Historically usbd_close_pipe() wasn't aborting transfers.
We changed it to do so as it happened to be the easiest fix to some
issues that had been copy/pasted.
It's just that nobody took the time to do the cleanup you're now
suggesting, thanks!
> But is there any specific reason why the most of our USB drivers are
> calling usbd_abort_pipe() right before usbd_close_pipe()? Since
> usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
> empty, as documented in the man page:
>
> DESCRIPTION
> The usbd_abort_pipe() function aborts any transfers queued on pipe.
>
> The usbd_close_pipe() function aborts any transfers queued on pipe
> then deletes it.
>
> In case this happened because of an inherited copy/paste chain, can we
> nuke the superfluous usbd_abort_pipe() calls?
Yes please, ok mpi@
> Index: if_atu.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
> retrieving revision 1.131
> diff -u -p -u -p -r1.131 if_atu.c
> --- if_atu.c 10 Jul 2020 13:26:40 - 1.131
> +++ if_atu.c 31 Jul 2020 08:26:24 -
> @@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
>
> /* Stop transfers. */
> if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> if (err) {
> DPRINTF(("%s: close rx pipe failed: %s\n",
> @@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
> }
>
> if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> if (err) {
> DPRINTF(("%s: close tx pipe failed: %s\n",
> Index: if_aue.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> retrieving revision 1.110
> diff -u -p -u -p -r1.110 if_aue.c
> --- if_aue.c 10 Jul 2020 13:26:40 - 1.110
> +++ if_aue.c 31 Jul 2020 08:26:25 -
> @@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
>
> /* Stop transfers. */
> if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> if (err) {
> printf("%s: close rx pipe failed: %s\n",
> @@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
> }
>
> if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> if (err) {
> printf("%s: close tx pipe failed: %s\n",
> @@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
> }
>
> if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
> - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> if (err) {
> printf("%s: close intr pipe failed: %s\n",
> Index: if_axe.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
> retrieving revision 1.141
> diff -u -p -u -p -r1.141 if_axe.c
> --- if_axe.c 10 Jul 2020 13:26:40 - 1.141
> +++ if_axe.c 31 Jul 2020 08:26:25 -
> @@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
>
> /* Stop transfers. */
> if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> if (err) {
> printf("axe%d: close rx pipe failed: %s\n",
> @@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
> }
>
> if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> if (err) {
> printf("axe%d: close tx pipe failed: %s\n",
> @@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
> }
>
> if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
> - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> if (err) {
> printf("axe%d: close intr pipe failed: %s\n",
> Index: if_axen.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
> retrieving revision 1.29
> diff -u -p -u -p -r1.29 if_axen.c
> --- if_axen.c 10 Jul 2020 13:26:40 - 1.29
> +++ if_axen.c 31 Jul 2020 08:26:25 -
> @@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
>
> /* Stop transfers. */
> if (sc->axen_ep[AXEN_
Re: usbd_abort_pipe(); usbd_close_pipe; dance
On Fri, 31 Jul 2020 11:59:45 +0200
Gerhard Roth wrote:
> Hi Marcus,
>
> On 2020-07-31 11:22, Marcus Glocker wrote:
> > Maybe I'm missing something here.
> >
> > But is there any specific reason why the most of our USB drivers are
> > calling usbd_abort_pipe() right before usbd_close_pipe()? Since
> > usbd_close_pipe() already will call usbd_abort_pipe() if the pipe
> > isn't empty, as documented in the man page:
> >
> > DESCRIPTION
> > The usbd_abort_pipe() function aborts any transfers queued on
> > pipe.
> >
> > The usbd_close_pipe() function aborts any transfers queued on
> > pipe then deletes it.
> >
> > In case this happened because of an inherited copy/paste chain, can
> > we nuke the superfluous usbd_abort_pipe() calls?
>
> I was asking myself the same question before ;)
>
> Apparently, the call to usbd_abort_pipe() inside of usbd_close_pipe()
> wasn't there right from the start. It was added by mpi@ with rev 1.57
> of usbdi.c abort 7 years ago. Hence drivers written before that
> needed the the usbd_abort_pipe(). But that is no longer the case.
Ah right, that explains it then - Thanks!
> ok gerhard@
>
>
>
>
> >
> >
> > Index: if_atu.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
> > retrieving revision 1.131
> > diff -u -p -u -p -r1.131 if_atu.c
> > --- if_atu.c10 Jul 2020 13:26:40 - 1.131
> > +++ if_atu.c31 Jul 2020 08:26:24 -
> > @@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
> >
> > /* Stop transfers. */
> > if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> > err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
> > if (err) {
> > DPRINTF(("%s: close rx pipe failed: %s\n",
> > @@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
> > }
> >
> > if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> > err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
> > if (err) {
> > DPRINTF(("%s: close tx pipe failed: %s\n",
> > Index: if_aue.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> > retrieving revision 1.110
> > diff -u -p -u -p -r1.110 if_aue.c
> > --- if_aue.c10 Jul 2020 13:26:40 - 1.110
> > +++ if_aue.c31 Jul 2020 08:26:25 -
> > @@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
> >
> > /* Stop transfers. */
> > if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
> > if (err) {
> > printf("%s: close rx pipe failed: %s\n",
> > @@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
> > }
> >
> > if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
> > if (err) {
> > printf("%s: close tx pipe failed: %s\n",
> > @@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
> > }
> >
> > if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
> > - usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> > err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
> > if (err) {
> > printf("%s: close intr pipe failed: %s\n",
> > Index: if_axe.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
> > retrieving revision 1.141
> > diff -u -p -u -p -r1.141 if_axe.c
> > --- if_axe.c10 Jul 2020 13:26:40 - 1.141
> > +++ if_axe.c31 Jul 2020 08:26:25 -
> > @@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
> >
> > /* Stop transfers. */
> > if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
> > if (err) {
> > printf("axe%d: close rx pipe failed:
> > %s\n", @@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
> > }
> >
> > if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
> > if (err) {
> > printf("axe%d: close tx pipe failed:
> > %s\n", @@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
> > }
> >
> > if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
> > - usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> > err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
> > if (err) {
> > printf("axe%d: close intr pipe failed:
> > %s\n", Index: if_axen.c
> > ===
Re: usbd_abort_pipe(); usbd_close_pipe; dance
Hi Marcus,
On 2020-07-31 11:22, Marcus Glocker wrote:
Maybe I'm missing something here.
But is there any specific reason why the most of our USB drivers are
calling usbd_abort_pipe() right before usbd_close_pipe()? Since
usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
empty, as documented in the man page:
DESCRIPTION
The usbd_abort_pipe() function aborts any transfers queued on pipe.
The usbd_close_pipe() function aborts any transfers queued on pipe
then deletes it.
In case this happened because of an inherited copy/paste chain, can we
nuke the superfluous usbd_abort_pipe() calls?
I was asking myself the same question before ;)
Apparently, the call to usbd_abort_pipe() inside of usbd_close_pipe()
wasn't there right from the start. It was added by mpi@ with rev 1.57 of
usbdi.c abort 7 years ago. Hence drivers written before that needed the
the usbd_abort_pipe(). But that is no longer the case.
ok gerhard@
Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 if_atu.c
--- if_atu.c10 Jul 2020 13:26:40 - 1.131
+++ if_atu.c31 Jul 2020 08:26:24 -
@@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
/* Stop transfers. */
if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
if (err) {
DPRINTF(("%s: close rx pipe failed: %s\n",
@@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
}
if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
if (err) {
DPRINTF(("%s: close tx pipe failed: %s\n",
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.110
diff -u -p -u -p -r1.110 if_aue.c
--- if_aue.c10 Jul 2020 13:26:40 - 1.110
+++ if_aue.c31 Jul 2020 08:26:25 -
@@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
/* Stop transfers. */
if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
if (err) {
printf("%s: close rx pipe failed: %s\n",
@@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
if (err) {
printf("%s: close tx pipe failed: %s\n",
@@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
if (err) {
printf("%s: close intr pipe failed: %s\n",
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 if_axe.c
--- if_axe.c10 Jul 2020 13:26:40 - 1.141
+++ if_axe.c31 Jul 2020 08:26:25 -
@@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
/* Stop transfers. */
if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
if (err) {
printf("axe%d: close rx pipe failed: %s\n",
@@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
if (err) {
printf("axe%d: close tx pipe failed: %s\n",
@@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
if (err) {
printf("axe%d: close intr pipe failed: %s\n",
Index: if_axen.c
===
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 if_axen.c
--- if_axen.c 10 Jul 2020 13:26:40 - 1.29
+++ if_axen.c 31 Jul 2020 08:26:25 -
@@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
/* Stop transfers. */
if (sc->axen_ep[AXEN_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->a
usbd_abort_pipe(); usbd_close_pipe; dance
Maybe I'm missing something here.
But is there any specific reason why the most of our USB drivers are
calling usbd_abort_pipe() right before usbd_close_pipe()? Since
usbd_close_pipe() already will call usbd_abort_pipe() if the pipe isn't
empty, as documented in the man page:
DESCRIPTION
The usbd_abort_pipe() function aborts any transfers queued on pipe.
The usbd_close_pipe() function aborts any transfers queued on pipe
then deletes it.
In case this happened because of an inherited copy/paste chain, can we
nuke the superfluous usbd_abort_pipe() calls?
Index: if_atu.c
===
RCS file: /cvs/src/sys/dev/usb/if_atu.c,v
retrieving revision 1.131
diff -u -p -u -p -r1.131 if_atu.c
--- if_atu.c10 Jul 2020 13:26:40 - 1.131
+++ if_atu.c31 Jul 2020 08:26:24 -
@@ -2252,7 +2252,6 @@ atu_stop(struct ifnet *ifp, int disable)
/* Stop transfers. */
if (sc->atu_ep[ATU_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_RX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_RX]);
if (err) {
DPRINTF(("%s: close rx pipe failed: %s\n",
@@ -2262,7 +2261,6 @@ atu_stop(struct ifnet *ifp, int disable)
}
if (sc->atu_ep[ATU_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->atu_ep[ATU_ENDPT_TX]);
err = usbd_close_pipe(sc->atu_ep[ATU_ENDPT_TX]);
if (err) {
DPRINTF(("%s: close tx pipe failed: %s\n",
Index: if_aue.c
===
RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
retrieving revision 1.110
diff -u -p -u -p -r1.110 if_aue.c
--- if_aue.c10 Jul 2020 13:26:40 - 1.110
+++ if_aue.c31 Jul 2020 08:26:25 -
@@ -1518,7 +1518,6 @@ aue_stop(struct aue_softc *sc)
/* Stop transfers. */
if (sc->aue_ep[AUE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_RX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_RX]);
if (err) {
printf("%s: close rx pipe failed: %s\n",
@@ -1528,7 +1527,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_TX]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_TX]);
if (err) {
printf("%s: close tx pipe failed: %s\n",
@@ -1538,7 +1536,6 @@ aue_stop(struct aue_softc *sc)
}
if (sc->aue_ep[AUE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
err = usbd_close_pipe(sc->aue_ep[AUE_ENDPT_INTR]);
if (err) {
printf("%s: close intr pipe failed: %s\n",
Index: if_axe.c
===
RCS file: /cvs/src/sys/dev/usb/if_axe.c,v
retrieving revision 1.141
diff -u -p -u -p -r1.141 if_axe.c
--- if_axe.c10 Jul 2020 13:26:40 - 1.141
+++ if_axe.c31 Jul 2020 08:26:25 -
@@ -1473,7 +1473,6 @@ axe_stop(struct axe_softc *sc)
/* Stop transfers. */
if (sc->axe_ep[AXE_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_RX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_RX]);
if (err) {
printf("axe%d: close rx pipe failed: %s\n",
@@ -1483,7 +1482,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_TX]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_TX]);
if (err) {
printf("axe%d: close tx pipe failed: %s\n",
@@ -1493,7 +1491,6 @@ axe_stop(struct axe_softc *sc)
}
if (sc->axe_ep[AXE_ENDPT_INTR] != NULL) {
- usbd_abort_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
err = usbd_close_pipe(sc->axe_ep[AXE_ENDPT_INTR]);
if (err) {
printf("axe%d: close intr pipe failed: %s\n",
Index: if_axen.c
===
RCS file: /cvs/src/sys/dev/usb/if_axen.c,v
retrieving revision 1.29
diff -u -p -u -p -r1.29 if_axen.c
--- if_axen.c 10 Jul 2020 13:26:40 - 1.29
+++ if_axen.c 31 Jul 2020 08:26:25 -
@@ -1426,7 +1426,6 @@ axen_stop(struct axen_softc *sc)
/* Stop transfers. */
if (sc->axen_ep[AXEN_ENDPT_RX] != NULL) {
- usbd_abort_pipe(sc->axen_ep[AXEN_ENDPT_RX]);
err = usbd_close_pipe(sc->axen_ep[AXEN_ENDPT_RX]);
if (err) {
printf("axen%d: close rx pipe failed: %s\n",
@@ -1436,7 +1435,6 @@ axen_stop(struct axen_softc *sc)
}
if (sc->axen_ep[AXEN_ENDPT_TX] != NULL) {
- usbd_abort_pipe(sc->axen_ep[AXEN_ENDPT_TX]);
