Re: Missing watchdog after ACPI watchdog creation failure

[Takashi Iwai]

On Wed, 10 Jan 2018 16:43:23 +0100,
Takashi Iwai wrote:
> 
> On Wed, 10 Jan 2018 16:23:43 +0100,
> Mika Westerberg wrote:
> > 
> > On Wed, Jan 10, 2018 at 04:13:51PM +0100, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > on the recent kernels, i2c-i801 skips the creation of iTCO wdt when
> > > ACPI WDAT is present.  It's fine when ACPI really creates the watchdog
> > > device.  But, we've got a report showing that the watchdog is missing
> > > on some machines because ACPI failed to create, and yet i2c-i801 still
> > > skips because acpi_has_watchdog() returns true.
> > > 
> > > More specifically, the machine gets an error from acpi_watchdog.c
> > > like:
> > >   platform wdat_wdt: failed to claim resource 3: [io 0x040a-0x040c]
> > >   ACPI: watchdog: Device creation failed: -16
> > > 
> > > where the region was registered by pnp,
> > >   % cat /proc/ioports
> > >   ....
> > >   0400-047f : pnp 00:01
> > > 
> > > 
> > > One may say that BIOS sucks, but OTOH, the complete lack of watchdog
> > > thereafter can be seen as a regression, too.  It used to work on the
> > > older kernel as iTCO wdt was provided by i2c-i801.
> > 
> > Hmm, if the resource is already taken I wonder how iTCO can work? Are
> > you sure iTCO works on those systems?
> 
> Yes, that's the reason we got a bug report :)
> 4.4 kernel worked, and 4.12 (and later) don't.
> 
> On 4.4.x,
> % /proc/ioports
> 0000-0cf7 : PCI Bus 0000:00
> ...
>   0400-047f : pnp 00:01
>     0400-041f : iTCO_wdt
>       0400-041f : iTCO_wdt
>   0500-0503 : ACPI PM1a_EVT_BLK
> 
> On 4.12.x,
> % /proc/ioports
> 0000-0cf7 : PCI Bus 0000:00
> ...
>   0400-047f : pnp 00:01
>   0500-053f : pnp 00:01
> 
> > > Shouldn't acpi_has_watchdog() rather checks whether the watchdog
> > > device creation succeeded or not?
> > 
> > Yes, or rather we should first figure out what the actual problem is ;-)
> > 
> > Are you able to get acpidump from that system with full dmesg?
> 
> I'll ask the reporter.

Unfortunately we couldn't get approval yet, since it's a prototype
machine.

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.)


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@xxxxxxx>
Subject: [PATCH] ACPI / watchdog: Return probe fail state from acpi_has_watchdog()

The commit 058dfc767008 ("ACPI / watchdog: Add support for WDAT
hardware watchdog") introduced the WDAT watchdog support and prefers
it over iTCO if present.  However, this introduced a regression on a
machine with funky BIOS that doesn't set up WDAT resources properly.
Since acpi_has_watchdog() checks only the presence of WDAT entry and
it doesn't care whether the acpi watchdog device creation succeeded or
not, it ends up with no watchdog.

This patch fixes acpi_has_wathdog() to return false if the acpi
watchdog probe failed, so that iTCO watchdog can be created as a
fallback.

Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
 drivers/acpi/acpi_watchdog.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
index 11b113f8e367..b421081f0948 100644
--- a/drivers/acpi/acpi_watchdog.c
+++ b/drivers/acpi/acpi_watchdog.c
@@ -17,6 +17,8 @@
 
 #include "internal.h"
 
+static bool watchdog_failed;
+
 /**
  * Returns true if this system should prefer ACPI based watchdog instead of
  * the native one (which are typically the same hardware).
@@ -25,7 +27,7 @@ bool acpi_has_watchdog(void)
 {
 	struct acpi_table_header hdr;
 
-	if (acpi_disabled)
+	if (acpi_disabled || watchdog_failed)
 		return false;
 
 	return ACPI_SUCCESS(acpi_get_table_header(ACPI_SIG_WDAT, 0, &hdr));
@@ -44,6 +46,8 @@ void __init acpi_watchdog_init(void)
 	acpi_status status;
 	int i;
 
+	watchdog_failed = true;
+
 	status = acpi_get_table(ACPI_SIG_WDAT, 0,
 				(struct acpi_table_header **)&wdat);
 	if (ACPI_FAILURE(status)) {
@@ -120,6 +124,8 @@ void __init acpi_watchdog_init(void)
 					       resources, nresources);
 	if (IS_ERR(pdev))
 		pr_err("Device creation failed: %ld\n", PTR_ERR(pdev));
+	else
+		watchdog_failed = false; /* probe success */
 
 	kfree(resources);
 
-- 
2.15.1