[PATCH] WDRT based watchdog timer

[PATCH] WDRT based watchdog timer

[Jordan Crouse]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

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

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

[-- Attachment #2: acpi-watchdog.patch --]
[-- Type: text/x-diff, Size: 9760 bytes --]

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

From: Jordan Crouse <jordan.crouse@amd.com>

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

Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
---

 MAINTAINERS                |    7 +
 drivers/watchdog/Kconfig   |   10 +
 drivers/watchdog/Makefile  |    1 
 drivers/watchdog/acpiwdt.c |  338 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 356 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3596d17..8500456 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -277,6 +277,13 @@ L:	linux-acpi@vger.kernel.org
 W:	http://www.lesswatts.org/projects/acpi/
 S:	Maintained
 
+ACPI WATCHDOG DRIVER
+P:     Jordan Crouse
+M:     jordan.crouse@amd.com
+L:     linux-acpi@vger.kernel.org
+W:     http://www.lesswatts.org/projects/acpi/
+S:     Maintained
+
 AD1889 ALSA SOUND DRIVER
 P:	Kyle McMartin
 M:	kyle@mcmartin.ca
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c510367..bf8ce27 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index e0ef123..d03167c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
diff --git a/drivers/watchdog/acpiwdt.c b/drivers/watchdog/acpiwdt.c
new file mode 100644
index 0000000..b6bcd52
--- /dev/null
+++ b/drivers/watchdog/acpiwdt.c
@@ -0,0 +1,338 @@
+/* Driver for systems that implement hardware watchdog
+ * timers described by the ACPI Watchdog Resource Table (WDRT)
+ *
+ * Copyright (C) 2008, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version
+ * 2 of the License
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/acpi.h>
+#include <linux/uaccess.h>
+#include <linux/io.h>
+#include <acpi/actbl.h>
+
+#define PFX "acpiwdt: "
+
+#define WATCHDOG_TIMEOUT 60
+#define WDT_FLAGS_OPEN 1
+#define WDT_FLAGS_ORPHAN 2
+
+static int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds, default="
+		 __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static u32 wdt_flags;
+static int expect_close;
+
+/* ACPI WDRT table */
+
+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;
+	u16 max_count;
+	u8 units;
+} __attribute__ ((packed));
+
+/* Bit definitions for the control register */
+
+#define WDT_CTRL_TRIGGER (1 << 7)
+#define WDT_CTRL_DISABLE (1 << 3)
+#define WDT_CTRL_ACTION  (1 << 2)
+#define WDT_CTRL_FIRED   (1 << 1)
+#define WDT_CTRL_RUN     (1 << 0)
+
+static u64 *wdt_ctrl_reg;	/* Address of the control register */
+static u64 *wdt_count_reg;	/* Address of the count register */
+
+static u32 wdt_units;		/* Number of milliseconds per tick */
+static u32 wdt_max_count;	/* Maximum number of ticks */
+
+static void acpiwdt_ping(void)
+{
+	u32 count = (timeout * 1000) / wdt_units;
+	u32 val;
+
+	/* Write the timeout to the counter */
+	writel(count, wdt_count_reg);
+
+	/* Hit the trigger bit */
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_TRIGGER, wdt_ctrl_reg);
+}
+
+static void acpiwdt_disable(void)
+{
+	u32 val = readl(wdt_ctrl_reg);
+	writel(val & ~WDT_CTRL_RUN, wdt_ctrl_reg);
+}
+
+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;
+
+	/* Enable the timer */
+
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
+
+	timeout = interval;
+
+	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);
+
+	acpiwdt_ping();
+	return nonseekable_open(inode, file);
+}
+
+static int acpiwdt_release(struct inode *inode, struct file *file)
+{
+	if (expect_close) {
+		acpiwdt_disable();
+		module_put(THIS_MODULE);
+	} else {
+		printk(KERN_CRIT PFX
+			"Unexpected close - watchdog is not stopping\n");
+		acpiwdt_ping();
+		set_bit(WDT_FLAGS_ORPHAN, &wdt_flags);
+	}
+
+	clear_bit(WDT_FLAGS_OPEN, &wdt_flags);
+	expect_close = 0;
+	return 0;
+}
+
+static ssize_t acpiwdt_write(struct file *file, const char __user *buf,
+			size_t len, loff_t *ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+			expect_close = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, buf + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+
+		acpiwdt_ping();
+	}
+
+	return len;
+}
+
+static struct watchdog_info ident = {
+	.options = WDIOF_SETTIMEOUT |
+		   WDIOF_KEEPALIVEPING |
+		   WDIOF_MAGICCLOSE,
+	.identity = "ACPI Watchdog Timer",
+};
+
+static long acpiwdt_ioctl(struct file *file,
+			unsigned cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+	int __user *p = argp;
+	int new_timeout;
+	int options;
+	int ret = 0;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			ret = -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		ret = put_user(0, p);
+		break;
+
+	case WDIOC_SETOPTIONS:
+		ret = -EINVAL;
+
+		if (get_user(options, p)) {
+			ret = -EFAULT;
+			break;
+		}
+		if (options & WDIOS_DISABLECARD) {
+			acpiwdt_disable();
+			ret = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			acpiwdt_ping();
+			ret = 0;
+		}
+		break;
+
+	case WDIOC_KEEPALIVE:
+		acpiwdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		ret = get_user(new_timeout, p);
+		if (ret)
+			break;
+		ret = acpiwdt_set_heartbeat(new_timeout);
+		if (ret)
+			break;
+
+		acpiwdt_ping();
+		/* fall through */
+	case WDIOC_GETTIMEOUT:
+		ret = put_user(timeout, p);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct file_operations acpiwdt_fops = {
+	.owner          = THIS_MODULE,
+	.llseek         = no_llseek,
+	.write          = acpiwdt_write,
+	.unlocked_ioctl = acpiwdt_ioctl,
+	.open           = acpiwdt_open,
+	.release        = acpiwdt_release,
+};
+
+static struct miscdevice acpiwdt_miscdev = {
+	.minor          = WATCHDOG_MINOR,
+	.name           = "watchdog",
+	.fops           = &acpiwdt_fops,
+};
+
+static int __init acpiwdt_init(void)
+{
+	struct wdrt_table *wdrt;
+	acpi_status status;
+	u32 val;
+	int ret = -ENODEV;
+
+	status = acpi_get_table(ACPI_SIG_WDRT, 0,
+				(struct acpi_table_header **) &wdrt);
+
+	/* Silently return if there is no WDRT table */
+
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	wdt_ctrl_reg = ioremap(wdrt->ctrl_addr.address, sizeof(u64));
+	wdt_count_reg = ioremap(wdrt->count_addr.address, sizeof(u64));
+
+	if (wdt_ctrl_reg == NULL || wdt_count_reg == NULL)
+		goto err;
+
+	val = readl(wdt_ctrl_reg);
+
+	if (val & WDT_CTRL_DISABLE) {
+		/* Try to enable the watchdog timer if we can - some
+		 * implementations may not allow this
+		 */
+
+		writel(val & ~WDT_CTRL_DISABLE, wdt_ctrl_reg);
+
+		val = readl(wdt_ctrl_reg);
+
+		if (val & WDT_CTRL_DISABLE) {
+			printk(KERN_ERR PFX
+				"Timer is disabled by the firmware\n");
+			goto err;
+		}
+	}
+
+	wdt_max_count = wdrt->max_count;
+
+	switch (wdrt->units) {
+	case 0:  /* 1 sec/count */
+		wdt_units = 1000;
+		break;
+	case 1: /* 100 ms/count */
+		wdt_units = 100;
+		break;
+	case 2: /* 10 ms/count */
+		wdt_units = 10;
+		break;
+	}
+
+	if (acpiwdt_set_heartbeat(timeout)) {
+		acpiwdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX
+			"The timeout must be > 0 and < %d seconds. Using %d\n",
+			 (wdt_units * 0xFFFF) / 1000, WATCHDOG_TIMEOUT);
+	}
+
+	ret = misc_register(&acpiwdt_miscdev);
+
+	if (ret) {
+		printk(KERN_ERR PFX "Could not register misc device\n");
+		goto err;
+	}
+
+	printk(KERN_INFO PFX "ACPI WRDT Watchdog Timer initialized\n");
+	return 0;
+
+err:
+	if (wdt_ctrl_reg != NULL)
+		iounmap(wdt_ctrl_reg);
+
+	if (wdt_count_reg != NULL)
+		iounmap(wdt_count_reg);
+
+	return ret;
+}
+
+static void __exit acpiwdt_exit(void)
+{
+	iounmap(wdt_ctrl_reg);
+	iounmap(wdt_count_reg);
+
+	misc_deregister(&acpiwdt_miscdev);
+}
+
+module_init(acpiwdt_init);
+module_exit(acpiwdt_exit);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Driver for watchdog timers described by the ACPI WDRT");
+MODULE_LICENSE("GPL");

