> Date: Wed, 17 Dec 2014 14:48:27 +0100
> From: Martin Pieuchot <mpieuc...@nolizard.org>
> 
> Hello Tilo,
> 
> 
> Thanks for the report, diff below fixes it for me, could you try it?
> 
> Index: ehci.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/usb/ehci.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 ehci.c
> --- ehci.c    9 Dec 2014 07:05:06 -0000       1.171
> +++ ehci.c    17 Dec 2014 12:25:00 -0000
> @@ -542,8 +542,8 @@ ehci_intr1(struct ehci_softc *sc)
>       sc->sc_bus.intr_context++;
>       sc->sc_bus.no_intrs++;
>       if (eintrs & EHCI_STS_HSE) {
> -             printf("%s: unrecoverable error, controller halted\n",
> -                    sc->sc_bus.bdev.dv_xname);
> +             printf("%s: host controller halted\n",
> +                 sc->sc_bus.bdev.dv_xname);
>               sc->sc_bus.dying = 1;
>               sc->sc_bus.intr_context--;
>               return (1);

Martin, I think the real bug is slightly above this chunk.  Upon
surprise unplug of the controller, we're supposed to hit the following
if-statement:

        intrs = EHCI_STS_INTRS(EOREAD4(sc, EHCI_USBSTS));
        if (intrs == 0xffffffff) {
                sc->sc_bus.dying = 1;
                return (0);
        }

But if you look at the definition of EHCI_STS_INTRS(), you quickly
realize that code is unreachable.  There's a similar problem in uhci(4).

The idea here is that reading a register from hardware that isn't
present will return 0xffffffff.  It's best to check as many bits as
possible to distinguish this condition from the case where the
hardware genuinly has these bits set.

Untested diff below attempts to fix this.

Index: ehci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.171
diff -u -p -r1.171 ehci.c
--- ehci.c      9 Dec 2014 07:05:06 -0000       1.171
+++ ehci.c      17 Dec 2014 14:30:16 -0000
@@ -524,11 +524,12 @@ ehci_intr1(struct ehci_softc *sc)
                return (0);
        }
 
-       intrs = EHCI_STS_INTRS(EOREAD4(sc, EHCI_USBSTS));
+       intrs = EOREAD4(sc, EHCI_USBSTS);
        if (intrs == 0xffffffff) {
                sc->sc_bus.dying = 1;
                return (0);
        }
+       intrs = EHCI_STS_INTRS(intrs);
        if (!intrs)
                return (0);
 
Index: uhci.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uhci.c,v
retrieving revision 1.133
diff -u -p -r1.133 uhci.c
--- uhci.c      9 Dec 2014 07:05:06 -0000       1.133
+++ uhci.c      17 Dec 2014 14:30:16 -0000
@@ -1020,13 +1020,14 @@ uhci_intr1(struct uhci_softc *sc)
        int status;
        int ack;
 
-       status = UREAD2(sc, UHCI_STS) & UHCI_STS_ALLINTRS;
-       if (status == 0)        /* The interrupt was not for us. */
-               return (0);
-       if (status == 0xffffffff) {
+       status = UREAD2(sc, UHCI_STS);
+       if (status == 0xffff) {
                sc->sc_bus.dying = 1;
                return (0);
        }
+       status &= UHCI_STS_ALLINTRS;
+       if (status == 0)        /* The interrupt was not for us. */
+               return (0);
 
 #ifdef UHCI_DEBUG
        if (uhcidebug > 15) {

Reply via email to