[RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Kyle Roeschley]

Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
time controllers.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 drivers/watchdog/Kconfig      |  11 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/ni9x3x_wdt.c | 264 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+)
 create mode 100644 drivers/watchdog/ni9x3x_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0f6d851..18bd13a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called sbc_epx_c3.
 
+config NI9X3X_WDT
+	tristate "NI 903x/913x Watchdog"
+	depends on X86 && ACPI
+	select WATCHDOG_CORE
+	---help---
+	  This is the driver for the watchdog timer on the National Instruments
+	  903x/913x real-time controllers.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ni9x3x_wdt.
+
 # M32R Architecture
 
 # M68K Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f566753..527978b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
 obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
 obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
 obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
+obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
 
 # M32R Architecture
 
diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
new file mode 100644
index 0000000..2cb5627
--- /dev/null
+++ b/drivers/watchdog/ni9x3x_wdt.c
@@ -0,0 +1,264 @@
+/*
+ * Copyright (C) 2013 National Instruments Corp.
+ *
+ * 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; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#define NIWD_CONTROL	0x01
+#define NIWD_COUNTER2	0x02
+#define NIWD_COUNTER1	0x03
+#define NIWD_COUNTER0	0x04
+#define NIWD_SEED2	0x05
+#define NIWD_SEED1	0x06
+#define NIWD_SEED0	0x07
+
+#define NIWD_IO_SIZE	0x08
+
+#define NIWD_CONTROL_MODE		0x80
+#define NIWD_CONTROL_PROC_RESET		0x20
+#define NIWD_CONTROL_PET		0x10
+#define NIWD_CONTROL_RUNNING		0x08
+#define NIWD_CONTROL_CAPTURECOUNTER	0x04
+#define NIWD_CONTROL_RESET		0x02
+#define NIWD_CONTROL_ALARM		0x01
+
+#define NIWD_PERIOD_NS		30720
+#define NIWD_MIN_TIMEOUT	1
+#define NIWD_MAX_TIMEOUT	515
+#define NIWD_DEFAULT_TIMEOUT	60
+
+#define NI9X3X_WDT_NAME		"ni9x3x_wdt"
+
+#define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
+
+struct ni9x3x_wdt {
+	struct acpi_device *acpi_device;
+	u16 io_base;
+	u16 io_size;
+	spinlock_t lock;
+	struct watchdog_device wdog;
+};
+
+static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
+{
+	u8 control = inb(wdt->io_base + NIWD_CONTROL);
+
+	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
+}
+
+static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
+				      unsigned int timeout)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
+
+	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
+	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
+	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
+
+	return 0;
+}
+
+static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+	u8 control, counter0, counter1, counter2;
+	u32 counter;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+	control |= NIWD_CONTROL_CAPTURECOUNTER;
+	outb(control, wdt->io_base + NIWD_CONTROL);
+
+	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
+	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
+	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
+
+	counter = (counter2 << 16) | (counter1 << 8) | counter0;
+	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
+
+	return (unsigned int)counter;
+}
+
+static int ni9x3x_wdt_wdd_start(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+
+	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_PROC_RESET,
+	     wdt->io_base + NIWD_CONTROL);
+
+	ni9x3x_wdt_wdd_set_timeout(wdd, wdd->timeout);
+	ni9x3x_wdt_start(wdt);
+
+	return 0;
+}
+
+static int ni9x3x_wdt_wdd_stop(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+
+	outb(NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
+
+	return 0;
+}
+
+static int ni9x3x_wdt_wdd_ping(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+	u8 control;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
+
+	return 0;
+}
+
+static acpi_status ni9x3x_wdt_resources(struct acpi_resource *res, void *data)
+{
+	struct ni9x3x_wdt *wdt = data;
+
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_IO:
+		if (wdt->io_base != 0 || wdt->io_size != 0) {
+			dev_err(&wdt->acpi_device->dev,
+				 "too many IO resources\n");
+			return AE_ERROR;
+		}
+
+		wdt->io_base = res->data.io.minimum;
+		wdt->io_size = res->data.io.address_length;
+
+		return AE_OK;
+
+	case ACPI_RESOURCE_TYPE_END_TAG:
+		return AE_OK;
+
+	default:
+		dev_err(&wdt->acpi_device->dev,
+			"unsupported resource type %d\n", res->type);
+		return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+static int ni9x3x_wdt_acpi_remove(struct acpi_device *device)
+{
+	struct ni9x3x_wdt *wdt = acpi_driver_data(device);
+
+	ni9x3x_wdt_wdd_stop(&wdt->wdog);
+	watchdog_unregister_device(&wdt->wdog);
+
+	return 0;
+}
+
+static const struct watchdog_info ni9x3x_wdt_wdd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "NI Watchdog",
+};
+
+static const struct watchdog_ops ni9x3x_wdt_wdd_ops = {
+	.owner		= THIS_MODULE,
+	.start		= ni9x3x_wdt_wdd_start,
+	.stop		= ni9x3x_wdt_wdd_stop,
+	.ping		= ni9x3x_wdt_wdd_ping,
+	.set_timeout	= ni9x3x_wdt_wdd_set_timeout,
+	.get_timeleft	= ni9x3x_wdt_wdd_get_timeleft,
+};
+
+static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
+{
+	struct ni9x3x_wdt *wdt;
+	acpi_status status;
+	int ret;
+
+	wdt = devm_kzalloc(&device->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	device->driver_data = wdt;
+	wdt->acpi_device = device;
+
+	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+				     ni9x3x_wdt_resources, wdt);
+	if (ACPI_FAILURE(status) || wdt->io_base == 0 ||
+	    wdt->io_size != NIWD_IO_SIZE) {
+		dev_err(&device->dev, "failed to get resources\n");
+		return -ENODEV;
+	}
+
+	if (!devm_request_region(&device->dev, wdt->io_base, wdt->io_size,
+				 NI9X3X_WDT_NAME)) {
+		dev_err(&device->dev, "failed to get memory region\n");
+		return -EBUSY;
+	}
+
+	spin_lock_init(&wdt->lock);
+
+	wdt->wdog.info = &ni9x3x_wdt_wdd_info;
+	wdt->wdog.ops = &ni9x3x_wdt_wdd_ops;
+	wdt->wdog.min_timeout = NIWD_MIN_TIMEOUT;
+	wdt->wdog.max_timeout = NIWD_MAX_TIMEOUT;
+	wdt->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
+	wdt->wdog.parent = &device->dev;
+
+	ret = watchdog_register_device(&wdt->wdog);
+	if (ret) {
+		dev_err(&device->dev, "failed to register watchdog\n");
+		return ret;
+	}
+
+	/* Switch from boot mode to user mode */
+	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
+	     wdt->io_base + NIWD_CONTROL);
+
+	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n",
+		 wdt->io_base, wdt->io_base + wdt->io_size - 1);
+	return 0;
+}
+
+static const struct acpi_device_id ni9x3x_wdt_device_ids[] = {
+	{"NIC775C", 0},
+	{"", 0},
+};
+
+static struct acpi_driver ni9x3x_wdt_acpi_driver = {
+	.name = NI9X3X_WDT_NAME,
+	.ids = ni9x3x_wdt_device_ids,
+	.ops = {
+		.add = ni9x3x_wdt_acpi_add,
+		.remove = ni9x3x_wdt_acpi_remove,
+		},
+};
+
+static int __init ni9x3x_wdt_init(void)
+{
+	return acpi_bus_register_driver(&ni9x3x_wdt_acpi_driver);
+}
+
+static void __exit ni9x3x_wdt_exit(void)
+{
+	acpi_bus_unregister_driver(&ni9x3x_wdt_acpi_driver);
+}
+
+module_init(ni9x3x_wdt_init);
+module_exit(ni9x3x_wdt_exit);
+
+MODULE_DEVICE_TABLE(acpi, ni9x3x_wdt_device_ids);
+MODULE_DESCRIPTION("NI Watchdog");
+MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
+MODULE_AUTHOR("Kyle Roeschley <kyle.roeschley@ni.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.0

[RFC 2/4] watchdog: ni9x3x_wdt: Add counter sysfs attribute

[Kyle Roeschley]

The NI 9x3x watchdog timer has 30.72 µs resolution, so expose this
by allowing the user to set the timeout counter directly via a sysfs
attribute.

RFC because we can't use the watchdog core lock inside the _show and
_store functions, so we're left without any protection on setting or
reading the control register. The options right now are either add our
own lock and double lock everything or force access to the watchdog core
lock. We don't want to do either, so for now we do nothing.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 drivers/watchdog/ni9x3x_wdt.c | 73 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
index 2cb5627..13f5c10 100644
--- a/drivers/watchdog/ni9x3x_wdt.c
+++ b/drivers/watchdog/ni9x3x_wdt.c
@@ -60,37 +60,45 @@ static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
 	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
 }
 
