kern/119675: [acpi] apic_hpet0 probe causes divide by zero kernel panic

[John Baldwin]

On Tuesday 15 January 2008 11:40:04 am Leo Bicknell wrote:
> The following reply was made to PR kern/119675; it has been noted by GNATS.
> 
> From: Leo Bicknell <bicknell at ufp.org>
> To: John Baldwin <jhb at FreeBSD.org>
> Cc: bug-followup at FreeBSD.org
> Subject: Re: kern/119675: [acpi] apic_hpet0 probe causes divide by zero kernel panic
> Date: Tue, 15 Jan 2008 11:33:14 -0500
> 
>  In a message written on Tue, Jan 15, 2008 at 10:13:07AM -0500, John Baldwin wrote:
>  > You can try the patch below.  It fixes a couple of places where we don't
>  > honor the spec (we don't shut it off in S1 and S2 as required and we don't
>  > preserve reserved bits in the global configuration register).  It also
>  > fails the attach if the period is zero which should fix your panic and
>  > just leave you with no HPET.
>  
>  Good news and bad news.
>  
>  With the patch "invalid period" is printed out, so I believe it's
>  correctly detecting the hpet0 issue.
>  
>  However, I immediately get an "integer divide fault while in kernel
>  mode" panic and the boot still fails.  I tried with boot -v and the
>  message is right after the "invalid period", so I'm not quite sure
>  what's causing it.
>  
>  Any recomendations, other than setting up a kernel debugger to see where
>  it's coming from?

I forgot to include a return() statement to make it actually fail the attach.
My bad.  Updated patch below:

--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_hpet.c	2007/10/09 07:52:34
+++ //depot/user/jhb/acpipci/dev/acpica/acpi_hpet.c	2008/01/15 17:42:12
@@ -82,6 +82,24 @@
 	return (bus_read_4(sc->mem_res, HPET_OFFSET_VALUE));
 }
 
+static void
+hpet_enable(struct acpi_hpet_softc *sc)
+{
+	uint32_t val;
+	
+	val = bus_read_4(sc->mem_res, HPET_OFFSET_ENABLE);
+	bus_write_4(sc->mem_res, HPET_OFFSET_ENABLE, val | 1);
+}
+
+static void
+hpet_disable(struct acpi_hpet_softc *sc)
+{
+	uint32_t val;
+	
+	val = bus_read_4(sc->mem_res, HPET_OFFSET_ENABLE);
+	bus_write_4(sc->mem_res, HPET_OFFSET_ENABLE, val & ~1);
+}
+
 /* Discover the HPET via the ACPI table of the same name. */
 static void 
 acpi_hpet_identify(driver_t *driver, device_t parent)
@@ -166,10 +184,17 @@
 	}
 
 	/* Be sure timer is enabled. */
-	bus_write_4(sc->mem_res, HPET_OFFSET_ENABLE, 1);
+	hpet_enable(sc);
 
 	/* Read basic statistics about the timer. */
 	val = bus_read_4(sc->mem_res, HPET_OFFSET_PERIOD);
+	if (val == 0) {
+		device_printf(dev, "invalid period\n");
+		hpet_disable(sc);
+		bus_free_resource(dev, SYS_RES_MEMORY, sc->mem_res);
+		return (ENXIO);
+	}
+
 	freq = (1000000000000000LL + val / 2) / val;
 	if (bootverbose) {
 		val = bus_read_4(sc->mem_res, HPET_OFFSET_INFO);
@@ -192,7 +217,7 @@
 	val2 = bus_read_4(sc->mem_res, HPET_OFFSET_VALUE);
 	if (val == val2) {
 		device_printf(dev, "HPET never increments, disabling\n");
-		bus_write_4(sc->mem_res, HPET_OFFSET_ENABLE, 0);
+		hpet_disable(sc);
 		bus_free_resource(dev, SYS_RES_MEMORY, sc->mem_res);
 		return (ENXIO);
 	}
@@ -214,13 +239,29 @@
 }
 
 static int
+acpi_hpet_suspend(device_t dev)
+{
+	struct acpi_hpet_softc *sc;
+
+	/*
+	 * Disable the timer during suspend.  The timer will not lose
+	 * its state in S1 or S2, but we are required to disable
+	 * it.
+	 */
+	sc = device_get_softc(dev);
+	hpet_disable(sc);
+
+	return (0);
+}
+
+static int
 acpi_hpet_resume(device_t dev)
 {
 	struct acpi_hpet_softc *sc;
 
 	/* Re-enable the timer after a resume to keep the clock advancing. */
 	sc = device_get_softc(dev);
-	bus_write_4(sc->mem_res, HPET_OFFSET_ENABLE, 1);
+	hpet_enable(sc);
 
 	return (0);
 }
@@ -260,6 +301,7 @@
 	DEVMETHOD(device_probe, acpi_hpet_probe),
 	DEVMETHOD(device_attach, acpi_hpet_attach),
 	DEVMETHOD(device_detach, acpi_hpet_detach),
+	DEVMETHOD(device_suspend, acpi_hpet_suspend),
 	DEVMETHOD(device_resume, acpi_hpet_resume),
 
 	{0, 0}

-- 
John Baldwin