On 17/12/14(Wed) 15:34, Mark Kettenis wrote:
> > Date: Wed, 17 Dec 2014 14:48:27 +0100
> > From: Martin Pieuchot <[email protected]>
> >
> > 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.
Thanks Mark. That was indeed my first though. I did some printf()
debugging and surprisingly this condition was never true... But I
didn't pay attention to the mask!
> Untested diff below attempts to fix this.
This works as expected and the "host controller halted" messages are
now gone, ok mpi@.
Also you still need the usb.c chunk since "dying" is also set in this
case to fix the reported bug. I'll commit it afterward.
> 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) {
>