-static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
-				      unsigned int timeout)
+static void ni9x3x_wdt_counter_set(struct ni9x3x_wdt *wdt, u32 counter)
 {
-	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
-	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
-
 	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
 	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
 	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
-
-	return 0;
 }
 
-static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
+static unsigned int ni9x3x_wdt_counter_get(struct ni9x3x_wdt *wdt)
 {
-	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
 	u8 control, counter0, counter1, counter2;
-	u32 counter;
 
 	control = inb(wdt->io_base + NIWD_CONTROL);
-	control |= NIWD_CONTROL_CAPTURECOUNTER;
-	outb(control, wdt->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_CAPTURECOUNTER,
+	     wdt->io_base + NIWD_CONTROL);
 
 	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
 	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
 	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
 
-	counter = (counter2 << 16) | (counter1 << 8) | counter0;
-	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
+	return (counter2 << 16) | (counter1 << 8) | counter0;
+}
+
+static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
+				      unsigned int timeout)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
+
+	ni9x3x_wdt_counter_set(wdt, counter);
+
+	return 0;
+}
+
+static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
+{
+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
+	u32 counter = ni9x3x_wdt_counter_get(wdt);
 
-	return (unsigned int)counter;
+	return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000);
 }
 
 static int ni9x3x_wdt_wdd_start(struct watchdog_device *wdd)
@@ -126,6 +134,40 @@ static int ni9x3x_wdt_wdd_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
+static ssize_t counter_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	u32 counter;
+
+	counter = ni9x3x_wdt_counter_get(wdt);
+
+	return sprintf(buf, "%u\n", counter);
+}
+
+static ssize_t counter_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	u32 val;
+
+	if (kstrtouint(buf, 10, &val) || val > 0xFFFFFF)
+		return -EINVAL;
+
+	ni9x3x_wdt_counter_set(wdt, val);
+
+	return count;
+}
+static DEVICE_ATTR_RW(counter);
+
+static struct attribute *ni9x3x_wdt_attrs[] = {
+	&dev_attr_counter.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ni9x3x_wdt);
+
 static acpi_status ni9x3x_wdt_resources(struct acpi_resource *res, void *data)
 {
 	struct ni9x3x_wdt *wdt = data;
@@ -214,6 +256,7 @@ static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
 	wdt->wdog.max_timeout = NIWD_MAX_TIMEOUT;
 	wdt->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
 	wdt->wdog.parent = &device->dev;
+	wdt->wdog.groups = ni9x3x_wdt_groups;
 
 	ret = watchdog_register_device(&wdt->wdog);
 	if (ret) {
-- 
2.7.0

[RFC 3/4] watchdog: ni9x3x_wdt: Add timeout_action sysfs attribute

[Kyle Roeschley]

The NI 903x/913x watchdog has the ability to either assert an interrupt
or reset the system on timeout, so expose this option via s sysfs
attribute.

RFC because we can't use the watchdog core lock inside the _show and
_store functions, so we're left without any protection on setting or
reading the control register. The options right now are either add our
own lock and double lock everything or force access to the watchdog core
lock. We don't want to do either, so for now we do nothing.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 drivers/watchdog/ni9x3x_wdt.c | 106 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
index 13f5c10..1fce4d0 100644
--- a/drivers/watchdog/ni9x3x_wdt.c
+++ b/drivers/watchdog/ni9x3x_wdt.c
@@ -15,6 +15,7 @@
 #include <linux/acpi.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/sysfs.h>
 #include <linux/watchdog.h>
 
 #define NIWD_CONTROL	0x01
@@ -28,6 +29,7 @@
 #define NIWD_IO_SIZE	0x08
 
 #define NIWD_CONTROL_MODE		0x80
+#define NIWD_CONTROL_PROC_INTERRUPT	0x40
 #define NIWD_CONTROL_PROC_RESET		0x20
 #define NIWD_CONTROL_PET		0x10
 #define NIWD_CONTROL_RUNNING		0x08
@@ -48,6 +50,7 @@ struct ni9x3x_wdt {
 	struct acpi_device *acpi_device;
 	u16 io_base;
 	u16 io_size;
+	u32 irq;
 	spinlock_t lock;
 	struct watchdog_device wdog;
 };
@@ -101,6 +104,34 @@ static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
 	return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000);
 }
 
+static irqreturn_t ni9x3x_wdt_irq_handler(int irq, void *data)
+{
+	struct ni9x3x_wdt *wdt = data;
+	irqreturn_t ret = IRQ_NONE;
+	u8 control;
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdt->lock, flags);
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+
+	if (!(NIWD_CONTROL_ALARM & control)) {
+		dev_err(&wdt->acpi_device->dev,
+			"Spurious watchdog interrupt, 0x%02X\n", control);
+		goto out_unlock;
+	}
+
+	/* Acknowledge the interrupt. */
+	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
+
+	ret = IRQ_HANDLED;
+
+out_unlock:
+	spin_unlock_irqrestore(&wdt->lock, flags);
+
+	return ret;
+}
+
 static int ni9x3x_wdt_wdd_start(struct watchdog_device *wdd)
 {
 	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
@@ -134,6 +165,55 @@ static int ni9x3x_wdt_wdd_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
+static ssize_t timeout_action_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	u8 control;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+
+	if (control & NIWD_CONTROL_PROC_RESET)
+		return sprintf(buf, "reset\n");
+	if (control & NIWD_CONTROL_PROC_INTERRUPT)
+		return sprintf(buf, "interrupt\n");
+	else
+		return sprintf(buf, "\n");
+}
+
+static ssize_t timeout_action_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	bool interrupt_not_reset;
+	u8 control;
+
+	if (!strcmp(buf, "interrupt"))
+		interrupt_not_reset = true;
+	else if (!strcmp(buf, "reset"))
+		interrupt_not_reset = false;
+	else
+		return -EINVAL;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+	if (interrupt_not_reset) {
+		control |= NIWD_CONTROL_PROC_INTERRUPT;
+		control &= !NIWD_CONTROL_PROC_RESET;
+	} else {
+		control |= NIWD_CONTROL_PROC_RESET;
+		control &= !NIWD_CONTROL_PROC_INTERRUPT;
+	}
+
+	outb(control, wdt->io_base + NIWD_CONTROL);
+
+	return count;
+}
+static DEVICE_ATTR_RW(timeout_action);
+
 static ssize_t counter_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
