Skip to contentSkip to conversation

WDRT based watchdog timer

PATCH In review Jordan Crouse 4 participants 11 messages Sep 26, 2008 – Jan 12, 2009

Subsystems (2)
ACPI Watchdog device drivers
lore
Share & tools
lore

Jordan Crouse

author
#0 lore raw

PATCH 1/1 WDRT based watchdog timer

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

Thanks,
Jordan

Signature · 3 lines
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

[PATCH]: Add a watchdog driver for the ACPI WDRT table

From: Jordan Crouse <[email protected]>

A watchdog timer driver for systems with that implement hardware timers described by the ACPI Watchdog Resource Table (WRDT).

Signed-off-by Jordan Crouse <[email protected]>

---

 MAINTAINERS                |    7 +
 drivers/watchdog/Kconfig   |   10 +
 drivers/watchdog/Makefile  |    1 
 drivers/watchdog/acpiwdt.c |  338 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 356 insertions(+), 0 deletions(-)
MAINTAINERS +7 -0
index 3596d17..8500456 100644
@@ -277,6 +277,13 @@ L:	[email protected]
 W:	http://www.lesswatts.org/projects/acpi/
 S:	Maintained
 
+ACPI WATCHDOG DRIVER
+P:     Jordan Crouse
+M:     [email protected]
+L:     [email protected]
+W:     http://www.lesswatts.org/projects/acpi/
+S:     Maintained
+
 AD1889 ALSA SOUND DRIVER
 P:	Kyle McMartin
 M:	[email protected]
drivers/watchdog/Kconfig +10 -0
index c510367..bf8ce27 100644
@@ -249,6 +249,16 @@ config BFIN_WDT
 
 # X86 (i386 + ia64 + x86_64) Architecture
 
+config ACPI_WDT
+	tristate "ACPI WDRT Watchdog Timer"
+	depends on X86 && ACPI
+	---help---
+	  This driver supports hardware watchdog timer implementations
+	  described by the Watchdog Resource Table (WDRT) ACPI table.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called acpiwdt.
+
 config ACQUIRE_WDT
 	tristate "Acquire SBC Watchdog Timer"
 	depends on X86
drivers/watchdog/Makefile +1 -0
index e0ef123..d03167c 100644
@@ -55,6 +55,7 @@ obj-$(CONFIG_BFIN_WDT) += bfin_wdt.o
 # H8300 Architecture
 
 # X86 (i386 + ia64 + x86_64) Architecture
+obj-$(CONFIG_ACPI_WDT) += acpiwdt.o
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
 obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
 obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
drivers/watchdog/acpiwdt.c new file +338 -0

338 lines not on this page · raw message

#1 lore raw
Quoted 4 lines · Jordan Crouse

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

Nice. I remember thinking about writing such a driver a long time ago.

Quoted 4 lines

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

acpi-watchdog ?

drivers/watchdog/acpiwdt.c line 52

