On 17/09/15(Thu) 02:34, Grant Czajkowski wrote:
> On Tue, Sep 15, 2015 at 12:04:35PM +0200, Martin Pieuchot wrote:
> > On 15/09/15(Tue) 04:50, Grant Czajkowski wrote:
> > > On Fri, Sep 11, 2015 at 02:41:04AM -0600, David Coppa wrote:
> > > >
> > > > Hi!
> > > >
> > > > Repeatedly hit the panic below with latest ugen.c code (v 1.88) and
> > > > pcsc-lite.
> > >
> > > Thanks for the report. Could you please try this patch?
> >
> > ok mpi@
> >
> > Grant what do you you think about doing an audit of the tree to see if
> > we're missing this check in other drivers? I might be interesting to
> > search bugs@ archives for similar reports.
>
> These are similar instances I found under /sys/dev/usb/*.
This is scary. Don't you think it's possible to handle this error code
in the stack? I think the "only" complex driver in this regard is
umass(4). By looking at its code I found this funky comment:
/*
[...]
* The control pipe has already been unstalled by the
* USB stack.
Having a single place where pipes are unstalled would make thing easier
for driver authors and simplify our like to test/fix this code.
By looking at the code around your diff I also found a lot of places where
drivers check for USBD_NOT_STARTED. This seems also wrong,
usb_transfer_complete() is only called when a transfer is aborted in which
case it has an USBD_TIMEOUT or USBD_CANCELLED status or when it finishes.
>
> Index: ubsa.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ubsa.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 ubsa.c
> --- ubsa.c 14 Mar 2015 03:38:49 -0000 1.64
> +++ ubsa.c 17 Sep 2015 02:22:47 -0000
> @@ -651,7 +651,8 @@ ubsa_intr(struct usbd_xfer *xfer, void *
>
> DPRINTF(("%s: ubsa_intr: abnormal status: %s\n",
> sc->sc_dev.dv_xname, usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
> Index: uchcom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 uchcom.c
> --- uchcom.c 14 Apr 2015 07:57:33 -0000 1.24
> +++ uchcom.c 17 Sep 2015 02:22:47 -0000
> @@ -931,7 +931,8 @@ uchcom_intr(struct usbd_xfer *xfer, void
>
> DPRINTF(("%s: abnormal status: %s\n",
> sc->sc_dev.dv_xname, usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
> DPRINTF(("%s: intr: 0x%02X 0x%02X 0x%02X 0x%02X "
> Index: ucom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ucom.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 ucom.c
> --- ucom.c 14 Mar 2015 03:38:49 -0000 1.65
> +++ ucom.c 17 Sep 2015 02:22:47 -0000
> @@ -1060,7 +1060,9 @@ ucomwritecb(struct usbd_xfer *xfer, void
>
> if (sc->sc_bulkin_pipe != NULL) {
> if (status) {
> - usbd_clear_endpoint_stall_async(sc->sc_bulkin_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(
> + sc->sc_bulkin_pipe);
> /* XXX we should restart after some delay. */
> goto error;
> }
> @@ -1145,7 +1147,9 @@ ucomreadcb(struct usbd_xfer *xfer, void
>
> if (status) {
> if (sc->sc_bulkin_pipe != NULL) {
> - usbd_clear_endpoint_stall_async(sc->sc_bulkin_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(
> + sc->sc_bulkin_pipe);
> /* XXX we should restart after some delay. */
> return;
> }
> Index: ugen.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ugen.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 ugen.c
> --- ugen.c 15 Sep 2015 13:37:44 -0000 1.89
> +++ ugen.c 17 Sep 2015 02:22:47 -0000
> @@ -721,7 +721,9 @@ ugen_do_write(struct ugen_softc *sc, int
> flags, sce->timeout, NULL);
> err = usbd_transfer(xfer);
> if (err) {
> - usbd_clear_endpoint_stall(sce->pipeh);
> + if (err == USBD_STALLED)
> + usbd_clear_endpoint_stall(sce->pipeh);
> +
> if (err == USBD_INTERRUPTED)
> error = EINTR;
> else if (err == USBD_TIMEOUT)
> Index: uhidev.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uhidev.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 uhidev.c
> --- uhidev.c 28 Feb 2015 08:42:41 -0000 1.70
> +++ uhidev.c 17 Sep 2015 02:22:47 -0000
> @@ -455,7 +455,8 @@ uhidev_intr(struct usbd_xfer *xfer, void
>
> if (status != USBD_NORMAL_COMPLETION) {
> DPRINTF(("%s: interrupt status=%d\n", DEVNAME(sc), status));
> - usbd_clear_endpoint_stall_async(sc->sc_ipipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_ipipe);
> return;
> }
>
> @@ -839,7 +840,7 @@ uhidev_write(struct uhidev_softc *sc, vo
> usbd_setup_xfer(sc->sc_owxfer, sc->sc_opipe, 0, data, len,
> USBD_SYNCHRONOUS | USBD_CATCH, 0, NULL);
> error = usbd_transfer(sc->sc_owxfer);
> - if (error)
> + if (error == USBD_STALLED)
> usbd_clear_endpoint_stall(sc->sc_opipe);
>
> return (error);
> Index: ulpt.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/ulpt.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 ulpt.c
> --- ulpt.c 9 Jul 2015 12:23:17 -0000 1.51
> +++ ulpt.c 17 Sep 2015 02:22:47 -0000
> @@ -630,7 +630,8 @@ ulpt_do_write(struct ulpt_softc *sc, str
> USBD_NO_COPY | USBD_SYNCHRONOUS | USBD_CATCH, 0, NULL);
> err = usbd_transfer(xfer);
> if (err) {
> - usbd_clear_endpoint_stall(sc->sc_out_pipe);
> + if (err == USBD_STALLED)
> + usbd_clear_endpoint_stall(sc->sc_out_pipe);
> DPRINTF(("ulptwrite: error=%d\n", err));
> error = EIO;
> break;
> @@ -700,7 +701,8 @@ ulpt_ucode_loader_hp(struct ulpt_softc *
> USBD_NO_COPY | USBD_SYNCHRONOUS, 0, NULL);
> error = usbd_transfer(xfer);
> if (error != USBD_NORMAL_COMPLETION) {
> - usbd_clear_endpoint_stall(sc->sc_out_pipe);
> + if (error == USBD_STALLED)
> + usbd_clear_endpoint_stall(sc->sc_out_pipe);
> printf("%s: ucode upload error=%s!\n",
> sc->sc_dev.dv_xname, usbd_errstr(error));
> break;
> Index: umcs.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/umcs.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 umcs.c
> --- umcs.c 14 Apr 2015 14:57:05 -0000 1.4
> +++ umcs.c 17 Sep 2015 02:22:47 -0000
> @@ -765,7 +765,8 @@ umcs_intr(struct usbd_xfer *xfer, void *
>
> if (status != USBD_NORMAL_COMPLETION) {
> DPRINTF("%s: interrupt status=%d\n", DEVNAME(sc), status);
> - usbd_clear_endpoint_stall_async(sc->sc_ipipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_ipipe);
> return;
> }
>
> Index: umct.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/umct.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 umct.c
> --- umct.c 26 Apr 2015 06:38:04 -0000 1.44
> +++ umct.c 17 Sep 2015 02:22:47 -0000
> @@ -589,7 +589,8 @@ umct_intr(struct usbd_xfer *xfer, void *
>
> DPRINTF(("%s: abnormal status: %s\n", sc->sc_dev.dv_xname,
> usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
> Index: umodem.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/umodem.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 umodem.c
> --- umodem.c 14 Mar 2015 03:38:50 -0000 1.59
> +++ umodem.c 17 Sep 2015 02:22:47 -0000
> @@ -460,7 +460,8 @@ umodem_intr(struct usbd_xfer *xfer, void
> return;
> DPRINTF(("%s: abnormal status: %s\n", sc->sc_dev.dv_xname,
> usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_notify_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_notify_pipe);
> return;
> }
>
> Index: umsm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/umsm.c,v
> retrieving revision 1.103
> diff -u -p -r1.103 umsm.c
> --- umsm.c 5 May 2015 10:17:09 -0000 1.103
> +++ umsm.c 17 Sep 2015 02:22:47 -0000
> @@ -514,7 +514,8 @@ umsm_intr(struct usbd_xfer *xfer, void *
>
> DPRINTF(("%s: umsm_intr: abnormal status: %s\n",
> sc->sc_dev.dv_xname, usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
> @@ -770,7 +771,8 @@ umsm_umass_changemode(struct umsm_softc
> USBD_NO_COPY | USBD_SYNCHRONOUS, 0, NULL);
> err = usbd_transfer(xfer);
> if (err) {
> - usbd_clear_endpoint_stall(cmdpipe);
> + if (err == USBD_STALLED)
> + usbd_clear_endpoint_stall(cmdpipe);
> DPRINTF(("%s: send error:%s", __func__,
> usbd_errstr(err)));
> }
> Index: uplcom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uplcom.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 uplcom.c
> --- uplcom.c 14 Mar 2015 03:38:50 -0000 1.66
> +++ uplcom.c 17 Sep 2015 02:22:47 -0000
> @@ -753,7 +753,8 @@ uplcom_intr(struct usbd_xfer *xfer, void
>
> DPRINTF(("%s: abnormal status: %s\n", sc->sc_dev.dv_xname,
> usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
> Index: uticom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uticom.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 uticom.c
> --- uticom.c 14 Mar 2015 03:38:50 -0000 1.27
> +++ uticom.c 17 Sep 2015 02:22:47 -0000
> @@ -790,7 +790,8 @@ uticom_intr(struct usbd_xfer *xfer, void
>
> DPRINTF(("%s: uticom_intr: abnormal status: %s\n",
> sc->sc_dev.dv_xname, usbd_errstr(status)));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
> Index: uvscom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvscom.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 uvscom.c
> --- uvscom.c 14 Mar 2015 03:38:50 -0000 1.33
> +++ uvscom.c 17 Sep 2015 02:22:47 -0000
> @@ -782,7 +782,8 @@ uvscom_intr(struct usbd_xfer *xfer, void
> printf("%s: uvscom_intr: abnormal status: %s\n",
> sc->sc_dev.dv_xname,
> usbd_errstr(status));
> - usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> + if (status == USBD_STALLED)
> + usbd_clear_endpoint_stall_async(sc->sc_intr_pipe);
> return;
> }
>