@@ -163,6 +243,7 @@ static ssize_t counter_store(struct device *dev, struct device_attribute *attr,
 static DEVICE_ATTR_RW(counter);
 
 static struct attribute *ni9x3x_wdt_attrs[] = {
+	&dev_attr_timeout_action.attr,
 	&dev_attr_counter.attr,
 	NULL
 };
@@ -185,6 +266,17 @@ static acpi_status ni9x3x_wdt_resources(struct acpi_resource *res, void *data)
 
 		return AE_OK;
 
+	case ACPI_RESOURCE_TYPE_IRQ:
+		if (wdt->irq != 0) {
+			dev_err(&wdt->acpi_device->dev,
+				"too many IRQ resources\n");
+			return AE_ERROR;
+		}
+
+		wdt->irq = res->data.irq.interrupts[0];
+
+		return AE_OK;
+
 	case ACPI_RESOURCE_TYPE_END_TAG:
 		return AE_OK;
 
@@ -237,7 +329,7 @@ static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
 	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
 				     ni9x3x_wdt_resources, wdt);
 	if (ACPI_FAILURE(status) || wdt->io_base == 0 ||
-	    wdt->io_size != NIWD_IO_SIZE) {
+	    wdt->io_size != NIWD_IO_SIZE || wdt->irq == 0) {
 		dev_err(&device->dev, "failed to get resources\n");
 		return -ENODEV;
 	}
@@ -250,6 +342,13 @@ static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
 
 	spin_lock_init(&wdt->lock);
 
+	ret = devm_request_irq(&device->dev, wdt->irq, ni9x3x_wdt_irq_handler,
+			       0, NI9X3X_WDT_NAME, wdt);
+	if (ret) {
+		dev_err(&device->dev, "failed to register interrupt\n");
+		return ret;
+	}
+
 	wdt->wdog.info = &ni9x3x_wdt_wdd_info;
 	wdt->wdog.ops = &ni9x3x_wdt_wdd_ops;
 	wdt->wdog.min_timeout = NIWD_MIN_TIMEOUT;
@@ -268,8 +367,9 @@ static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
 	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
 	     wdt->io_base + NIWD_CONTROL);
 
-	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n",
-		 wdt->io_base, wdt->io_base + wdt->io_size - 1);
+	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n, IRQ %d\n",
+		 wdt->io_base, wdt->io_base + wdt->io_size - 1, wdt->irq);
+
 	return 0;
 }
 
-- 
2.7.0

[RFC 4/4] watchdog: ni9x3x_wdt: Let user control watchdog mode

[Kyle Roeschley]

When first powering on, the NI 903x/913x watchdog starts in boot mode.
If not switched from boot mode to user mode within 30 seconds of power
on, the device status LED indicates an unrecoverable error. While before
this was handled automatically, now add a module parameter to disable
this behavior.

In order to still have a useful watchdog, also add a R/W sysfs attribute
`mode' which can be used to switch the watchdog mode from boot to user
mode (but not user to boot mode).

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
---
 drivers/watchdog/ni9x3x_wdt.c | 52 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
index 1fce4d0..ba660b0 100644
--- a/drivers/watchdog/ni9x3x_wdt.c
+++ b/drivers/watchdog/ni9x3x_wdt.c
@@ -46,6 +46,14 @@
 
 #define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
 
+static bool auto_mode_switch = true;
+module_param(auto_mode_switch, bool, 0);
+MODULE_PARM_DESC(auto_mode_switch,
+		 "Automatically switch to user mode on driver load. If not"
+		 "set, the user must manually switch the timer within 30"
+		 "seconds of power on or the system will reset."
+		 "(default=true)");
+
 struct ni9x3x_wdt {
 	struct acpi_device *acpi_device;
 	u16 io_base;
@@ -242,9 +250,48 @@ static ssize_t counter_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(counter);
 
+static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	u8 control;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+
+	control &= NIWD_CONTROL_MODE;
+
+	return sprintf(buf, "%s\n", control ? "boot" : "user");
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct acpi_device *acpi_dev = to_acpi_device(dev);
+	struct ni9x3x_wdt *wdt = acpi_driver_data(acpi_dev);
+	u8 control;
+
+	/* You can only switch boot->user */
+	if (strcmp(buf, "user"))
+		return -EINVAL;
+
+	control = inb(wdt->io_base + NIWD_CONTROL);
+
+	/* Check if we're already in user mode */
+	if (!(control & NIWD_CONTROL_MODE))
+		return count;
+
+	control = NIWD_CONTROL_MODE | NIWD_CONTROL_RESET;
+	outb(control, wdt->io_base + NIWD_CONTROL);
+
+	return count;
+}
+static DEVICE_ATTR_RW(mode);
+
 static struct attribute *ni9x3x_wdt_attrs[] = {
 	&dev_attr_timeout_action.attr,
 	&dev_attr_counter.attr,
+	&dev_attr_mode.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ni9x3x_wdt);
@@ -364,8 +411,9 @@ static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
 	}
 
 	/* Switch from boot mode to user mode */
-	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
-	     wdt->io_base + NIWD_CONTROL);
+	if (auto_mode_switch)
+		outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
+		     wdt->io_base + NIWD_CONTROL);
 
 	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n, IRQ %d\n",
 		 wdt->io_base, wdt->io_base + wdt->io_size - 1, wdt->irq);
-- 
2.7.0

Re: [RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Josh Cartwright]

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

On Thu, Feb 04, 2016 at 07:28:00PM -0600, Kyle Roeschley wrote:
> Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> time controllers.
> 
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
[..]
> +++ b/drivers/watchdog/ni9x3x_wdt.c
> @@ -0,0 +1,264 @@
[..]
> +#define NIWD_CONTROL_MODE		0x80
> +#define NIWD_CONTROL_PROC_RESET		0x20
> +#define NIWD_CONTROL_PET		0x10
> +#define NIWD_CONTROL_RUNNING		0x08
> +#define NIWD_CONTROL_CAPTURECOUNTER	0x04
> +#define NIWD_CONTROL_RESET		0x02
> +#define NIWD_CONTROL_ALARM		0x01
> +
> +#define NIWD_PERIOD_NS		30720
> +#define NIWD_MIN_TIMEOUT	1
> +#define NIWD_MAX_TIMEOUT	515
> +#define NIWD_DEFAULT_TIMEOUT	60
> +
> +#define NI9X3X_WDT_NAME		"ni9x3x_wdt"
> +
> +#define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
> +
> +struct ni9x3x_wdt {
> +	struct acpi_device *acpi_device;

Probably a bit easier if you just cache the 'struct device *' instead.
Or, just use wdog.parent.

> +	u16 io_base;
> +	u16 io_size;

You don't actually need io_size after probe().  I'm wondering if you
could just move the devm_request_region directly into your
acpi_walk_resources callback and drop this.

> +	spinlock_t lock;

This isn't used at all?  (I'm assuming it might be used in a later
patch, but if so, it should be introduced there).

> +	struct watchdog_device wdog;
> +};
> +
> +static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
> +{
> +	u8 control = inb(wdt->io_base + NIWD_CONTROL);
> +
> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> +}
> +
> +static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
> +				      unsigned int timeout)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +
> +	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> +	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> +	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> +
> +	return 0;
> +}
> +
> +static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u8 control, counter0, counter1, counter2;
> +	u32 counter;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> +	outb(control, wdt->io_base + NIWD_CONTROL);
> +
> +	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
> +	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
> +	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
> +
> +	counter = (counter2 << 16) | (counter1 << 8) | counter0;
> +	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> +
> +	return (unsigned int)counter;

Nit: unnecessary cast.

> +}
[..]
> +static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
> +{
> +	struct ni9x3x_wdt *wdt;
> +	acpi_status status;
> +	int ret;
> +
> +	wdt = devm_kzalloc(&device->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	device->driver_data = wdt;
> +	wdt->acpi_device = device;
> +
> +	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +				     ni9x3x_wdt_resources, wdt);
> +	if (ACPI_FAILURE(status) || wdt->io_base == 0 ||
> +	    wdt->io_size != NIWD_IO_SIZE) {
> +		dev_err(&device->dev, "failed to get resources\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!devm_request_region(&device->dev, wdt->io_base, wdt->io_size,
> +				 NI9X3X_WDT_NAME)) {
> +		dev_err(&device->dev, "failed to get memory region\n");
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->wdog.info = &ni9x3x_wdt_wdd_info;
> +	wdt->wdog.ops = &ni9x3x_wdt_wdd_ops;
> +	wdt->wdog.min_timeout = NIWD_MIN_TIMEOUT;
> +	wdt->wdog.max_timeout = NIWD_MAX_TIMEOUT;
> +	wdt->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
> +	wdt->wdog.parent = &device->dev;
> +
> +	ret = watchdog_register_device(&wdt->wdog);
> +	if (ret) {
> +		dev_err(&device->dev, "failed to register watchdog\n");
> +		return ret;
> +	}
> +
> +	/* Switch from boot mode to user mode */
> +	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
> +	     wdt->io_base + NIWD_CONTROL);
> +
> +	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n",
> +		 wdt->io_base, wdt->io_base + wdt->io_size - 1);

dev_dbg(), please.  Booting is loud enough.

[..]
> +static struct acpi_driver ni9x3x_wdt_acpi_driver = {
> +	.name = NI9X3X_WDT_NAME,
> +	.ids = ni9x3x_wdt_device_ids,
> +	.ops = {
> +		.add = ni9x3x_wdt_acpi_add,
> +		.remove = ni9x3x_wdt_acpi_remove,
> +		},
> +};
> +
> +static int __init ni9x3x_wdt_init(void)
> +{
> +	return acpi_bus_register_driver(&ni9x3x_wdt_acpi_driver);
> +}
> +
> +static void __exit ni9x3x_wdt_exit(void)
> +{
> +	acpi_bus_unregister_driver(&ni9x3x_wdt_acpi_driver);
> +}
> +
> +module_init(ni9x3x_wdt_init);
> +module_exit(ni9x3x_wdt_exit);

module_acpi_driver(ni9x3x_wdt_acpi_driver)?

> +
> +MODULE_DEVICE_TABLE(acpi, ni9x3x_wdt_device_ids);

Suggestion: move this right below the table.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 2/4] watchdog: ni9x3x_wdt: Add counter sysfs attribute

[Josh Cartwright]

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

On Thu, Feb 04, 2016 at 07:28:01PM -0600, Kyle Roeschley wrote:
> The NI 9x3x watchdog timer has 30.72 µs resolution, so expose this
> by allowing the user to set the timeout counter directly via a sysfs
> attribute.

But _why_?  That isn't clear to me.  Why do we want a user to directly
muck with the counter value?  (As opposed to working in units of time?).

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 3/4] watchdog: ni9x3x_wdt: Add timeout_action sysfs attribute

[Josh Cartwright]

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

On Thu, Feb 04, 2016 at 07:28:02PM -0600, Kyle Roeschley wrote:
[..]
> diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> index 13f5c10..1fce4d0 100644
> --- a/drivers/watchdog/ni9x3x_wdt.c
> +++ b/drivers/watchdog/ni9x3x_wdt.c
> @@ -15,6 +15,7 @@
>  #include <linux/acpi.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/sysfs.h>
>  #include <linux/watchdog.h>
>  
>  #define NIWD_CONTROL	0x01
> @@ -28,6 +29,7 @@
>  #define NIWD_IO_SIZE	0x08
>  
>  #define NIWD_CONTROL_MODE		0x80
> +#define NIWD_CONTROL_PROC_INTERRUPT	0x40
>  #define NIWD_CONTROL_PROC_RESET		0x20
>  #define NIWD_CONTROL_PET		0x10
>  #define NIWD_CONTROL_RUNNING		0x08
> @@ -48,6 +50,7 @@ struct ni9x3x_wdt {
>  	struct acpi_device *acpi_device;
>  	u16 io_base;
>  	u16 io_size;
> +	u32 irq;

Interrupts have type 'int'.

>  	spinlock_t lock;
>  	struct watchdog_device wdog;
>  };
> @@ -101,6 +104,34 @@ static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
>  	return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000);
>  }
>  
> +static irqreturn_t ni9x3x_wdt_irq_handler(int irq, void *data)
> +{
> +	struct ni9x3x_wdt *wdt = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	u8 control;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt->lock, flags);

Interrupts are disabled already, so the _irqsave()/_irqrestore() is
unnecessary.

> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +
> +	if (!(NIWD_CONTROL_ALARM & control)) {
> +		dev_err(&wdt->acpi_device->dev,
> +			"Spurious watchdog interrupt, 0x%02X\n", control);

No need to print anything here, just return IRQ_NONE.  The irq core will
print a much better (and rate-limited) error when/if this occurs.

> +		goto out_unlock;
> +	}
> +
> +	/* Acknowledge the interrupt. */
> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +
> +	ret = IRQ_HANDLED;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&wdt->lock, flags);
> +
> +	return ret;
> +}

This is...strange.  You're allowing a user to enable an interrupt reset
action, but...there is no way that a user can actually _do_ anything
when that action occurs.

I'm reminded of one of these:

  https://www.youtube.com/watch?v=aqAUmgE3WyM

(The only reason I can think of where this would be a legitimate thing
to do was if we were relying on the watchdog interrupt to bring the CPU
out of a low power state...but AFAIK that's not something we do).

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 4/4] watchdog: ni9x3x_wdt: Let user control watchdog mode

[Josh Cartwright]

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

On Thu, Feb 04, 2016 at 07:28:03PM -0600, Kyle Roeschley wrote:
> When first powering on, the NI 903x/913x watchdog starts in boot mode.
> If not switched from boot mode to user mode within 30 seconds of power
> on, the device status LED indicates an unrecoverable error. While before
> this was handled automatically, now add a module parameter to disable
> this behavior.
> 
> In order to still have a useful watchdog, also add a R/W sysfs attribute
> `mode' which can be used to switch the watchdog mode from boot to user
> mode (but not user to boot mode).