Quoted 10 lines
drivers/watchdog/acpiwdt.c new file +10 -0
+struct wdrt_table {
+	struct acpi_table_header header;
+	struct acpi_generic_address ctrl_addr;
+	struct acpi_generic_address count_addr;
+	u16 pci_devid;
+	u16 pci_venid;
+	u8 pci_bus;
+	u8 pci_device;
+	u8 pci_function;
+	u8 pci_segment;

You don't seem to use the PCI device information currently, but they would be needed on IOMMU enabled systems to map the correct device, wouldn't they?

drivers/watchdog/acpiwdt.c line 97

Quoted 8 lines
drivers/watchdog/acpiwdt.c new file +8 -0
+static int acpiwdt_set_heartbeat(unsigned int interval)
+{
+	u32 val;
+
+	/* Make sure the interval is sane */
+
+	if (((interval * 1000) / wdt_units) > wdt_max_count)
+		return -EINVAL;

It's probably not a big issue in this context, but the check can overflow and miss wrong intervals.

Quoted 7 lines
drivers/watchdog/acpiwdt.c new file +7 -0
+
+	/* Enable the timer */
+
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
+
+	timeout = interval;

drivers/watchdog/acpiwdt.c line 114

Quoted 10 lines
drivers/watchdog/acpiwdt.c new file +10 -0
+
+	return 0;
+}
+
+static int acpiwdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
+		return -EBUSY;
+	if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
+		__module_get(THIS_MODULE);

This shouldn't be needed because the VFS does that. Also doing it in the module itself is always racy because the module can be unloaded before it increases its count. That is why the VFS does it.

I suppose you copied that from somehere, that somewhere is also wrong and should be ideally fixed too.

In general these watchdog drivers seem to have all a lot of copied code and at some point it would be nice to write a generic driver where the backend code is just plugged in.

-Andi

Signature · 1 line
[email protected]
author
#2 lore raw
Quoted 10 lines · Andi Kleen
Quoted 4 lines · Jordan Crouse

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

Nice. I remember thinking about writing such a driver a long time ago.

Quoted 6 lines
Quoted 4 lines

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

acpi-watchdog ?

Sure. The only thing I worry about with "acpi" in the name is that somebody think this might be a watchdog implemented by ACPI, which is a half truth. You also need the hardware interface to come along for the ride.

Quoted 6 lines
Quoted 6 lines
drivers/watchdog/acpiwdt.c new file +6 -0
+struct wdrt_table {
+	struct acpi_table_header header;
+	struct acpi_generic_address ctrl_addr;
+	struct acpi_generic_address count_addr;
+	u16 pci_devid;
+	u16 pci_venid;
Quoted 8 lines

drivers/watchdog/acpiwdt.c line 52

Quoted 4 lines
drivers/watchdog/acpiwdt.c new file +4 -0
+	u8 pci_bus;
+	u8 pci_device;
+	u8 pci_function;
+	u8 pci_segment;

You don't seem to use the PCI device information currently, but they would be needed on IOMMU enabled systems to map the correct device, wouldn't they?

I think so, but I have to admit a certain amount of ignorance about how the IOMMU operates. All the ACPI implementations I've seen that include the WDRT are not IOMMU aware (yet).

Quoted 11 lines

drivers/watchdog/acpiwdt.c line 97

Quoted 8 lines
drivers/watchdog/acpiwdt.c new file +8 -0
+static int acpiwdt_set_heartbeat(unsigned int interval)
+{
+	u32 val;
+
+	/* Make sure the interval is sane */
+
+	if (((interval * 1000) / wdt_units) > wdt_max_count)
+		return -EINVAL;

It's probably not a big issue in this context, but the check can overflow and miss wrong intervals.

Okay - I'll adjust it.

Quoted 22 lines
Quoted 7 lines
drivers/watchdog/acpiwdt.c new file +7 -0
+
+	/* Enable the timer */
+
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
+
+	timeout = interval;

drivers/watchdog/acpiwdt.c line 114

Quoted 10 lines
drivers/watchdog/acpiwdt.c new file +10 -0
+
+	return 0;
+}
+
+static int acpiwdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(WDT_FLAGS_OPEN, &wdt_flags))
+		return -EBUSY;
+	if (!test_and_clear_bit(WDT_FLAGS_ORPHAN, &wdt_flags))
+		__module_get(THIS_MODULE);

This shouldn't be needed because the VFS does that. Also doing it in the module itself is always racy because the module can be unloaded before it increases its count. That is why the VFS does it.

Quoted 2 lines

I suppose you copied that from somehere, that somewhere is also wrong and should be ideally fixed too.

Yep - I shared this from the the geodewdt driver, which got it from one of the other drivers in the directory.

Quoted 3 lines

In general these watchdog drivers seem to have all a lot of copied code and at some point it would be nice to write a generic driver where the backend code is just plugged in.

This is my second WDT driver in the last year, and I have to admit, that idea occurred to me as well.

Jordan

Signature · 3 lines
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.
reviewer
#3 lore raw
In reply to Jordan Crouse
Quoted 9 lines · Jordan Crouse

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

While this driver depends on ACPI, it actually isn't part of ACPI.

Indeed, here is 100% of the ACPI spec support for this watchdog timer:

"WDRT" Watchdog Resource Table Watchdog Timer Hardware Requirements for
Windows Server 2003
http://www.microsoft.com/whdc/system/CEC/
watchdog.mspx

so while the driver uses ACPI to enumerate the device, the device doesn't use any run-time ACPI support, and the Windows spec above actually defines the hardware.

Quoted 1 line

drivers/watchdog/acpiwdt.c

I'm not wild about having the string "acpi" be part of the driver name,
config option, variable names etc. Maybe the spec above which actually
defines the timer would suggest a better name; or maybe simply name it after its configuration table, wrdt.c?

The spec URL should be in the driver comments or commit log, BTW.

I don't know who maintains /drivers/watchdog/,
but Wim Van Sebroeck has scribbled there the most recently, and can probably weigh in on this.

I searched my logs for WRDT and found only one, and it does happen to be one with an Intel processor -- though an nvidia chipset:

http://bugzilla.kernel.org/show_bug.cgi?id=8950

so maybe Marc will be able to test the new driver?

thanks,
-Len

#4 lore raw

Hi Len,

Quoted 3 lines

I don't know who maintains /drivers/watchdog/,
but Wim Van Sebroeck has scribbled there the most recently, and can probably weigh in on this.

I do maintain it and still have to look at the driver in more detail. I'll come back on this.

Kind regards,
Wim.

author
#5 lore raw
In reply to Len Brown
Quoted 27 lines · Len Brown
Quoted 9 lines · Jordan Crouse

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

While this driver depends on ACPI, it actually isn't part of ACPI.

Indeed, here is 100% of the ACPI spec support for this watchdog timer:

"WDRT" Watchdog Resource Table Watchdog Timer Hardware Requirements for
Windows Server 2003
http://www.microsoft.com/whdc/system/CEC/
watchdog.mspx

so while the driver uses ACPI to enumerate the device, the device doesn't use any run-time ACPI support, and the Windows spec above actually defines the hardware.

Quoted 6 lines
Quoted 1 line

drivers/watchdog/acpiwdt.c

I'm not wild about having the string "acpi" be part of the driver name,
config option, variable names etc. Maybe the spec above which actually
defines the timer would suggest a better name; or maybe simply name it after its configuration table, wrdt.c?

yeah - the string 'acpi' concerned as well. I didn't want to really call it win2k3wdt.c, though. :).

Quoted 1 line

The spec URL should be in the driver comments or commit log, BTW.

Will do.

Quoted 3 lines

I don't know who maintains /drivers/watchdog/,
but Wim Van Sebroeck has scribbled there the most recently, and can probably weigh in on this.

Yeah - I forgot to CC him on my previous email, and I forwarded it to him separately.

Quoted 7 lines

I searched my logs for WRDT and found only one, and it does happen to be one with an Intel processor -- though an nvidia chipset:

http://bugzilla.kernel.org/show_bug.cgi?id=8950

so maybe Marc will be able to test the new driver?

Sure - once Wim sends his comments, I'll ping him. Thanks.

Jordan

Signature · 3 lines
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.
#6 lore raw

Hi Jordan,

Quoted 25 lines
Quoted 25 lines
Quoted 9 lines

Attached is a patch to support watchdog timers as described by the WDRT (Watch Dog Resource Table). This has been tested on the AMD SB600 chipset, but it should work for any chipset / BIOS that implements the WDRT table.

I'm not in love with the name - but I couldn't think of anything better, so I'm open to suggestions. I have a limited number of systems that implement this interface, so I would be very happy to hear from other users, especially ones not using AMD chipsets.

While this driver depends on ACPI, it actually isn't part of ACPI.

Indeed, here is 100% of the ACPI spec support for this watchdog timer:

"WDRT" Watchdog Resource Table Watchdog Timer Hardware Requirements for
Windows Server 2003
http://www.microsoft.com/whdc/system/CEC/
watchdog.mspx

so while the driver uses ACPI to enumerate the device, the device doesn't use any run-time ACPI support, and the Windows spec above actually defines the hardware.

Quoted 1 line

drivers/watchdog/acpiwdt.c

Quoted 7 lines
Quoted 4 lines

I'm not wild about having the string "acpi" be part of the driver name,
config option, variable names etc. Maybe the spec above which actually
defines the timer would suggest a better name; or maybe simply name it after its configuration table, wrdt.c?

yeah - the string 'acpi' concerned as well. I didn't want to really call it win2k3wdt.c, though. :).

wdrt.c would indeed be better then (I agree with Len's comment about ACPI). Can you change the code and the documentation (Kconfig) so that it is clear that this is about the Watchdog Resource Table Watchdog Timer and not about an ACPI WATCHDOG.

Furthermore:

drivers/watchdog/Kconfig config BFIN_WDT · line 254

Quoted 9 lines
drivers/watchdog/Kconfig +3 -0
@@ -249,6 +249,16 @@ config BFIN_WDT
 
 # X86 (i386 + ia64 + x86_64) Architecture
 
+config ACPI_WDT
+	tristate "ACPI WDRT Watchdog Timer"
+	depends on X86 && ACPI

The tristate description is what I like to see changed.

drivers/watchdog/Kconfig config BFIN_WDT · line 258

Quoted 4 lines
drivers/watchdog/Kconfig +4 -0
+	---help---
+	  This driver supports hardware watchdog timer implementations
+	  described by the Watchdog Resource Table (WDRT) ACPI table.
+

Ok for me.

drivers/watchdog/Kconfig config BFIN_WDT · line 260

Quoted 2 lines
drivers/watchdog/Kconfig +2 -0
+	  To compile this driver as a module, choose M here: the
+	  module will be called acpiwdt.

acpiwdt should be wdrt then.

...

drivers/watchdog/Kconfig config BFIN_WDT · line 261

Quoted 1 line
drivers/watchdog/Kconfig +1 -0
+obj-$(CONFIG_ACPI_WDT) += acpiwdt.o

...

same here.

drivers/watchdog/acpiwdt.c

Quoted 6 lines
drivers/watchdog/acpiwdt.c new file +0 -0
new file mode 100644
index 0000000..b6bcd52
@@ -0,0 +1,338 @@

same here.

drivers/watchdog/acpiwdt.c line 38

Quoted 1 line
drivers/watchdog/acpiwdt.c new file +1 -0
+static u32 wdt_flags;

Can you make this an unsigned long so that we don't get warnings.

drivers/watchdog/acpiwdt.c line 331

Quoted 7 lines
drivers/watchdog/acpiwdt.c new file +7 -0
+static void __exit acpiwdt_exit(void)
+{
+	iounmap(wdt_ctrl_reg);
+	iounmap(wdt_count_reg);
+
+	misc_deregister(&acpiwdt_miscdev);
+}

You should do the misc_deregister before the iounmap's (so first disable user_space access and then unmap the io-area).

I would also add spin_lock'ing for the ping and disable routines. This to avoid problems with forked children from the same program that opened /dev/watchdog.

For the rest it seems ok to me.

Kind regards,
Wim.

#7 lore raw

Hi Jordan,

Quoted 4 lines

wdrt.c would indeed be better then (I agree with Len's comment about ACPI). Can you change the code and the documentation (Kconfig) so that it is clear that this is about the Watchdog Resource Table Watchdog Timer and not about an ACPI WATCHDOG.

I applied my comments into the below patch.
If it's ok for you then we can continue on getting this in the mainline tree (after that Marc could test it).

Kind regards,
Wim.

commit 8277a9b616e90be295e50a93fa049454544f1a00

Author: Jordan Crouse <[email protected]>
Date:   Fri Sep 26 17:27:52 2008 -0600

[WATCHDOG] Add a watchdog driver for the WDRT ACPI table

A watchdog timer driver for systems with that implement hardware timers described by the ACPI Watchdog Resource Table (WDRT).

Signed-off-by Jordan Crouse <[email protected]>
Signed-off-by Wim Van Sebroeck <[email protected]>
MAINTAINERS +7 -0
index 5d0b8a2..f5d64f7 100644
@@ -4569,6 +4569,13 @@ M:	[email protected]
 T:	git kernel.org:/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog.git
 S:	Maintained
 
+WATCHDOG RESOURCE TABLE (WDRT) DRIVER
+P:	Jordan Crouse
+M:	[email protected]
+L:	[email protected]
+W:	http://www.lesswatts.org/projects/acpi/
+S:	Maintained
+
 WAVELAN NETWORK DRIVER & WIRELESS EXTENSIONS
 P:	Jean Tourrilhes
 M:	[email protected]
drivers/watchdog/Kconfig +10 -0
index 1a22fe7..132c884 100644
@@ -551,6 +551,16 @@ config CPU5_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called cpu5wdt.
 
+config WDRT_WDT
+	tristate "Watchdog Resource Table (WDRT) Watchdog Timer"
+	depends on X86 && ACPI
+	---help---
+	  This driver supports hardware watchdog timer implementations
+	  described by the Watchdog Resource Table (WDRT) ACPI table.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called wdrt.
+
 config SMSC37B787_WDT
 	tristate "Winbond SMsC37B787 Watchdog Timer"
 	depends on X86
drivers/watchdog/Makefile +1 -0
index e352bbb..2b8afe7 100644
@@ -83,6 +83,7 @@ obj-$(CONFIG_60XX_WDT) += sbc60xxwdt.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC7240_WDT) += sbc7240_wdt.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
+obj-$(CONFIG_WDRT_WDT) += wdrt.o
 obj-$(CONFIG_SMSC37B787_WDT) += smsc37b787_wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
 obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
drivers/watchdog/acpiwdt.c new file +345 -0

345 lines not on this page · raw message

author
#9 lore raw
Quoted 3 lines · Len Brown

Jordan,
just to clarify, Wim is in charge of the watchdog drivers, and so this one would go upstream via Wim.

Of course.

Quoted 8 lines

Re: ACPI-ness, I'm find with Wim's edited version, except for this remaining bit:

MAINTAINERS +2 -0
+L:     [email protected]
+W:     http://www.lesswatts.org/projects/acpi/

otherwise...

Acked-by Len Brown <[email protected]>

Yes, I'm happy with the final version. Thank you very much Wim and Len for your comments. Now, we just need to find some testers :)

Jordan

Signature · 3 lines
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.
#10 lore raw

Hi Jordan,

Quoted 9 lines
Quoted 3 lines

Jordan,
just to clarify, Wim is in charge of the watchdog drivers, and so this one would go upstream via Wim.

Of course.

Quoted 3 lines

Re: ACPI-ness, I'm find with Wim's edited version, except for this remaining bit:

Quoted 8 lines
Quoted 5 lines
MAINTAINERS +2 -0
+L:     [email protected]
+W:     http://www.lesswatts.org/projects/acpi/

otherwise...

Acked-by Len Brown <[email protected]>

Yes, I'm happy with the final version. Thank you very much Wim and Len for your comments. Now, we just need to find some testers :)

Any news about this?

Kind regards,
Wim.