Re: RE: Reboot on "shutdown -r" hangs after final "uptime ..." string

On 11 Aug, Don Lewis wrote:
> On 11 Aug, To: bruce_at_cran.org.uk wrote:
>> On 12 Aug, bruce_at_cran.org.uk wrote:
> 
>> Something that you might want to also print is the value of
>> 	EOREAD4(sc, EHCI_USBSTS)
>> immediately before the
>> 	EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
>> call in ehci_hcreset().  Bit 12 of this register (EHCI_STS_HCH) should
>> be a zero before the write to do the reset is performed.
> 
> Correction, this bit should be a one before doing the reset.
> 
>> Maybe
>> ehci_hcreset() needs to wait for that to happen (as is done in
>> ehci_init()), rather than just waiting a fixed amount of time.
> 
> I misread the code in ehci_init(), which is waiting for a transition in
> the opposite direction.  I don't know why, because the EHCI spec says
> that the 1->0 transition is immediate.

A revised version of the patch is below.  The only change is the
addition of a second wait loop in ehci_hcreset(), which would be
necessary according the the EHCI spec if the controller could ever
require more than 1ms to stop.

This patch might fix the problem on your machine if you are observing a
zero for bit 12 (EHCI_STS_HCH) of the EHCI_USBSTS register in
ehci_hcreset().

Index: sys/dev/usb/ehci.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.55
diff -u -r1.55 ehci.c
--- sys/dev/usb/ehci.c	20 Jun 2007 05:10:52 -0000	1.55
+++ sys/dev/usb/ehci.c	12 Aug 2007 03:55:54 -0000
_at__at_ -311,6 +311,39 _at__at_
 	ehci_device_isoc_done,
 };
 
+static usbd_status
+ehci_hcreset(ehci_softc_t *sc)
+{
+	u_int32_t hcr;
+	u_int i;
+
+	EOWRITE4(sc, EHCI_USBCMD, 0);	/* Halt controller */
+	for (i = 0; i < 100; i++) {
+		usb_delay_ms(&sc->sc_bus, 1);
+		hcr = EOREAD4(sc, EHCI_USBSTS) & EHCI_STS_HCH;
+		if (hcr)
+			break;
+	}
+	if (!hcr)
+		/*
+                 * Fall through and try reset anyway even though
+                 * Table 2-9 in the EHCI spec says this will result
+                 * in undefined behavior.
+                 */
+		printf("%s: stop timeout\n",
+		       device_get_nameunit(sc->sc_bus.bdev));
+
+	EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
+	for (i = 0; i < 100; i++) {
+		usb_delay_ms(&sc->sc_bus, 1);
+		hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
+		if (!hcr)
+			return (USBD_NORMAL_COMPLETION);
+	}
+	printf("%s: reset timeout\n", device_get_nameunit(sc->sc_bus.bdev));
+	return (USBD_IOERROR);
+}
+
 usbd_status
 ehci_init(ehci_softc_t *sc)
 {
_at__at_ -365,20 +398,9 _at__at_
 
 	/* Reset the controller */
 	DPRINTF(("%s: resetting\n", device_get_nameunit(sc->sc_bus.bdev)));
-	EOWRITE4(sc, EHCI_USBCMD, 0);	/* Halt controller */
-	usb_delay_ms(&sc->sc_bus, 1);
-	EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
-	for (i = 0; i < 100; i++) {
-		usb_delay_ms(&sc->sc_bus, 1);
-		hcr = EOREAD4(sc, EHCI_USBCMD) & EHCI_CMD_HCRESET;
-		if (!hcr)
-			break;
-	}
-	if (hcr) {
-		printf("%s: reset timeout\n",
-		    device_get_nameunit(sc->sc_bus.bdev));
-		return (USBD_IOERROR);
-	}
+	err = ehci_hcreset(sc);
+	if (err != USBD_NORMAL_COMPLETION)
+		return (err);
 
 	/* frame list size at default, read back what we got and use that */
 	switch (EHCI_CMD_FLS(EOREAD4(sc, EHCI_USBCMD))) {
_at__at_ -927,8 +949,7 _at__at_
 	sc->sc_dying = 1;
 
 	EOWRITE4(sc, EHCI_USBINTR, sc->sc_eintrs);
-	EOWRITE4(sc, EHCI_USBCMD, 0);
-	EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
+	(void) ehci_hcreset(sc);
 	callout_stop(&sc->sc_tmo_intrlist);
 	callout_stop(&sc->sc_tmo_pcd);
 
_at__at_ -1090,8 +1111,7 _at__at_
 	ehci_softc_t *sc = v;
 
 	DPRINTF(("ehci_shutdown: stopping the HC\n"));
-	EOWRITE4(sc, EHCI_USBCMD, 0);	/* Halt controller */
-	EOWRITE4(sc, EHCI_USBCMD, EHCI_CMD_HCRESET);
+	(void) ehci_hcreset(sc);
 }
 
 usbd_status


If this doesn't help, you could test the SMI hypothesis by disabling the
SMI on OS Ownership Enable bit before clearing the EHCI_LEGSUP_OSOWNED
bit.  Something like the following should disable the SMI enable bit,
but I don't know what side effects it might have.  It could even melt
your computer ;-)  In any case, it's not a proper fix.

static void
ehci_pci_givecontroller(device_t self)
{
        ehci_softc_t *sc = device_get_softc(self);
        u_int32_t cparams, eec, legsup;
        int eecp;

        cparams = EREAD4(sc, EHCI_HCCPARAMS);
        for (eecp = EHCI_HCC_EECP(cparams); eecp != 0;
            eecp = EHCI_EECP_NEXT(eec)) {
                eec = pci_read_config(self, eecp, 4);
                if (EHCI_EECP_ID(eec) != EHCI_EC_LEGSUP)
                        continue;
/* NEW */       legsup = pci_read_config(self,eecp+EHCI_LEGSUP_USBLEGCTLSTS,4);
/* NEW */       legsup = legsup & ~0x200;
/* NEW */       pci_write_config(self, eecp+EHCI_LEGSUP_USBLEGCTLSTS, legsup,4);
                legsup = eec;
                pci_write_config(self, eecp, legsup & ~EHCI_LEGSUP_OSOWNED, 4);
        }
}