Re: [PATCH] WDRT based watchdog timer

[Andi Kleen]

Jordan Crouse <jordan.crouse@amd.com> writes:

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

> 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 ? 

> +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?

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

> +
> +	/* Enable the timer */
> +
> +	val = readl(wdt_ctrl_reg);
> +	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
> +
> +	timeout = interval;
> +
> +	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
-- 
ak@linux.intel.com

Re: WDRT based watchdog timer

[Jordan Crouse]

On 07/10/08 09:44 +0200, Andi Kleen wrote:
> Jordan Crouse <jordan.crouse@amd.com> writes:
> 
> > 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.
> 
> > 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.

> > +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?

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

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

> > +
> > +	/* Enable the timer */
> > +
> > +	val = readl(wdt_ctrl_reg);
> > +	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
> > +
> > +	timeout = interval;
> > +
> > +	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.

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

> 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

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Re: [PATCH] WDRT based watchdog timer

[Len Brown]

On Fri, 26 Sep 2008, Jordan Crouse wrote:

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

> 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

Re: [PATCH] WDRT based watchdog timer

[Wim Van Sebroeck]

Hi Len,

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

Re: WDRT based watchdog timer

[Jordan Crouse]

On 08/10/08 16:25 -0400, Len Brown wrote:
> 
> 
> On Fri, 26 Sep 2008, Jordan Crouse wrote:
> 
> > 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.
> 
> > 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. :). 

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