Hmm.  Instead of an attribute, why don't we just ensure the watchdog is
in "user" mode in start()?

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 4/4] watchdog: ni9x3x_wdt: Let user control watchdog mode

[Kyle Roeschley]

On Fri, Feb 05, 2016 at 02:27:20PM -0600, Josh Cartwright wrote:
> On Thu, Feb 04, 2016 at 07:28:03PM -0600, Kyle Roeschley wrote:
> > When first powering on, the NI 903x/913x watchdog starts in boot mode.
> > If not switched from boot mode to user mode within 30 seconds of power
> > on, the device status LED indicates an unrecoverable error. While before
> > this was handled automatically, now add a module parameter to disable
> > this behavior.
> > 
> > In order to still have a useful watchdog, also add a R/W sysfs attribute
> > `mode' which can be used to switch the watchdog mode from boot to user
> > mode (but not user to boot mode).
> 
> Hmm.  Instead of an attribute, why don't we just ensure the watchdog is
> in "user" mode in start()?
> 
>   Josh

Seems like a good idea, except for the fact that we then lose a way to query
the current watchdog mode. Maybe we could only enable the `mode' attribute
(and possibly make it read-only) when auto_mode_switch is set to false?

Kyle

Re: [RFC 3/4] watchdog: ni9x3x_wdt: Add timeout_action sysfs attribute

[Kyle Roeschley]

