Re: Missing watchdog after ACPI watchdog creation failure

[Mika Westerberg]

On Thu, Jan 18, 2018 at 12:20:32PM +0200, Mika Westerberg wrote:
> On Wed, Jan 17, 2018 at 12:53:41PM +0100, Takashi Iwai wrote:
> > Unfortunately we couldn't get approval yet, since it's a prototype
> > machine.
> 
> In that case, I think the system itself and its ACPI tables should be
> fixed if possible. Windows relies on that table as well so unless there
> is something terribly wrong in how we allocate resources in Linux,
> Windows should fail the same way. There is good reason why the WDAT
> table is there in the first place so using iTCO to poke the hardware
> directly might cause some other problems. Windows does not have iTCO
> driver at all.
> 
> > Meanwhile, the reporter tested the patch below and confirmed to work.
> > (It might be racy for acpi_has_watchdog() call during the probe, but
> >  you see the idea.)
> 
> I would rather not to add any kinds of quirks for systems that are still
> in development phase and the BIOS can be fixed. Basic idea is that if
> the WDAT table is there we expect it to be correct and at least the
> systems I'm aware of that's the case.
> 
> Of course if it turns out to be a problem in a real production system we
> need to find out what the actual problem is (i.e why the resource
> allocation fails in the first place) and fix it there.
> 
> That said, if Rafael says we should still add the check, I'll make a
> patch that does it (based on yours) and send it upstream :)

However, we can still check if the WDAT is actually enabled and prevent
creation of the device in that case. It may be that the BIOS always
exposes the table but the device itself is disabled.

Can you ask the reporter to try the below patch and see if it helps?

diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
index 11b113f8e367..e785a1ae57c8 100644
--- a/drivers/acpi/acpi_watchdog.c
+++ b/drivers/acpi/acpi_watchdog.c
@@ -17,18 +17,34 @@
 
 #include "internal.h"
 
+static const struct acpi_table_wdat * acpi_watchdog_get_wdat(void)
+{
+	const struct acpi_table_wdat *wdat = NULL;
+	acpi_status status;
+
+	status = acpi_get_table(ACPI_SIG_WDAT, 0,
+				(struct acpi_table_header **)&wdat);
+	if (ACPI_FAILURE(status)) {
+		/* It is fine if there is no WDAT */
+		return NULL;
+	}
+
+	return wdat;
+}
+
 /**
  * Returns true if this system should prefer ACPI based watchdog instead of
  * the native one (which are typically the same hardware).
  */
 bool acpi_has_watchdog(void)
 {
-	struct acpi_table_header hdr;
+	const struct acpi_table_wdat *wdat;
 
 	if (acpi_disabled)
 		return false;
 
-	return ACPI_SUCCESS(acpi_get_table_header(ACPI_SIG_WDAT, 0, &hdr));
+	wdat = acpi_watchdog_get_wdat();
+	return wdat && (wdat->flags & ACPI_WDAT_ENABLED);
 }
 EXPORT_SYMBOL_GPL(acpi_has_watchdog);
 
@@ -41,18 +57,11 @@ void __init acpi_watchdog_init(void)
 	struct platform_device *pdev;
 	struct resource *resources;
 	size_t nresources = 0;
-	acpi_status status;
 	int i;
 
-	status = acpi_get_table(ACPI_SIG_WDAT, 0,
-				(struct acpi_table_header **)&wdat);
-	if (ACPI_FAILURE(status)) {
-		/* It is fine if there is no WDAT */
-		return;
-	}
-
+	wdat = acpi_watchdog_get_wdat();
 	/* Watchdog disabled by BIOS */
-	if (!(wdat->flags & ACPI_WDAT_ENABLED))
+	if (!wdat || !(wdat->flags & ACPI_WDAT_ENABLED))
 		return;
 
 	/* Skip legacy PCI WDT devices */