Will do.

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

> 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

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Re: WDRT based watchdog timer

[Wim Van Sebroeck]

Hi Jordan,

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

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:

> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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.

> +	---help---
> +	  This driver supports hardware watchdog timer implementations
> +	  described by the Watchdog Resource Table (WDRT) ACPI table.
> +

Ok for me.

> +	  To compile this driver as a module, choose M here: the
> +	  module will be called acpiwdt.

acpiwdt should be wdrt then.

...
> +obj-$(CONFIG_ACPI_WDT) += acpiwdt.o
...

same here.

> diff --git a/drivers/watchdog/acpiwdt.c b/drivers/watchdog/acpiwdt.c
> new file mode 100644
> index 0000000..b6bcd52
> --- /dev/null
> +++ b/drivers/watchdog/acpiwdt.c
> @@ -0,0 +1,338 @@

same here.

> +static u32 wdt_flags;

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

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

Re: WDRT based watchdog timer

[Wim Van Sebroeck]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Hi Jordan,

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


[-- Attachment #2: wdrt.patch --]
[-- Type: text/plain, Size: 10020 bytes --]

commit 8277a9b616e90be295e50a93fa049454544f1a00
Author: Jordan Crouse <jordan.crouse@amd.com>
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 <jordan.crouse@amd.com>
    Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d0b8a2..f5d64f7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4569,6 +4569,13 @@ M:	wim@iguana.be
 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:	jordan.crouse@amd.com
+L:	linux-acpi@vger.kernel.org
+W:	http://www.lesswatts.org/projects/acpi/
+S:	Maintained
+
 WAVELAN NETWORK DRIVER & WIRELESS EXTENSIONS
 P:	Jean Tourrilhes
 M:	jt@hpl.hp.com
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1a22fe7..132c884 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index e352bbb..2b8afe7 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
diff --git a/drivers/watchdog/acpiwdt.c b/drivers/watchdog/acpiwdt.c
new file mode 100644
index 0000000..9b082b4
--- /dev/null
+++ b/drivers/watchdog/acpiwdt.c
@@ -0,0 +1,345 @@
+/* Driver for systems that implement hardware watchdog
+ * timers described by the ACPI Watchdog Resource Table (WDRT)
+ *
+ * Copyright (C) 2008, Advanced Micro Devices, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version
+ * 2 of the License
+ */
+
+/*
+ * For more information:
+ * "WDRT" Watchdog Resource Table Watchdog Timer Hardware Requirements for
+ * Windows Server 2003 - http://www.microsoft.com/whdc/system/CEC/watchdog.mspx
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/acpi.h>
+#include <linux/uaccess.h>
+#include <linux/io.h>
+#include <acpi/actbl.h>
+
+#define PFX "wdrt: "
+
+#define WATCHDOG_TIMEOUT 60
+#define WDT_FLAGS_OPEN 1
+#define WDT_FLAGS_ORPHAN 2
+
+static int timeout = WATCHDOG_TIMEOUT;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds, default="
+			__MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static unsigned long wdt_flags;
+static int expect_close;
+
+/* WDRT ACPI table */
+
+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;
+	u16 max_count;
+	u8 units;
+} __attribute__ ((packed));
+
+/* Bit definitions for the control register */
+
+#define WDT_CTRL_TRIGGER (1 << 7)
+#define WDT_CTRL_DISABLE (1 << 3)
+#define WDT_CTRL_ACTION  (1 << 2)
+#define WDT_CTRL_FIRED   (1 << 1)
+#define WDT_CTRL_RUN     (1 << 0)
+
+static u64 *wdt_ctrl_reg;	/* Address of the control register */
+static u64 *wdt_count_reg;	/* Address of the count register */
+
+static u32 wdt_units;		/* Number of milliseconds per tick */
+static u32 wdt_max_count;	/* Maximum number of ticks */
+
+static void wdrt_ping(void)
+{
+	u32 count = (timeout * 1000) / wdt_units;
+	u32 val;
+
+	/* Write the timeout to the counter */
+	writel(count, wdt_count_reg);
+
+	/* Hit the trigger bit */
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_TRIGGER, wdt_ctrl_reg);
+}
+
+static void wdrt_disable(void)
+{
+	u32 val = readl(wdt_ctrl_reg);
+	writel(val & ~WDT_CTRL_RUN, wdt_ctrl_reg);
+}
+
+static int wdrt_set_heartbeat(unsigned int interval)
+{
+	u32 val;
+
+	/* Make sure the interval is sane */
+
+	if (((interval * 1000) / wdt_units) > wdt_max_count)
+		return -EINVAL;
+
+	/* Enable the timer */
+
+	val = readl(wdt_ctrl_reg);
+	writel(val | WDT_CTRL_RUN, wdt_ctrl_reg);
+
+	timeout = interval;
+
+	return 0;
+}
+
+static int wdrt_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);
+
+	wdrt_ping();
+	return nonseekable_open(inode, file);
+}
+
+static int wdrt_release(struct inode *inode, struct file *file)
+{
+	if (expect_close) {
+		wdrt_disable();
+		module_put(THIS_MODULE);
+	} else {
+		printk(KERN_CRIT PFX
+			"Unexpected close - watchdog is not stopping\n");
+		wdrt_ping();
+		set_bit(WDT_FLAGS_ORPHAN, &wdt_flags);
+	}
+
+	clear_bit(WDT_FLAGS_OPEN, &wdt_flags);
+	expect_close = 0;
+	return 0;
+}
+
+static ssize_t wdrt_write(struct file *file, const char __user *buf,
+			size_t len, loff_t *ppos)
+{
+	if (len) {
+		if (!nowayout) {
+			size_t i;
+			expect_close = 0;
+
+			for (i = 0; i != len; i++) {
+				char c;
+				if (get_user(c, buf + i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+
+		wdrt_ping();
+	}
+
+	return len;
+}
+
+static struct watchdog_info ident = {
+	.options = WDIOF_SETTIMEOUT |
+		   WDIOF_KEEPALIVEPING |
+		   WDIOF_MAGICCLOSE,
+	.identity = "WDRT Watchdog Timer",
+};
+
+static long wdrt_ioctl(struct file *file, unsigned cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+	int __user *p = argp;
+	int new_timeout;
+	int options;
+	int ret = 0;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			ret = -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		ret = put_user(0, p);
+		break;
+
+	case WDIOC_SETOPTIONS:
+		ret = -EINVAL;
+
+		if (get_user(options, p)) {
+			ret = -EFAULT;
+			break;
+		}
+		if (options & WDIOS_DISABLECARD) {
+			wdrt_disable();
+			ret = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdrt_ping();
+			ret = 0;
+		}
+		break;
+
+	case WDIOC_KEEPALIVE:
+		wdrt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		ret = get_user(new_timeout, p);
+		if (ret)
+			break;
+		ret = wdrt_set_heartbeat(new_timeout);
+		if (ret)
+			break;
+
+		wdrt_ping();
+		/* fall through */
+	case WDIOC_GETTIMEOUT:
+		ret = put_user(timeout, p);
+		break;
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct file_operations wdrt_fops = {
+	.owner          = THIS_MODULE,
+	.llseek         = no_llseek,
+	.write          = wdrt_write,
+	.unlocked_ioctl = wdrt_ioctl,
+	.open           = wdrt_open,
+	.release        = wdrt_release,
+};
+
+static struct miscdevice wdrt_miscdev = {
+	.minor          = WATCHDOG_MINOR,
+	.name           = "watchdog",
+	.fops           = &wdrt_fops,
+};
+
+static int __init wdrt_init(void)
+{
+	struct wdrt_table *wdrt;
+	acpi_status status;
+	u32 val;
+	int ret = -ENODEV;
+
+	status = acpi_get_table(ACPI_SIG_WDRT, 0,
+				(struct acpi_table_header **) &wdrt);
+
+	/* Silently return if there is no WDRT table */
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	wdt_ctrl_reg = ioremap(wdrt->ctrl_addr.address, sizeof(u64));
+	if (wdt_ctrl_reg == NULL)
+		return -EBUSY;
+
+	wdt_count_reg = ioremap(wdrt->count_addr.address, sizeof(u64));
+	if (wdt_count_reg == NULL) {
+		ret = -EBUSY;
+		goto err_ioremap;
+	}
+
+	val = readl(wdt_ctrl_reg);
+
+	if (val & WDT_CTRL_DISABLE) {
+		/* Try to enable the watchdog timer if we can - some
+		 * implementations may not allow this
+		 */
+
+		writel(val & ~WDT_CTRL_DISABLE, wdt_ctrl_reg);
+
+		val = readl(wdt_ctrl_reg);
+
+		if (val & WDT_CTRL_DISABLE) {
+			printk(KERN_ERR PFX
+				"Timer is disabled by the firmware\n");
+			goto err;
+		}
+	}
+
+	wdt_max_count = wdrt->max_count;
+
+	switch (wdrt->units) {
+	case 0:  /* 1 sec/count */
+		wdt_units = 1000;
+		break;
+	case 1: /* 100 ms/count */
+		wdt_units = 100;
+		break;
+	case 2: /* 10 ms/count */
+		wdt_units = 10;
+		break;
+	}
+
+	if (wdrt_set_heartbeat(timeout)) {
+		wdrt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX
+			"The timeout must be > 0 and < %d seconds. Using %d\n",
+			 (wdt_units * 0xFFFF) / 1000, WATCHDOG_TIMEOUT);
+	}
+
+	ret = misc_register(&wdrt_miscdev);
+	if (ret) {
+		printk(KERN_ERR PFX "Could not register misc device\n");
+		goto err;
+	}
+
+	printk(KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+	return 0;
+
+err:
+	iounmap(wdt_count_reg);
+err_ioremap:
+	iounmap(wdt_ctrl_reg);
+	return ret;
+}
+
+static void __exit wdrt_exit(void)
+{
+	misc_deregister(&wdrt_miscdev);
+
+	iounmap(wdt_count_reg);
+	iounmap(wdt_ctrl_reg);
+}
+
+module_init(wdrt_init);
+module_exit(wdrt_exit);
+
+MODULE_AUTHOR("Advanced Micro Devices, Inc");
+MODULE_DESCRIPTION("Driver for watchdog timers described by the ACPI WDRT");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
+

Re: WDRT based watchdog timer

[Len Brown]

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

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

+L:     linux-acpi@vger.kernel.org
+W:     http://www.lesswatts.org/projects/acpi/

otherwise...
Acked-by: Len Brown <len.brown@intel.com>

thanks,
-Len

Re: WDRT based watchdog timer

[Jordan Crouse]

On 16/10/08 18:23 -0400, Len Brown wrote:
> Jordan,
> just to clarify, Wim is in charge of the watchdog drivers,
> and so this one would go upstream via Wim.

Of course. 

> Re: ACPI-ness, I'm find with Wim's edited version,
> except for this remaining bit:
> 
> +L:     linux-acpi@vger.kernel.org
> +W:     http://www.lesswatts.org/projects/acpi/
> 
> otherwise...
> Acked-by: Len Brown <len.brown@intel.com>

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

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.

Re: WDRT based watchdog timer

[Wim Van Sebroeck]

Hi Jordan,

> > Jordan,
> > just to clarify, Wim is in charge of the watchdog drivers,
> > and so this one would go upstream via Wim.
> 
> Of course. 
> 
> > Re: ACPI-ness, I'm find with Wim's edited version,
> > except for this remaining bit:
> > 
> > +L:     linux-acpi@vger.kernel.org
> > +W:     http://www.lesswatts.org/projects/acpi/
> > 
> > otherwise...
> > Acked-by: Len Brown <len.brown@intel.com>
> 
> 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.