On Fri, Feb 05, 2016 at 02:08:03PM -0600, Josh Cartwright wrote:
> On Thu, Feb 04, 2016 at 07:28:02PM -0600, Kyle Roeschley wrote:
> [..]
> > diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> > index 13f5c10..1fce4d0 100644
> > --- a/drivers/watchdog/ni9x3x_wdt.c
> > +++ b/drivers/watchdog/ni9x3x_wdt.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/module.h>
> > +#include <linux/sysfs.h>
> >  #include <linux/watchdog.h>
> >  
> >  #define NIWD_CONTROL	0x01
> > @@ -28,6 +29,7 @@
> >  #define NIWD_IO_SIZE	0x08
> >  
> >  #define NIWD_CONTROL_MODE		0x80
> > +#define NIWD_CONTROL_PROC_INTERRUPT	0x40
> >  #define NIWD_CONTROL_PROC_RESET		0x20
> >  #define NIWD_CONTROL_PET		0x10
> >  #define NIWD_CONTROL_RUNNING		0x08
> > @@ -48,6 +50,7 @@ struct ni9x3x_wdt {
> >  	struct acpi_device *acpi_device;
> >  	u16 io_base;
> >  	u16 io_size;
> > +	u32 irq;
> 
> Interrupts have type 'int'.
> 
> >  	spinlock_t lock;
> >  	struct watchdog_device wdog;
> >  };
> > @@ -101,6 +104,34 @@ static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
> >  	return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000);
> >  }
> >  
> > +static irqreturn_t ni9x3x_wdt_irq_handler(int irq, void *data)
> > +{
> > +	struct ni9x3x_wdt *wdt = data;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&wdt->lock, flags);
> 
> Interrupts are disabled already, so the _irqsave()/_irqrestore() is
> unnecessary.
> 
> > +
> > +	control = inb(wdt->io_base + NIWD_CONTROL);
> > +
> > +	if (!(NIWD_CONTROL_ALARM & control)) {
> > +		dev_err(&wdt->acpi_device->dev,
> > +			"Spurious watchdog interrupt, 0x%02X\n", control);
> 
> No need to print anything here, just return IRQ_NONE.  The irq core will
> print a much better (and rate-limited) error when/if this occurs.
> 
> > +		goto out_unlock;
> > +	}
> > +
> > +	/* Acknowledge the interrupt. */
> > +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> > +
> > +	ret = IRQ_HANDLED;
> > +
> > +out_unlock:
> > +	spin_unlock_irqrestore(&wdt->lock, flags);
> > +
> > +	return ret;
> > +}
> 
> This is...strange.  You're allowing a user to enable an interrupt reset
> action, but...there is no way that a user can actually _do_ anything
> when that action occurs.
> 
> I'm reminded of one of these:
> 
>   https://www.youtube.com/watch?v=aqAUmgE3WyM
> 
> (The only reason I can think of where this would be a legitimate thing
> to do was if we were relying on the watchdog interrupt to bring the CPU
> out of a low power state...but AFAIK that's not something we do).
> 
>   Josh

This is another reason (that I forgot to mention) for this being an RFC.
Ideally we'd poll or select the /dev/watchdogN file to wait on interrupt, but
this isn't supported by the watchdog core. My next instinct would be to have an
sysfs attribute named interrupt or event which a user could read/poll/select
to look for a 1 value indicating that an interrupt has occurred. Else, maybe
polling support could be added to the core, but I don't expect it would be very
useful.

Kyle

Re: [RFC 4/4] watchdog: ni9x3x_wdt: Let user control watchdog mode

[Josh Cartwright]

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

On Fri, Feb 05, 2016 at 03:03:28PM -0600, Kyle Roeschley wrote:
> On Fri, Feb 05, 2016 at 02:27:20PM -0600, Josh Cartwright wrote:
> > On Thu, Feb 04, 2016 at 07:28:03PM -0600, Kyle Roeschley wrote:
> > > When first powering on, the NI 903x/913x watchdog starts in boot mode.
> > > If not switched from boot mode to user mode within 30 seconds of power
> > > on, the device status LED indicates an unrecoverable error. While before
> > > this was handled automatically, now add a module parameter to disable
> > > this behavior.
> > > 
> > > In order to still have a useful watchdog, also add a R/W sysfs attribute
> > > `mode' which can be used to switch the watchdog mode from boot to user
> > > mode (but not user to boot mode).
> > 
> > Hmm.  Instead of an attribute, why don't we just ensure the watchdog is
> > in "user" mode in start()?
> > 
> >   Josh
> 
> Seems like a good idea, except for the fact that we then lose a way to query
> the current watchdog mode.

Is that a problem?  It isn't clear what a user would even do with that
information if they had it.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 3/4] watchdog: ni9x3x_wdt: Add timeout_action sysfs attribute

[Josh Cartwright]

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

On Fri, Feb 05, 2016 at 03:13:52PM -0600, Kyle Roeschley wrote:
> On Fri, Feb 05, 2016 at 02:08:03PM -0600, Josh Cartwright wrote:
> > On Thu, Feb 04, 2016 at 07:28:02PM -0600, Kyle Roeschley wrote:
[..]
> > This is...strange.  You're allowing a user to enable an interrupt reset
> > action, but...there is no way that a user can actually _do_ anything
> > when that action occurs.
> > 
> > I'm reminded of one of these:
> > 
> >   https://www.youtube.com/watch?v=aqAUmgE3WyM
> > 
> > (The only reason I can think of where this would be a legitimate thing
> > to do was if we were relying on the watchdog interrupt to bring the CPU
> > out of a low power state...but AFAIK that's not something we do).
> > 
> >   Josh
> 
> This is another reason (that I forgot to mention) for this being an RFC.
> Ideally we'd poll or select the /dev/watchdogN file to wait on interrupt, but
> this isn't supported by the watchdog core. My next instinct would be to have an
> sysfs attribute named interrupt or event which a user could read/poll/select
> to look for a 1 value indicating that an interrupt has occurred. Else, maybe
> polling support could be added to the core, but I don't expect it would be very
> useful.

There are already plenty of timer interfaces a user can use for managing
timeouts.  We don't need to invent another one.

Now, we did enumerate what might be a legitimate use case, which is when
an external signal is being used to feed/pet/kick, but that
functionality isn't supported in the cRIO/cDAQ case, so it shouldn't be
included in your patchset.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 3/4] watchdog: ni9x3x_wdt: Add timeout_action sysfs attribute

[Guenter Roeck]

On 02/05/2016 01:13 PM, Kyle Roeschley wrote:
> On Fri, Feb 05, 2016 at 02:08:03PM -0600, Josh Cartwright wrote:
>> On Thu, Feb 04, 2016 at 07:28:02PM -0600, Kyle Roeschley wrote:
>> [..]
>>> diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
>>> index 13f5c10..1fce4d0 100644
>>> --- a/drivers/watchdog/ni9x3x_wdt.c
>>> +++ b/drivers/watchdog/ni9x3x_wdt.c
>>> @@ -15,6 +15,7 @@
>>>   #include <linux/acpi.h>
>>>   #include <linux/interrupt.h>
>>>   #include <linux/module.h>
>>> +#include <linux/sysfs.h>
>>>   #include <linux/watchdog.h>
>>>
>>>   #define NIWD_CONTROL	0x01
>>> @@ -28,6 +29,7 @@
>>>   #define NIWD_IO_SIZE	0x08
>>>
>>>   #define NIWD_CONTROL_MODE		0x80
>>> +#define NIWD_CONTROL_PROC_INTERRUPT	0x40
>>>   #define NIWD_CONTROL_PROC_RESET		0x20
>>>   #define NIWD_CONTROL_PET		0x10
>>>   #define NIWD_CONTROL_RUNNING		0x08
>>> @@ -48,6 +50,7 @@ struct ni9x3x_wdt {
>>>   	struct acpi_device *acpi_device;
>>>   	u16 io_base;
>>>   	u16 io_size;
>>> +	u32 irq;
>>
>> Interrupts have type 'int'.
>>
>>>   	spinlock_t lock;
>>>   	struct watchdog_device wdog;
>>>   };
>>> @@ -101,6 +104,34 @@ static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
>>>   	return (unsigned int)((counter * (u64)NIWD_PERIOD_NS) / 1000000000);
>>>   }
>>>
>>> +static irqreturn_t ni9x3x_wdt_irq_handler(int irq, void *data)
>>> +{
>>> +	struct ni9x3x_wdt *wdt = data;
>>> +	irqreturn_t ret = IRQ_NONE;
>>> +	u8 control;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&wdt->lock, flags);
>>
>> Interrupts are disabled already, so the _irqsave()/_irqrestore() is
>> unnecessary.
>>
>>> +
>>> +	control = inb(wdt->io_base + NIWD_CONTROL);
>>> +
>>> +	if (!(NIWD_CONTROL_ALARM & control)) {
>>> +		dev_err(&wdt->acpi_device->dev,
>>> +			"Spurious watchdog interrupt, 0x%02X\n", control);
>>
>> No need to print anything here, just return IRQ_NONE.  The irq core will
>> print a much better (and rate-limited) error when/if this occurs.
>>
>>> +		goto out_unlock;
>>> +	}
>>> +
>>> +	/* Acknowledge the interrupt. */
>>> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
>>> +
>>> +	ret = IRQ_HANDLED;
>>> +
>>> +out_unlock:
>>> +	spin_unlock_irqrestore(&wdt->lock, flags);
>>> +
>>> +	return ret;
>>> +}
>>
>> This is...strange.  You're allowing a user to enable an interrupt reset
>> action, but...there is no way that a user can actually _do_ anything
>> when that action occurs.
>>
>> I'm reminded of one of these:
>>
>>    https://www.youtube.com/watch?v=aqAUmgE3WyM
>>
>> (The only reason I can think of where this would be a legitimate thing
>> to do was if we were relying on the watchdog interrupt to bring the CPU
>> out of a low power state...but AFAIK that's not something we do).
>>
>>    Josh
>
> This is another reason (that I forgot to mention) for this being an RFC.
> Ideally we'd poll or select the /dev/watchdogN file to wait on interrupt, but
> this isn't supported by the watchdog core. My next instinct would be to have an
> sysfs attribute named interrupt or event which a user could read/poll/select
> to look for a 1 value indicating that an interrupt has occurred. Else, maybe
> polling support could be added to the core, but I don't expect it would be very
> useful.
>

I _really_ get the impression that you are trying to use the watchdog
subsystem for something it isn't supposed to support. poll/select on the
watchdog device is pretty much the opposite of what should happen.
the watchdog device node is intended to be written to to trigger a
heartbeat, not a device to use to wait for an interrupt. There is
no way we'll ever add support for this into the watchdog subsystem.

I'll really have to spend much more time on this, probably much more than
I have. I would suggest to either implement a traditional, standard,
watchdog driver, or consider implementing a non-watchdog driver if
your use case is that much different.

Thanks,
Guenter

Re: [RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Guenter Roeck]

On 02/04/2016 05:28 PM, Kyle Roeschley wrote:
> Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> time controllers.
>
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> ---
>   drivers/watchdog/Kconfig      |  11 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/ni9x3x_wdt.c | 264 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 276 insertions(+)
>   create mode 100644 drivers/watchdog/ni9x3x_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0f6d851..18bd13a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config NI9X3X_WDT
> +	tristate "NI 903x/913x Watchdog"
> +	depends on X86 && ACPI
> +	select WATCHDOG_CORE
> +	---help---
> +	  This is the driver for the watchdog timer on the National Instruments
> +	  903x/913x real-time controllers.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ni9x3x_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f566753..527978b 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
>   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
>   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
>   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> +obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
>
>   # M32R Architecture
>
> diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> new file mode 100644
> index 0000000..2cb5627
> --- /dev/null
> +++ b/drivers/watchdog/ni9x3x_wdt.c
> @@ -0,0 +1,264 @@
> +/*
> + * Copyright (C) 2013 National Instruments Corp.
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +
> +#define NIWD_CONTROL	0x01
> +#define NIWD_COUNTER2	0x02
> +#define NIWD_COUNTER1	0x03
> +#define NIWD_COUNTER0	0x04
> +#define NIWD_SEED2	0x05
> +#define NIWD_SEED1	0x06
> +#define NIWD_SEED0	0x07
> +
> +#define NIWD_IO_SIZE	0x08
> +
> +#define NIWD_CONTROL_MODE		0x80
> +#define NIWD_CONTROL_PROC_RESET		0x20
> +#define NIWD_CONTROL_PET		0x10
> +#define NIWD_CONTROL_RUNNING		0x08
> +#define NIWD_CONTROL_CAPTURECOUNTER	0x04
> +#define NIWD_CONTROL_RESET		0x02
> +#define NIWD_CONTROL_ALARM		0x01
> +
> +#define NIWD_PERIOD_NS		30720
> +#define NIWD_MIN_TIMEOUT	1
> +#define NIWD_MAX_TIMEOUT	515
> +#define NIWD_DEFAULT_TIMEOUT	60
> +
> +#define NI9X3X_WDT_NAME		"ni9x3x_wdt"
> +
> +#define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
> +
> +struct ni9x3x_wdt {
> +	struct acpi_device *acpi_device;
> +	u16 io_base;
> +	u16 io_size;
> +	spinlock_t lock;
> +	struct watchdog_device wdog;
> +};
> +
> +static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
> +{
> +	u8 control = inb(wdt->io_base + NIWD_CONTROL);
> +
> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> +}
> +
> +static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
> +				      unsigned int timeout)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +
> +	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> +	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> +	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> +
> +	return 0;
> +}
> +
> +static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u8 control, counter0, counter1, counter2;
> +	u32 counter;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> +	outb(control, wdt->io_base + NIWD_CONTROL);
> +
> +	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
> +	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
> +	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
> +
> +	counter = (counter2 << 16) | (counter1 << 8) | counter0;
> +	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> +

The maximum value of 'counter' from the registers is
	515 * (1000000000/30720) = 16764280

Multiplying this value by 30720 yields 514998681600, much more than
fits in a 32 bit value. This means you'll have to use u64 for the
counter variable and use 64 bit operations for the divide operation.

Guenter

Re: [RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Guenter Roeck]

On 02/04/2016 05:28 PM, Kyle Roeschley wrote:
> Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> time controllers.
>
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> ---
>   drivers/watchdog/Kconfig      |  11 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/ni9x3x_wdt.c | 264 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 276 insertions(+)
>   create mode 100644 drivers/watchdog/ni9x3x_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0f6d851..18bd13a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called sbc_epx_c3.
>
> +config NI9X3X_WDT
> +	tristate "NI 903x/913x Watchdog"
> +	depends on X86 && ACPI
> +	select WATCHDOG_CORE
> +	---help---
> +	  This is the driver for the watchdog timer on the National Instruments
> +	  903x/913x real-time controllers.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ni9x3x_wdt.
> +
>   # M32R Architecture
>
>   # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f566753..527978b 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
>   obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
>   obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
>   obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> +obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
>
>   # M32R Architecture
>
> diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> new file mode 100644
> index 0000000..2cb5627
> --- /dev/null
> +++ b/drivers/watchdog/ni9x3x_wdt.c
> @@ -0,0 +1,264 @@
> +/*
> + * Copyright (C) 2013 National Instruments Corp.
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +
> +#define NIWD_CONTROL	0x01
> +#define NIWD_COUNTER2	0x02
> +#define NIWD_COUNTER1	0x03
> +#define NIWD_COUNTER0	0x04
> +#define NIWD_SEED2	0x05
> +#define NIWD_SEED1	0x06
> +#define NIWD_SEED0	0x07
> +
> +#define NIWD_IO_SIZE	0x08
> +
> +#define NIWD_CONTROL_MODE		0x80
> +#define NIWD_CONTROL_PROC_RESET		0x20
> +#define NIWD_CONTROL_PET		0x10
> +#define NIWD_CONTROL_RUNNING		0x08
> +#define NIWD_CONTROL_CAPTURECOUNTER	0x04
> +#define NIWD_CONTROL_RESET		0x02
> +#define NIWD_CONTROL_ALARM		0x01
> +
> +#define NIWD_PERIOD_NS		30720
> +#define NIWD_MIN_TIMEOUT	1
> +#define NIWD_MAX_TIMEOUT	515
> +#define NIWD_DEFAULT_TIMEOUT	60
> +
> +#define NI9X3X_WDT_NAME		"ni9x3x_wdt"
> +
> +#define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
> +
> +struct ni9x3x_wdt {
> +	struct acpi_device *acpi_device;
> +	u16 io_base;
> +	u16 io_size;
> +	spinlock_t lock;
> +	struct watchdog_device wdog;
> +};
> +
> +static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
> +{
> +	u8 control = inb(wdt->io_base + NIWD_CONTROL);
> +
> +	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> +}
> +
> +static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
> +				      unsigned int timeout)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +
> +	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> +	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> +	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> +

This function has to set wdd->timeout to the actually configured timeout value,
ie
	wdd->timeout = timeout;

is missing.

Guenter

> +	return 0;
> +}
> +
> +static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u8 control, counter0, counter1, counter2;
> +	u32 counter;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> +	outb(control, wdt->io_base + NIWD_CONTROL);
> +
> +	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
> +	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
> +	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
> +
> +	counter = (counter2 << 16) | (counter1 << 8) | counter0;
> +	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> +
> +	return (unsigned int)counter;
> +}
> +
> +static int ni9x3x_wdt_wdd_start(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +
> +	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_PROC_RESET,
> +	     wdt->io_base + NIWD_CONTROL);
> +
> +	ni9x3x_wdt_wdd_set_timeout(wdd, wdd->timeout);
> +	ni9x3x_wdt_start(wdt);
> +
> +	return 0;
> +}
> +
> +static int ni9x3x_wdt_wdd_stop(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +
> +	outb(NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> +
> +	return 0;
> +}
> +
> +static int ni9x3x_wdt_wdd_ping(struct watchdog_device *wdd)
> +{
> +	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> +	u8 control;
> +
> +	control = inb(wdt->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> +
> +	return 0;
> +}
> +
> +static acpi_status ni9x3x_wdt_resources(struct acpi_resource *res, void *data)
> +{
> +	struct ni9x3x_wdt *wdt = data;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_IO:
> +		if (wdt->io_base != 0 || wdt->io_size != 0) {
> +			dev_err(&wdt->acpi_device->dev,
> +				 "too many IO resources\n");
> +			return AE_ERROR;
> +		}
> +
> +		wdt->io_base = res->data.io.minimum;
> +		wdt->io_size = res->data.io.address_length;
> +
> +		return AE_OK;
> +
> +	case ACPI_RESOURCE_TYPE_END_TAG:
> +		return AE_OK;
> +
> +	default:
> +		dev_err(&wdt->acpi_device->dev,
> +			"unsupported resource type %d\n", res->type);
> +		return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static int ni9x3x_wdt_acpi_remove(struct acpi_device *device)
> +{
> +	struct ni9x3x_wdt *wdt = acpi_driver_data(device);
> +
> +	ni9x3x_wdt_wdd_stop(&wdt->wdog);
> +	watchdog_unregister_device(&wdt->wdog);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info ni9x3x_wdt_wdd_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.identity = "NI Watchdog",
> +};
> +
> +static const struct watchdog_ops ni9x3x_wdt_wdd_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= ni9x3x_wdt_wdd_start,
> +	.stop		= ni9x3x_wdt_wdd_stop,
> +	.ping		= ni9x3x_wdt_wdd_ping,
> +	.set_timeout	= ni9x3x_wdt_wdd_set_timeout,
> +	.get_timeleft	= ni9x3x_wdt_wdd_get_timeleft,
> +};
> +
> +static int ni9x3x_wdt_acpi_add(struct acpi_device *device)
> +{
> +	struct ni9x3x_wdt *wdt;
> +	acpi_status status;
> +	int ret;
> +
> +	wdt = devm_kzalloc(&device->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	device->driver_data = wdt;
> +	wdt->acpi_device = device;
> +
> +	status = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +				     ni9x3x_wdt_resources, wdt);
> +	if (ACPI_FAILURE(status) || wdt->io_base == 0 ||
> +	    wdt->io_size != NIWD_IO_SIZE) {
> +		dev_err(&device->dev, "failed to get resources\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!devm_request_region(&device->dev, wdt->io_base, wdt->io_size,
> +				 NI9X3X_WDT_NAME)) {
> +		dev_err(&device->dev, "failed to get memory region\n");
> +		return -EBUSY;
> +	}
> +
> +	spin_lock_init(&wdt->lock);
> +
> +	wdt->wdog.info = &ni9x3x_wdt_wdd_info;
> +	wdt->wdog.ops = &ni9x3x_wdt_wdd_ops;
> +	wdt->wdog.min_timeout = NIWD_MIN_TIMEOUT;
> +	wdt->wdog.max_timeout = NIWD_MAX_TIMEOUT;
> +	wdt->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
> +	wdt->wdog.parent = &device->dev;
> +
> +	ret = watchdog_register_device(&wdt->wdog);
> +	if (ret) {
> +		dev_err(&device->dev, "failed to register watchdog\n");
> +		return ret;
> +	}
> +
> +	/* Switch from boot mode to user mode */
> +	outb(NIWD_CONTROL_RESET | NIWD_CONTROL_MODE,
> +	     wdt->io_base + NIWD_CONTROL);
> +
> +	dev_info(&wdt->acpi_device->dev, "IO range 0x%04X-0x%04X\n",
> +		 wdt->io_base, wdt->io_base + wdt->io_size - 1);
> +	return 0;
> +}
> +
> +static const struct acpi_device_id ni9x3x_wdt_device_ids[] = {
> +	{"NIC775C", 0},
> +	{"", 0},
> +};
> +
> +static struct acpi_driver ni9x3x_wdt_acpi_driver = {
> +	.name = NI9X3X_WDT_NAME,
> +	.ids = ni9x3x_wdt_device_ids,
> +	.ops = {
> +		.add = ni9x3x_wdt_acpi_add,
> +		.remove = ni9x3x_wdt_acpi_remove,
> +		},
> +};
> +
> +static int __init ni9x3x_wdt_init(void)
> +{
> +	return acpi_bus_register_driver(&ni9x3x_wdt_acpi_driver);
> +}
> +
> +static void __exit ni9x3x_wdt_exit(void)
> +{
> +	acpi_bus_unregister_driver(&ni9x3x_wdt_acpi_driver);
> +}
> +
> +module_init(ni9x3x_wdt_init);
> +module_exit(ni9x3x_wdt_exit);
> +
> +MODULE_DEVICE_TABLE(acpi, ni9x3x_wdt_device_ids);
> +MODULE_DESCRIPTION("NI Watchdog");
> +MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
> +MODULE_AUTHOR("Kyle Roeschley <kyle.roeschley@ni.com>");
> +MODULE_LICENSE("GPL v2");
>

Re: [RFC 2/4] watchdog: ni9x3x_wdt: Add counter sysfs attribute

[Guenter Roeck]

On 02/05/2016 11:53 AM, Josh Cartwright wrote:
> On Thu, Feb 04, 2016 at 07:28:01PM -0600, Kyle Roeschley wrote:
>> The NI 9x3x watchdog timer has 30.72 µs resolution, so expose this
>> by allowing the user to set the timeout counter directly via a sysfs
>> attribute.
>
> But _why_?  That isn't clear to me.  Why do we want a user to directly
> muck with the counter value?  (As opposed to working in units of time?).
>

In addition to that, this also messes up the watchdog core's notion of
wdd->timeout, meaning the watchdog subsystem would no longer report
the actually configured timeout value to user space.

Guenter

Re: [RFC 4/4] watchdog: ni9x3x_wdt: Let user control watchdog mode

[Guenter Roeck]

On 02/05/2016 01:03 PM, Kyle Roeschley wrote:
> On Fri, Feb 05, 2016 at 02:27:20PM -0600, Josh Cartwright wrote:
>> On Thu, Feb 04, 2016 at 07:28:03PM -0600, Kyle Roeschley wrote:
>>> When first powering on, the NI 903x/913x watchdog starts in boot mode.
>>> If not switched from boot mode to user mode within 30 seconds of power
>>> on, the device status LED indicates an unrecoverable error. While before
>>> this was handled automatically, now add a module parameter to disable
>>> this behavior.
>>>
>>> In order to still have a useful watchdog, also add a R/W sysfs attribute
>>> `mode' which can be used to switch the watchdog mode from boot to user
>>> mode (but not user to boot mode).
>>
>> Hmm.  Instead of an attribute, why don't we just ensure the watchdog is
>> in "user" mode in start()?
>>
>>    Josh
>
> Seems like a good idea, except for the fact that we then lose a way to query
> the current watchdog mode. Maybe we could only enable the `mode' attribute
> (and possibly make it read-only) when auto_mode_switch is set to false?

If the system is still running after 30 seconds, it is in user mode.
So your query would have to be pretty quick. If you want to know if the
watchdog is running, which might be more important for the user,
you could check the 'state' attribute instead.

Guenter

Re: [RFC 1/4] watchdog: ni9x3x_wdt: Add NI 903x/913x watchdog driver

[Kyle Roeschley]

On Sat, Feb 06, 2016 at 08:30:08AM -0800, Guenter Roeck wrote:
> On 02/04/2016 05:28 PM, Kyle Roeschley wrote:
> >Add support for the watchdog timer on NI cRIO-903x and cDAQ-913x real-
> >time controllers.
> >
> >Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> >Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> >---
> >  drivers/watchdog/Kconfig      |  11 ++
> >  drivers/watchdog/Makefile     |   1 +
> >  drivers/watchdog/ni9x3x_wdt.c | 264 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 276 insertions(+)
> >  create mode 100644 drivers/watchdog/ni9x3x_wdt.c
> >
> >diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >index 0f6d851..18bd13a 100644
> >--- a/drivers/watchdog/Kconfig
> >+++ b/drivers/watchdog/Kconfig
> >@@ -1214,6 +1214,17 @@ config SBC_EPX_C3_WATCHDOG
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called sbc_epx_c3.
> >
> >+config NI9X3X_WDT
> >+	tristate "NI 903x/913x Watchdog"
> >+	depends on X86 && ACPI
> >+	select WATCHDOG_CORE
> >+	---help---
> >+	  This is the driver for the watchdog timer on the National Instruments
> >+	  903x/913x real-time controllers.
> >+
> >+	  To compile this driver as a module, choose M here: the module will be
> >+	  called ni9x3x_wdt.
> >+
> >  # M32R Architecture
> >
> >  # M68K Architecture
> >diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> >index f566753..527978b 100644
> >--- a/drivers/watchdog/Makefile
> >+++ b/drivers/watchdog/Makefile
> >@@ -126,6 +126,7 @@ obj-$(CONFIG_MACHZ_WDT) += machzwd.o
> >  obj-$(CONFIG_SBC_EPX_C3_WATCHDOG) += sbc_epx_c3.o
> >  obj-$(CONFIG_INTEL_SCU_WATCHDOG) += intel_scu_watchdog.o
> >  obj-$(CONFIG_INTEL_MID_WATCHDOG) += intel-mid_wdt.o
> >+obj-$(CONFIG_NI9X3X_WDT) += ni9x3x_wdt.o
> >
> >  # M32R Architecture
> >
> >diff --git a/drivers/watchdog/ni9x3x_wdt.c b/drivers/watchdog/ni9x3x_wdt.c
> >new file mode 100644
> >index 0000000..2cb5627
> >--- /dev/null
> >+++ b/drivers/watchdog/ni9x3x_wdt.c
> >@@ -0,0 +1,264 @@
> >+/*
> >+ * Copyright (C) 2013 National Instruments Corp.
> >+ *
> >+ * 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; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ */
> >+
> >+#include <linux/acpi.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/module.h>
> >+#include <linux/watchdog.h>
> >+
> >+#define NIWD_CONTROL	0x01
> >+#define NIWD_COUNTER2	0x02
> >+#define NIWD_COUNTER1	0x03
> >+#define NIWD_COUNTER0	0x04
> >+#define NIWD_SEED2	0x05
> >+#define NIWD_SEED1	0x06
> >+#define NIWD_SEED0	0x07
> >+
> >+#define NIWD_IO_SIZE	0x08
> >+
> >+#define NIWD_CONTROL_MODE		0x80
> >+#define NIWD_CONTROL_PROC_RESET		0x20
> >+#define NIWD_CONTROL_PET		0x10
> >+#define NIWD_CONTROL_RUNNING		0x08
> >+#define NIWD_CONTROL_CAPTURECOUNTER	0x04
> >+#define NIWD_CONTROL_RESET		0x02
> >+#define NIWD_CONTROL_ALARM		0x01
> >+
> >+#define NIWD_PERIOD_NS		30720
> >+#define NIWD_MIN_TIMEOUT	1
> >+#define NIWD_MAX_TIMEOUT	515
> >+#define NIWD_DEFAULT_TIMEOUT	60
> >+
> >+#define NI9X3X_WDT_NAME		"ni9x3x_wdt"
> >+
> >+#define to_ni9x3x_wdt(_wdog)	container_of(_wdog, struct ni9x3x_wdt, wdog)
> >+
> >+struct ni9x3x_wdt {
> >+	struct acpi_device *acpi_device;
> >+	u16 io_base;
> >+	u16 io_size;
> >+	spinlock_t lock;
> >+	struct watchdog_device wdog;
> >+};
> >+
> >+static void ni9x3x_wdt_start(struct ni9x3x_wdt *wdt)
> >+{
> >+	u8 control = inb(wdt->io_base + NIWD_CONTROL);
> >+
> >+	outb(control | NIWD_CONTROL_RESET, wdt->io_base + NIWD_CONTROL);
> >+	outb(control | NIWD_CONTROL_PET, wdt->io_base + NIWD_CONTROL);
> >+}
> >+
> >+static int ni9x3x_wdt_wdd_set_timeout(struct watchdog_device *wdd,
> >+				      unsigned int timeout)
> >+{
> >+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> >+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> >+
> >+	outb(((0x00FF0000 & counter) >> 16), wdt->io_base + NIWD_SEED2);
> >+	outb(((0x0000FF00 & counter) >> 8), wdt->io_base + NIWD_SEED1);
> >+	outb((0x000000FF & counter), wdt->io_base + NIWD_SEED0);
> >+
> >+	return 0;
> >+}
> >+
> >+static unsigned int ni9x3x_wdt_wdd_get_timeleft(struct watchdog_device *wdd)
> >+{
> >+	struct ni9x3x_wdt *wdt = to_ni9x3x_wdt(wdd);
> >+	u8 control, counter0, counter1, counter2;
> >+	u32 counter;
> >+
> >+	control = inb(wdt->io_base + NIWD_CONTROL);
> >+	control |= NIWD_CONTROL_CAPTURECOUNTER;
> >+	outb(control, wdt->io_base + NIWD_CONTROL);
> >+
> >+	counter2 = inb(wdt->io_base + NIWD_COUNTER2);
> >+	counter1 = inb(wdt->io_base + NIWD_COUNTER1);
> >+	counter0 = inb(wdt->io_base + NIWD_COUNTER0);
> >+
> >+	counter = (counter2 << 16) | (counter1 << 8) | counter0;
> >+	counter = (counter * (u64)NIWD_PERIOD_NS) / 1000000000;
> >+
> 
> The maximum value of 'counter' from the registers is
> 	515 * (1000000000/30720) = 16764280
> 
> Multiplying this value by 30720 yields 514998681600, much more than
> fits in a 32 bit value. This means you'll have to use u64 for the
> counter variable and use 64 bit operations for the divide operation.
> 
> Guenter
> 

A u32 * u64 is a u64, and a u64 / u32 is a u64, so counter doesn't need to be
u64. Unless you meant I should move the cast to counter or use a u64 counter
for clarity's sake, in which case sure. I've also tested this with an
explicitly 32-bit counter and 64-bit NIWD_PERIOD_NS with no problems, i.e.
	sizeof(uint32_t): 4
	sizeof(uint64_t): 8
	sizeof(counter * (u64)NIWD_PERIOD_NS): 8
	sizeof((counter * (u64)NIWD_PERIOD_NS)/1000000000): 8
	register = 16777215 -> counter value = 514

Kyle