[PATCH 1/2] watchdog: add NI 903x/913x watchdog support

[PATCH 1/2] watchdog: add NI 903x/913x watchdog support

[Kyle Roeschley]

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

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 drivers/watchdog/Kconfig        |  11 +
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/niwatchdog.c   | 507 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/Kbuild       |   1 +
 include/uapi/linux/niwatchdog.h |  30 +++
 5 files changed, 550 insertions(+)
 create mode 100644 drivers/watchdog/niwatchdog.c
 create mode 100644 include/uapi/linux/niwatchdog.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f0e7be..cf7f2c2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1212,6 +1212,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 NI_WATCHDOG
+	bool "NI Watchdog support for NI 903x/913x"
+	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 niwatchdog.
+
 # M32R Architecture
 
 # M68K Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f566753..3cb8ebf 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_NI_WATCHDOG) += niwatchdog.o
 
 # M32R Architecture
 
diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
new file mode 100644
index 0000000..3908846
--- /dev/null
+++ b/drivers/watchdog/niwatchdog.c
@@ -0,0 +1,507 @@
+/*
+ * 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/niwatchdog.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_INTERRUPT	0x40
+#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_MAX_COUNTER	0x00FFFFFF
+#define NIWD_MIN_TIMEOUT	1
+#define NIWD_MAX_TIMEOUT	515
+#define NIWD_DEFAULT_TIMEOUT	60
+
+#define to_niwatchdog(_wdog)	container_of(_wdog, struct niwatchdog, wdog)
+
+struct niwatchdog {
+	struct acpi_device *acpi_device;
+	u16 io_base;
+	u16 io_size;
+	u32 irq;
+	spinlock_t lock;
+	struct watchdog_device wdog;
+	atomic_t available;
+	wait_queue_head_t irq_event;
+	u32 running:1;
+	u32 expired:1;
+};
+
+static ssize_t niwatchdog_wdmode_get(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct acpi_device *acpi_device = to_acpi_device(dev);
+	struct niwatchdog *niwatchdog = acpi_device->driver_data;
+	u8 data;
+
+	data = inb(niwatchdog->io_base + NIWD_CONTROL);
+
+	data &= NIWD_CONTROL_MODE;
+
+	return sprintf(buf, "%s\n", data ? "boot" : "user");
+}
+
+static ssize_t niwatchdog_wdmode_set(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct acpi_device *acpi_device = to_acpi_device(dev);
+	struct niwatchdog *niwatchdog = acpi_device->driver_data;
+	u8 data;
+
+	/* you can only switch boot->user */
+	if (strcmp(buf, "user"))
+		return -EINVAL;
+
+	data = inb(niwatchdog->io_base + NIWD_CONTROL);
+
+	/* Check if we're already in user mode. */
+	if (!(data & NIWD_CONTROL_MODE))
+		return count;
+
+	data = NIWD_CONTROL_MODE | NIWD_CONTROL_RESET;
+
+	outb(data, niwatchdog->io_base + NIWD_CONTROL);
+
+	return count;
+}
+
+static DEVICE_ATTR(watchdog_mode, S_IRUSR|S_IWUSR, niwatchdog_wdmode_get,
+	niwatchdog_wdmode_set);
+
+static const struct attribute *niwatchdog_attrs[] = {
+	&dev_attr_watchdog_mode.attr,
+	NULL
+};
+
+static int niwatchdog_counter_set(struct niwatchdog *niwatchdog, u32 counter)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	/* Prevent changing the counter while the watchdog is running. */
+	if (niwatchdog->running) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	outb(((0x00FF0000 & counter) >> 16), niwatchdog->io_base + NIWD_SEED2);
+	outb(((0x0000FF00 & counter) >> 8), niwatchdog->io_base + NIWD_SEED1);
+	outb((0x000000FF & counter), niwatchdog->io_base + NIWD_SEED0);
+
+	ret = 0;
+
+out_unlock:
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+
+	return ret;
+}
+
+static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
+{
+	u8 action_mask;
+	u8 control;
+	unsigned long flags;
+
+	if (action == NIWATCHDOG_ACTION_INTERRUPT)
+		action_mask = NIWD_CONTROL_PROC_INTERRUPT;
+	else if (action == NIWATCHDOG_ACTION_RESET)
+		action_mask = NIWD_CONTROL_PROC_RESET;
+	else
+		return -ENOTSUPP;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
+	control |= action_mask;
+	outb(control, niwatchdog->io_base + NIWD_CONTROL);
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+
+	return 0;
+}
+
+static void niwatchdog_start(struct niwatchdog *niwatchdog)
+{
+	u8 control;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	niwatchdog->running = true;
+	niwatchdog->expired = false;
+
+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
+	outb(control | NIWD_CONTROL_PET, niwatchdog->io_base + NIWD_CONTROL);
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+}
+
+static void niwatchdog_pet(struct niwatchdog *niwatchdog, u32 *state)
+{
+	u8 control;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	if (niwatchdog->expired) {
+		*state = NIWATCHDOG_STATE_EXPIRED;
+	} else if (!niwatchdog->running) {
+		*state = NIWATCHDOG_STATE_DISABLED;
+	} else {
+		control = inb(niwatchdog->io_base + NIWD_CONTROL);
+		control |= NIWD_CONTROL_PET;
+		outb(control, niwatchdog->io_base + NIWD_CONTROL);
+
+		*state = NIWATCHDOG_STATE_RUNNING;
+	}
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+}
+
+static void niwatchdog_reset(struct niwatchdog *niwatchdog)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	niwatchdog->running = false;
+	niwatchdog->expired = false;
+
+	outb(NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+}
+
+static void niwatchdog_counter_get(struct niwatchdog *niwatchdog, u32 *counter)
+{
+	u8 control, counter2, counter1, counter0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
+	control |= NIWD_CONTROL_CAPTURECOUNTER;
+	outb(control, niwatchdog->io_base + NIWD_CONTROL);
+
+	counter2 = inb(niwatchdog->io_base + NIWD_COUNTER2);
+	counter1 = inb(niwatchdog->io_base + NIWD_COUNTER1);
+	counter0 = inb(niwatchdog->io_base + NIWD_COUNTER0);
+
+	*counter = (counter2 << 16) | (counter1 << 8) | counter0;
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+}
+
+static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
+				      unsigned int timeout)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
+
+	niwatchdog_reset(niwatchdog);
+	niwatchdog_counter_set(niwatchdog, counter);
+	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
+
+	return niwatchdog_start(niwatchdog);
+}
+
+static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+	u32 counter;
+
+	niwatchdog_counter_get(niwatchdog, &counter);
+	return (unsigned int)((counter * (unsigned long)NIWD_PERIOD_NS) /
+			      1000000000);
+}
+
+static irqreturn_t niwatchdog_irq(int irq, void *data)
+{
+	struct niwatchdog *niwatchdog = data;
+	irqreturn_t ret = IRQ_NONE;
+	u8 control;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
+
+	if (!(NIWD_CONTROL_ALARM & control)) {
+		dev_err(&niwatchdog->acpi_device->dev,
+			"Spurious watchdog interrupt, 0x%02X\n", control);
+		goto out_unlock;
+	}
+
+	niwatchdog->running = false;
+	niwatchdog->expired = true;
+
+	/* Acknowledge the interrupt. */
+	control |= NIWD_CONTROL_RESET;
+	outb(control, niwatchdog->io_base + NIWD_CONTROL);
+
+	/* Signal the watchdog event. */
+	wake_up_all(&niwatchdog->irq_event);
+
+	ret = IRQ_HANDLED;
+
+out_unlock:
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+
+	return ret;
+}
+
+static int niwatchdog_wdd_open(struct watchdog_device *wdd)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+	u32 counter;
+
+	if (!atomic_dec_and_test(&niwatchdog->available)) {
+		atomic_inc(&niwatchdog->available);
+		return -EBUSY;
+	}
+
+	niwatchdog_reset(niwatchdog);
+
+	counter = wdd->timeout * (1000000000 / NIWD_PERIOD_NS);
+	niwatchdog_counter_set(niwatchdog, counter);
+	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
+
+	niwatchdog_start(niwatchdog);
+
+	return request_irq(niwatchdog->irq, niwatchdog_irq, 0,
+			   NIWATCHDOG_NAME, niwatchdog);
+}
+
+static int niwatchdog_wdd_release(struct watchdog_device *wdd)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+
+	niwatchdog_reset(niwatchdog);
+
+	free_irq(niwatchdog->irq, niwatchdog);
+	atomic_inc(&niwatchdog->available);
+	return 0;
+}
+
+static int niwatchdog_ping(struct watchdog_device *wdd)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+	__u32 state;
+
+	niwatchdog_pet(niwatchdog, &state);
+	if (state != NIWATCHDOG_STATE_RUNNING)
+		return -state;
+
+	return 0;
+}
+
+static acpi_status niwatchdog_resources(struct acpi_resource *res, void *data)
+{
+	struct niwatchdog *niwatchdog = data;
+
+	switch (res->type) {
+	case ACPI_RESOURCE_TYPE_IO:
+		if ((niwatchdog->io_base != 0) ||
+		    (niwatchdog->io_size != 0)) {
+			dev_err(&niwatchdog->acpi_device->dev,
+			"too many IO resources\n");
+			return AE_ERROR;
+		}
+
+		niwatchdog->io_base = res->data.io.minimum;
+		niwatchdog->io_size = res->data.io.address_length;
+
+		return AE_OK;
+
+	case ACPI_RESOURCE_TYPE_IRQ:
+		if (niwatchdog->irq != 0) {
+			dev_err(&niwatchdog->acpi_device->dev,
+				"too many IRQ resources\n");
+			return AE_ERROR;
+		}
+
+		niwatchdog->irq = res->data.irq.interrupts[0];
+
+		return AE_OK;
+
+	case ACPI_RESOURCE_TYPE_END_TAG:
+		return AE_OK;
+
+	default:
+		dev_err(&niwatchdog->acpi_device->dev,
+			"unsupported resource type %d\n",
+			res->type);
+		return AE_ERROR;
+	}
+
+	return AE_OK;
+}
+
+static int niwatchdog_acpi_remove(struct acpi_device *device)
+{
+	struct niwatchdog *niwatchdog = device->driver_data;
+
+	watchdog_unregister_device(&niwatchdog->wdog);
+
+	sysfs_remove_files(&niwatchdog->acpi_device->dev.kobj,
+			   niwatchdog_attrs);
+
+	if ((niwatchdog->io_base != 0) &&
+	    (niwatchdog->io_size == NIWD_IO_SIZE))
+		release_region(niwatchdog->io_base, niwatchdog->io_size);
+
+	device->driver_data = NULL;
+
+	kfree(niwatchdog);
+
+	return 0;
+}
+
+static const struct watchdog_info niwatchdog_wdd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "NI Watchdog",
+};
+
+static const struct watchdog_ops niwatchdog_wdd_ops = {
+	.owner		= THIS_MODULE,
+	.start		= niwatchdog_wdd_open,
+	.stop		= niwatchdog_wdd_release,
+	.ping		= niwatchdog_ping,
+	.set_timeout	= niwatchdog_wdd_set_timeout,
+	.get_timeleft	= niwatchdog_wdd_get_timeleft,
+};
+
+static int niwatchdog_acpi_add(struct acpi_device *device)
+{
+	struct niwatchdog *niwatchdog;
+	acpi_status acpi_ret;
+	int err;
+
+	niwatchdog = kzalloc(sizeof(*niwatchdog), GFP_KERNEL);
+
+	if (!niwatchdog)
+		return -ENOMEM;
+
+	device->driver_data = niwatchdog;
+
+	niwatchdog->acpi_device = device;
+
+	acpi_ret = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
+				       niwatchdog_resources, niwatchdog);
+
+	if (ACPI_FAILURE(acpi_ret) ||
+	    (niwatchdog->io_base == 0) ||
+	    (niwatchdog->io_size != NIWD_IO_SIZE) ||
+	    (niwatchdog->irq == 0)) {
+		niwatchdog_acpi_remove(device);
+		return -ENODEV;
+	}
+
+	if (!request_region(niwatchdog->io_base, niwatchdog->io_size,
+	    NIWATCHDOG_NAME)) {
+		niwatchdog_acpi_remove(device);
+		return -EBUSY;
+	}
+
+	err = sysfs_create_files(&niwatchdog->acpi_device->dev.kobj,
+				 niwatchdog_attrs);
+	if (err) {
+		niwatchdog_acpi_remove(device);
+		return err;
+	}
+
+	spin_lock_init(&niwatchdog->lock);
+
+	atomic_set(&niwatchdog->available, 1);
+	init_waitqueue_head(&niwatchdog->irq_event);
+	niwatchdog->expired = false;
+
+	niwatchdog->wdog.info = &niwatchdog_wdd_info;
+	niwatchdog->wdog.ops = &niwatchdog_wdd_ops;
+	niwatchdog->wdog.min_timeout = NIWD_MIN_TIMEOUT;
+	niwatchdog->wdog.max_timeout = NIWD_MAX_TIMEOUT;
+	niwatchdog->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
+	niwatchdog->wdog.parent = &device->dev;
+	err = watchdog_register_device(&niwatchdog->wdog);
+
+	if (err) {
+		niwatchdog_acpi_remove(device);
+		return err;
+	}
+
+	dev_info(&niwatchdog->acpi_device->dev,
+		 "IO range 0x%04X-0x%04X, IRQ %d\n",
+		 niwatchdog->io_base,
+		 niwatchdog->io_base + niwatchdog->io_size - 1,
+		 niwatchdog->irq);
+
+	return 0;
+}
+
+static const struct acpi_device_id niwatchdog_device_ids[] = {
+	{"NIC775C", 0},
+	{"", 0},
+};
+
+static struct acpi_driver niwatchdog_acpi_driver = {
+	.name = NIWATCHDOG_NAME,
+	.ids = niwatchdog_device_ids,
+	.ops = {
+		.add = niwatchdog_acpi_add,
+		.remove = niwatchdog_acpi_remove,
+		},
+};
+
+static int __init niwatchdog_init(void)
+{
+	return acpi_bus_register_driver(&niwatchdog_acpi_driver);
+}
+
+static void __exit niwatchdog_exit(void)
+{
+	acpi_bus_unregister_driver(&niwatchdog_acpi_driver);
+}
+
+module_init(niwatchdog_init);
+module_exit(niwatchdog_exit);
+
+MODULE_DEVICE_TABLE(acpi, niwatchdog_device_ids);
+MODULE_DESCRIPTION("NI Watchdog");
+MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 628e6e6..8c5dde3 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -303,6 +303,7 @@ header-y += nfs_fs.h
 header-y += nfs.h
 header-y += nfs_idmap.h
 header-y += nfs_mount.h
+header-y += niwatchdog.h
 header-y += nl80211.h
 header-y += n_r3964.h
 header-y += nubus.h
diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
new file mode 100644
index 0000000..9d3613d
--- /dev/null
+++ b/include/uapi/linux/niwatchdog.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2012 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.
+ */
+
+#ifndef _LINUX_NIWATCHDOG_H_
+#define _LINUX_NIWATCHDOG_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#define NIWATCHDOG_ACTION_INTERRUPT	0
+#define NIWATCHDOG_ACTION_RESET		1
+
+#define NIWATCHDOG_STATE_RUNNING	0
+#define NIWATCHDOG_STATE_EXPIRED	1
+#define NIWATCHDOG_STATE_DISABLED	2
+
+#define NIWATCHDOG_NAME			"niwatchdog"
+
+#endif /* _LINUX_NIWATCHDOG_H_ */
-- 
2.6.4

[PATCH 2/2] niwatchdog: add support for custom ioctls

[Kyle Roeschley]

Add custom ioctls for features specific to the NI watchdog, including
disabling the watchdog, changing timeout behavior, and setting timeouts
with sub-second resolution.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/niwatchdog.h |  10 ++++
 2 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
index 3908846..fe637a3 100644
--- a/drivers/watchdog/niwatchdog.c
+++ b/drivers/watchdog/niwatchdog.c
@@ -131,6 +131,35 @@ out_unlock:
 	return ret;
 }
 
+static int niwatchdog_check_action(u32 action)
+{
+	int err = 0;
+
+	switch (action) {
+	case NIWD_CONTROL_PROC_INTERRUPT:
+	case NIWD_CONTROL_PROC_RESET:
+		break;
+	default:
+		err = -ENOTSUPP;
+	}
+
+	return err;
+}
+
+static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
+{
+	u8 control;
+	unsigned long flags;
+
+	spin_lock_irqsave(&niwatchdog->lock, flags);
+
+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
+	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
+			      NIWD_CONTROL_PROC_RESET);
+
+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
+}
+
 static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
 {
 	u8 action_mask;
@@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
 {
 	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
 	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
+	u8 actions;
 
+	niwatchdog_get_actions(niwatchdog, &actions);
 	niwatchdog_reset(niwatchdog);
 	niwatchdog_counter_set(niwatchdog, counter);
-	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
 
-	return niwatchdog_start(niwatchdog);
+	if (actions & NIWD_CONTROL_PROC_RESET)
+		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
+	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
+		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
+
+	niwatchdog_start(niwatchdog);
+
+	return 0;
 }
 
 static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
@@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
 	return 0;
 }
 
+static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
+	int err = 0;
+
+	switch (cmd) {
+	case NIWATCHDOG_IOCTL_PERIOD_NS: {
+		__u32 period = NIWD_PERIOD_NS;
+
+		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
+		break;
+	}
+	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
+		__u32 counter = NIWD_MAX_COUNTER;
+
+		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
+		break;
+	}
+	case NIWATCHDOG_IOCTL_COUNTER_SET: {
+		__u32 counter;
+
+		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
+		if (!err)
+			err = niwatchdog_counter_set(niwatchdog, counter);
+		break;
+	}
+	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
+		__u32 action;
+
+		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
+		if (!err)
+			err = niwatchdog_check_action(action);
+		break;
+	}
+	case NIWATCHDOG_IOCTL_ADD_ACTION: {
+		__u32 action;
+
+		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
+		if (!err)
+			err = niwatchdog_add_action(niwatchdog, action);
+		break;
+	}
+	case NIWATCHDOG_IOCTL_START: {
+		niwatchdog_start(niwatchdog);
+		break;
+	}
+	case NIWATCHDOG_IOCTL_PET: {
+		__u32 state;
+
+		niwatchdog_pet(niwatchdog, &state);
+		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
+		break;
+	}
+	case NIWATCHDOG_IOCTL_RESET: {
+		niwatchdog_reset(niwatchdog);
+		break;
+	}
+	case NIWATCHDOG_IOCTL_COUNTER_GET: {
+		__u32 counter;
+
+		niwatchdog_counter_get(niwatchdog, &counter);
+		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
+		break;
+	}
+	default:
+		err = -ENOIOCTLCMD;
+		break;
+	};
+
+	return err;
+}
+
 static int niwatchdog_ping(struct watchdog_device *wdd)
 {
 	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
@@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
 	.start		= niwatchdog_wdd_open,
 	.stop		= niwatchdog_wdd_release,
 	.ping		= niwatchdog_ping,
+	.ioctl		= niwatchdog_wdd_ioctl,
 	.set_timeout	= niwatchdog_wdd_set_timeout,
 	.get_timeleft	= niwatchdog_wdd_get_timeleft,
 };
diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
index 9d3613d..4fb8d47 100644
--- a/include/uapi/linux/niwatchdog.h
+++ b/include/uapi/linux/niwatchdog.h
@@ -25,6 +25,16 @@
 #define NIWATCHDOG_STATE_EXPIRED	1
 #define NIWATCHDOG_STATE_DISABLED	2
 
+#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
+#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
+#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
+#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
+#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
+#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
+#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
+#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
+#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
+
 #define NIWATCHDOG_NAME			"niwatchdog"
 
 #endif /* _LINUX_NIWATCHDOG_H_ */
-- 
2.6.4

Re: [1/2] watchdog: add NI 903x/913x watchdog support

[Guenter Roeck]

Hi,

On Mon, Jan 11, 2016 at 06:23:00PM -0600, Kyle Roeschley wrote:
> This patch adds support for the watchdog timer on NI cRIO-903x and
> cDAQ-913x real-time controllers.
> 

Add support" ... is sufficient.

> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> ---
>  drivers/watchdog/Kconfig        |  11 +
>  drivers/watchdog/Makefile       |   1 +
>  drivers/watchdog/niwatchdog.c   | 507 ++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/Kbuild       |   1 +
>  include/uapi/linux/niwatchdog.h |  30 +++
>  5 files changed, 550 insertions(+)
>  create mode 100644 drivers/watchdog/niwatchdog.c
>  create mode 100644 include/uapi/linux/niwatchdog.h
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 4f0e7be..cf7f2c2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1212,6 +1212,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 NI_WATCHDOG
> +	bool "NI Watchdog support for NI 903x/913x"
> +	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 niwatchdog.
> +
>  # M32R Architecture
>  
>  # M68K Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f566753..3cb8ebf 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_NI_WATCHDOG) += niwatchdog.o
>  
>  # M32R Architecture
>  
> diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
> new file mode 100644
> index 0000000..3908846
> --- /dev/null
> +++ b/drivers/watchdog/niwatchdog.c
> @@ -0,0 +1,507 @@
> +/*
> + * 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/niwatchdog.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_INTERRUPT	0x40
> +#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_MAX_COUNTER	0x00FFFFFF

Not used anywhere.

> +#define NIWD_MIN_TIMEOUT	1
> +#define NIWD_MAX_TIMEOUT	515
> +#define NIWD_DEFAULT_TIMEOUT	60
> +
> +#define to_niwatchdog(_wdog)	container_of(_wdog, struct niwatchdog, wdog)
> +
> +struct niwatchdog {
> +	struct acpi_device *acpi_device;
> +	u16 io_base;
> +	u16 io_size;
> +	u32 irq;
> +	spinlock_t lock;
> +	struct watchdog_device wdog;
> +	atomic_t available;

I do not see the point of this variable.

> +	wait_queue_head_t irq_event;

I do not see the point of this wait queue.

> +	u32 running:1;
> +	u32 expired:1;

Should be bool, though I don't see the point of the variables
in the first place.

There is no standardized means to return "expired" as response
to a watchdog ping, and you can not just invent one. Please stick
with the ABI.

Overall, the driver seems way more complex than needed, to quite some
degree because of non-standard functionality. I would suggest to reduce
it to standard functionality to simplify both the driver and its review.

> +};
> +
> +static ssize_t niwatchdog_wdmode_get(struct device *dev,
> +				     struct device_attribute *attr,
> +				     char *buf)
> +{
> +	struct acpi_device *acpi_device = to_acpi_device(dev);
> +	struct niwatchdog *niwatchdog = acpi_device->driver_data;

Please use acpi_driver_data().

> +	u8 data;
> +
> +	data = inb(niwatchdog->io_base + NIWD_CONTROL);
> +
> +	data &= NIWD_CONTROL_MODE;
> +
> +	return sprintf(buf, "%s\n", data ? "boot" : "user");
> +}
> +
> +static ssize_t niwatchdog_wdmode_set(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct acpi_device *acpi_device = to_acpi_device(dev);
> +	struct niwatchdog *niwatchdog = acpi_device->driver_data;
> +	u8 data;
> +
> +	/* you can only switch boot->user */
> +	if (strcmp(buf, "user"))
> +		return -EINVAL;
> +
> +	data = inb(niwatchdog->io_base + NIWD_CONTROL);
> +
> +	/* Check if we're already in user mode. */
> +	if (!(data & NIWD_CONTROL_MODE))
> +		return count;
> +
> +	data = NIWD_CONTROL_MODE | NIWD_CONTROL_RESET;
> +
> +	outb(data, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	return count;
> +}

I don't immediately see where this attribute adds value.
You might as well switch to 'user' mode when the watchdog
is started (opened) for the first time.

> +
> +static DEVICE_ATTR(watchdog_mode, S_IRUSR|S_IWUSR, niwatchdog_wdmode_get,
> +	niwatchdog_wdmode_set);
> +
Please align continuation lines consistently with '('.

> +static const struct attribute *niwatchdog_attrs[] = {
> +	&dev_attr_watchdog_mode.attr,
> +	NULL
> +};
> +
> +static int niwatchdog_counter_set(struct niwatchdog *niwatchdog, u32 counter)
> +{
> +	int ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	/* Prevent changing the counter while the watchdog is running. */
> +	if (niwatchdog->running) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
This is unusual and problematic, since standard applications
_will_ change the timeout while the watchdog is running.

If the counter can not be changed while the watchdog is running,
you'll have to stop it while updating the timer.

> +	outb(((0x00FF0000 & counter) >> 16), niwatchdog->io_base + NIWD_SEED2);
> +	outb(((0x0000FF00 & counter) >> 8), niwatchdog->io_base + NIWD_SEED1);
> +	outb((0x000000FF & counter), niwatchdog->io_base + NIWD_SEED0);
> +
> +	ret = 0;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
> +{
> +	u8 action_mask;
> +	u8 control;
> +	unsigned long flags;
> +
> +	if (action == NIWATCHDOG_ACTION_INTERRUPT)
> +		action_mask = NIWD_CONTROL_PROC_INTERRUPT;

This function is never called with NIWATCHDOG_ACTION_INTERRUPT
as parameter. Which makes me wonder what the interrupt handler
is all about.

> +	else if (action == NIWATCHDOG_ACTION_RESET)
> +		action_mask = NIWD_CONTROL_PROC_RESET;
> +	else
> +		return -ENOTSUPP;
> +

If you want to use separate actions 'RESET' and 'INTERRUPT',
you have a number of options.

- provide a module parameter to determine the desired action.
- provide a sysfs attribute to determien the desired action
  (less preferred)
- use the pretimeout functionality, and have the driver generate
  an interrupt if a pretimeout is configured (followed by a hard reset
  if the watchdog times out again).

> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +	control |= action_mask;
> +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void niwatchdog_start(struct niwatchdog *niwatchdog)
> +{
> +	u8 control;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	niwatchdog->running = true;
> +	niwatchdog->expired = false;
> +
> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
> +	outb(control | NIWD_CONTROL_PET, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +}
> +
> +static void niwatchdog_pet(struct niwatchdog *niwatchdog, u32 *state)
> +{
> +	u8 control;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	if (niwatchdog->expired) {
> +		*state = NIWATCHDOG_STATE_EXPIRED;
> +	} else if (!niwatchdog->running) {
> +		*state = NIWATCHDOG_STATE_DISABLED;

This function will never be called if the watchdog is not running.

> +	} else {
> +		control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +		control |= NIWD_CONTROL_PET;
> +		outb(control, niwatchdog->io_base + NIWD_CONTROL);
> +
> +		*state = NIWATCHDOG_STATE_RUNNING;
> +	}
> +

Please consider using just standard watchdog functionality.
If the watchdog core requests a ping, execute a ping. All the other
functionality is non-standard and just adds unnecessary complexity
to the driver.

> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +}
> +
> +static void niwatchdog_reset(struct niwatchdog *niwatchdog)
> +{

May be better named niwatchdog_stop().

> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	niwatchdog->running = false;
> +	niwatchdog->expired = false;
> +
As mentioned above, those variables are not needed.

> +	outb(NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +}
> +
> +static void niwatchdog_counter_get(struct niwatchdog *niwatchdog, u32 *counter)
> +{

There is no reason to return the value in a pointer.
Plese use the function return value.

> +	u8 control, counter2, counter1, counter0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	counter2 = inb(niwatchdog->io_base + NIWD_COUNTER2);
> +	counter1 = inb(niwatchdog->io_base + NIWD_COUNTER1);
> +	counter0 = inb(niwatchdog->io_base + NIWD_COUNTER0);
> +
> +	*counter = (counter2 << 16) | (counter1 << 8) | counter0;
> +
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +}
> +
> +static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
> +				      unsigned int timeout)
> +{
> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +
> +	niwatchdog_reset(niwatchdog);
> +	niwatchdog_counter_set(niwatchdog, counter);
> +	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> +
> +	return niwatchdog_start(niwatchdog);
> +}
> +
> +static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +	u32 counter;
> +
> +	niwatchdog_counter_get(niwatchdog, &counter);
> +	return (unsigned int)((counter * (unsigned long)NIWD_PERIOD_NS) /
> +			      1000000000);

This can overflow if sizeof(unsigned long) == 4.
Please use u64 for the calculations.

> +}
> +
> +static irqreturn_t niwatchdog_irq(int irq, void *data)
> +{
> +	struct niwatchdog *niwatchdog = data;
> +	irqreturn_t ret = IRQ_NONE;
> +	u8 control;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +
> +	if (!(NIWD_CONTROL_ALARM & control)) {
> +		dev_err(&niwatchdog->acpi_device->dev,
> +			"Spurious watchdog interrupt, 0x%02X\n", control);
> +		goto out_unlock;
> +	}
> +
> +	niwatchdog->running = false;
> +	niwatchdog->expired = true;
> +
> +	/* Acknowledge the interrupt. */
> +	control |= NIWD_CONTROL_RESET;
> +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> +
> +	/* Signal the watchdog event. */
> +	wake_up_all(&niwatchdog->irq_event);
> +
Nothing is ever waiting here.

The functionality of this interrupt handler is unusual.
Normal expectation for it would be to reset the system.
It is highly unusual (and unacceptable) to do nothing
and (try to) return the 'expired' status with the next ping.

> +	ret = IRQ_HANDLED;
> +
> +out_unlock:
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int niwatchdog_wdd_open(struct watchdog_device *wdd)
> +{

'niwatchdog_wdd_open' is misleading and unrelated to opening the watchdog
device. Please change the function name to sonething like niwatchdog_wdd_start.

> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +	u32 counter;
> +
> +	if (!atomic_dec_and_test(&niwatchdog->available)) {
> +		atomic_inc(&niwatchdog->available);
> +		return -EBUSY;
> +	}

This function won't be called if the watchdog is already running.
The variable and the check are therefore not necessary.

> +
> +	niwatchdog_reset(niwatchdog);
> +
Strictly speaking it is not necessary to stop the watchdog here,
as it won't be running.

> +	counter = wdd->timeout * (1000000000 / NIWD_PERIOD_NS);
> +	niwatchdog_counter_set(niwatchdog, counter);
> +	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> +
Curiously, there is never a del_action() call.

> +	niwatchdog_start(niwatchdog);
> +
> +	return request_irq(niwatchdog->irq, niwatchdog_irq, 0,
> +			   NIWATCHDOG_NAME, niwatchdog);

It is highly unusual to request an interrupt line when the watchdog is
started. Please explain why it would make sense to do this here
instead of in the probe function like other drivers.

> +}
> +
> +static int niwatchdog_wdd_release(struct watchdog_device *wdd)
> +{

This would be more appropriately named niwatchdog_wdd_stop.

> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +
> +	niwatchdog_reset(niwatchdog);
> +
> +	free_irq(niwatchdog->irq, niwatchdog);
> +	atomic_inc(&niwatchdog->available);
> +	return 0;
> +}
> +
> +static int niwatchdog_ping(struct watchdog_device *wdd)
> +{
> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +	__u32 state;
> +
> +	niwatchdog_pet(niwatchdog, &state);
> +	if (state != NIWATCHDOG_STATE_RUNNING)
> +		return -state;
> +

The function won't be called if the watchdog is not running,
so this check is unnecessary. Besides, the error return values are
non-standard, which is absolutely unacceptable.

> +	return 0;
> +}
> +
> +static acpi_status niwatchdog_resources(struct acpi_resource *res, void *data)
> +{
> +	struct niwatchdog *niwatchdog = data;
> +
> +	switch (res->type) {
> +	case ACPI_RESOURCE_TYPE_IO:
> +		if ((niwatchdog->io_base != 0) ||
> +		    (niwatchdog->io_size != 0)) {
> +			dev_err(&niwatchdog->acpi_device->dev,
> +			"too many IO resources\n");

Inconsistent alignment, and unneceaasry ().

> +			return AE_ERROR;
> +		}
> +
> +		niwatchdog->io_base = res->data.io.minimum;
> +		niwatchdog->io_size = res->data.io.address_length;
> +
> +		return AE_OK;
> +
> +	case ACPI_RESOURCE_TYPE_IRQ:
> +		if (niwatchdog->irq != 0) {
> +			dev_err(&niwatchdog->acpi_device->dev,
> +				"too many IRQ resources\n");
> +			return AE_ERROR;
> +		}
> +
> +		niwatchdog->irq = res->data.irq.interrupts[0];
> +
> +		return AE_OK;
> +
> +	case ACPI_RESOURCE_TYPE_END_TAG:
> +		return AE_OK;
> +
> +	default:
> +		dev_err(&niwatchdog->acpi_device->dev,
> +			"unsupported resource type %d\n",
> +			res->type);
> +		return AE_ERROR;
> +	}
> +
> +	return AE_OK;
> +}
> +
> +static int niwatchdog_acpi_remove(struct acpi_device *device)
> +{
> +	struct niwatchdog *niwatchdog = device->driver_data;
> +
> +	watchdog_unregister_device(&niwatchdog->wdog);
> +
> +	sysfs_remove_files(&niwatchdog->acpi_device->dev.kobj,
> +			   niwatchdog_attrs);
> +
> +	if ((niwatchdog->io_base != 0) &&
> +	    (niwatchdog->io_size == NIWD_IO_SIZE))

Please no unneceassary ().

> +		release_region(niwatchdog->io_base, niwatchdog->io_size);
> +
> +	device->driver_data = NULL;
> +
Unnecessary.

> +	kfree(niwatchdog);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info niwatchdog_wdd_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.identity = "NI Watchdog",
> +};
> +
> +static const struct watchdog_ops niwatchdog_wdd_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= niwatchdog_wdd_open,
> +	.stop		= niwatchdog_wdd_release,
> +	.ping		= niwatchdog_ping,
> +	.set_timeout	= niwatchdog_wdd_set_timeout,
> +	.get_timeleft	= niwatchdog_wdd_get_timeleft,
> +};
> +
> +static int niwatchdog_acpi_add(struct acpi_device *device)
> +{
> +	struct niwatchdog *niwatchdog;
> +	acpi_status acpi_ret;
> +	int err;
> +
> +	niwatchdog = kzalloc(sizeof(*niwatchdog), GFP_KERNEL);
> +
No empty line here please.
Please use devm_kzalloc() unless you have a good reason not to.

> +	if (!niwatchdog)
> +		return -ENOMEM;
> +
> +	device->driver_data = niwatchdog;
> +
> +	niwatchdog->acpi_device = device;
> +
> +	acpi_ret = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> +				       niwatchdog_resources, niwatchdog);
> +
> +	if (ACPI_FAILURE(acpi_ret) ||
> +	    (niwatchdog->io_base == 0) ||
> +	    (niwatchdog->io_size != NIWD_IO_SIZE) ||
> +	    (niwatchdog->irq == 0)) {

Unnecessary ( ).

> +		niwatchdog_acpi_remove(device);

Please use proper return handling. 
Releaseing everything blindly even if it was not yet requested
or registered is not acceptable.

> +		return -ENODEV;
> +	}
> +
> +	if (!request_region(niwatchdog->io_base, niwatchdog->io_size,

Please use devm_request_region().

> +	    NIWATCHDOG_NAME)) {
> +		niwatchdog_acpi_remove(device);
> +		return -EBUSY;
> +	}
> +
> +	err = sysfs_create_files(&niwatchdog->acpi_device->dev.kobj,
> +				 niwatchdog_attrs);
> +	if (err) {
> +		niwatchdog_acpi_remove(device);
> +		return err;
> +	}

The watchdog core now supports a means to attach sysfs attributes
to the watchdog device. Please use this mechanism if needed
(though I would want to be convinced that the sysfs attribute
is needed in the first place).

> +
> +	spin_lock_init(&niwatchdog->lock);
> +
> +	atomic_set(&niwatchdog->available, 1);
> +	init_waitqueue_head(&niwatchdog->irq_event);
> +	niwatchdog->expired = false;
> +
> +	niwatchdog->wdog.info = &niwatchdog_wdd_info;
> +	niwatchdog->wdog.ops = &niwatchdog_wdd_ops;
> +	niwatchdog->wdog.min_timeout = NIWD_MIN_TIMEOUT;
> +	niwatchdog->wdog.max_timeout = NIWD_MAX_TIMEOUT;
> +	niwatchdog->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
> +	niwatchdog->wdog.parent = &device->dev;
> +	err = watchdog_register_device(&niwatchdog->wdog);
> +

Please no empty lines between assignments and associated error checks.

> +	if (err) {
> +		niwatchdog_acpi_remove(device);
> +		return err;
> +	}
> +
> +	dev_info(&niwatchdog->acpi_device->dev,
> +		 "IO range 0x%04X-0x%04X, IRQ %d\n",
> +		 niwatchdog->io_base,
> +		 niwatchdog->io_base + niwatchdog->io_size - 1,
> +		 niwatchdog->irq);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id niwatchdog_device_ids[] = {
> +	{"NIC775C", 0},
> +	{"", 0},
> +};
> +
> +static struct acpi_driver niwatchdog_acpi_driver = {
> +	.name = NIWATCHDOG_NAME,
> +	.ids = niwatchdog_device_ids,
> +	.ops = {
> +		.add = niwatchdog_acpi_add,
> +		.remove = niwatchdog_acpi_remove,
> +		},
> +};
> +
> +static int __init niwatchdog_init(void)
> +{
> +	return acpi_bus_register_driver(&niwatchdog_acpi_driver);
> +}
> +
> +static void __exit niwatchdog_exit(void)
> +{
> +	acpi_bus_unregister_driver(&niwatchdog_acpi_driver);
> +}
> +
> +module_init(niwatchdog_init);
> +module_exit(niwatchdog_exit);
> +
> +MODULE_DEVICE_TABLE(acpi, niwatchdog_device_ids);
> +MODULE_DESCRIPTION("NI Watchdog");
> +MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 628e6e6..8c5dde3 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -303,6 +303,7 @@ header-y += nfs_fs.h
>  header-y += nfs.h
>  header-y += nfs_idmap.h
>  header-y += nfs_mount.h
> +header-y += niwatchdog.h
>  header-y += nl80211.h
>  header-y += n_r3964.h
>  header-y += nubus.h
> diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
> new file mode 100644
> index 0000000..9d3613d
> --- /dev/null
> +++ b/include/uapi/linux/niwatchdog.h

This file is unnecessary.

First, it is only used with patch 2, and if anything should be introduced there.
Second, I do not agree that patch 2 should exist in the first place.
I'll comment on that in my comments to patch 2.

> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (C) 2012 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.
> + */
> +
> +#ifndef _LINUX_NIWATCHDOG_H_
> +#define _LINUX_NIWATCHDOG_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#define NIWATCHDOG_ACTION_INTERRUPT	0
> +#define NIWATCHDOG_ACTION_RESET		1
> +
> +#define NIWATCHDOG_STATE_RUNNING	0
> +#define NIWATCHDOG_STATE_EXPIRED	1
> +#define NIWATCHDOG_STATE_DISABLED	2
> +
> +#define NIWATCHDOG_NAME			"niwatchdog"
> +
> +#endif /* _LINUX_NIWATCHDOG_H_ */

Re: [2/2] niwatchdog: add support for custom ioctls

[Guenter Roeck]

On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
> Add custom ioctls for features specific to the NI watchdog, including
> disabling the watchdog, changing timeout behavior, and setting timeouts
> with sub-second resolution.
> 
I don't really agree with any of the added functionality.
Most of the added ioctls duplicate standard functionality.


> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> ---
>  drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/niwatchdog.h |  10 ++++
>  2 files changed, 123 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
> index 3908846..fe637a3 100644
> --- a/drivers/watchdog/niwatchdog.c
> +++ b/drivers/watchdog/niwatchdog.c
> @@ -131,6 +131,35 @@ out_unlock:
>  	return ret;
>  }
>  
> +static int niwatchdog_check_action(u32 action)
> +{
> +	int err = 0;
> +
> +	switch (action) {
> +	case NIWD_CONTROL_PROC_INTERRUPT:
> +	case NIWD_CONTROL_PROC_RESET:
> +		break;
> +	default:
> +		err = -ENOTSUPP;
> +	}
> +
> +	return err;
> +}
> +
> +static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
> +{
> +	u8 control;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&niwatchdog->lock, flags);
> +
> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> +	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
> +			      NIWD_CONTROL_PROC_RESET);
> +
> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> +}
> +
>  static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
>  {
>  	u8 action_mask;
> @@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
>  {
>  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>  	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> +	u8 actions;
>  
> +	niwatchdog_get_actions(niwatchdog, &actions);
>  	niwatchdog_reset(niwatchdog);
>  	niwatchdog_counter_set(niwatchdog, counter);
> -	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>  
> -	return niwatchdog_start(niwatchdog);
> +	if (actions & NIWD_CONTROL_PROC_RESET)
> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> +	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
> +
> +	niwatchdog_start(niwatchdog);
> +
> +	return 0;
>  }
>  
>  static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
> @@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
>  	return 0;
>  }
>  
> +static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
> +				 unsigned long arg)
> +{
> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> +	int err = 0;
> +
> +	switch (cmd) {
> +	case NIWATCHDOG_IOCTL_PERIOD_NS: {
> +		__u32 period = NIWD_PERIOD_NS;
> +
> +		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
> +		break;
> +	}
> +	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
> +		__u32 counter = NIWD_MAX_COUNTER;
> +
> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> +		break;
> +	}

Those are constants and can as well be defined in some documentation.

> +	case NIWATCHDOG_IOCTL_COUNTER_SET: {
> +		__u32 counter;
> +
> +		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
> +		if (!err)
> +			err = niwatchdog_counter_set(niwatchdog, counter);
> +		break;
> +	}
Duplicates standard functionality

> +	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
> +		__u32 action;
> +
> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> +		if (!err)
> +			err = niwatchdog_check_action(action);
> +		break;
> +	}
> +	case NIWATCHDOG_IOCTL_ADD_ACTION: {
> +		__u32 action;
> +
> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> +		if (!err)
> +			err = niwatchdog_add_action(niwatchdog, action);
> +		break;
> +	}

Use a module parameter or sysfs attribute for those.

> +	case NIWATCHDOG_IOCTL_START: {
> +		niwatchdog_start(niwatchdog);
> +		break;
> +	}

Duplicates standard functionality.

> +	case NIWATCHDOG_IOCTL_PET: {
> +		__u32 state;
> +
> +		niwatchdog_pet(niwatchdog, &state);
> +		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
> +		break;
> +	}

Duplicates standard functionality.

> +	case NIWATCHDOG_IOCTL_RESET: {
> +		niwatchdog_reset(niwatchdog);
> +		break;
> +	}

Duplicates standard functionality.

> +	case NIWATCHDOG_IOCTL_COUNTER_GET: {
> +		__u32 counter;
> +
> +		niwatchdog_counter_get(niwatchdog, &counter);
> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> +		break;
> +	}

Duplicates standard functionality.

> +	default:
> +		err = -ENOIOCTLCMD;
> +		break;
> +	};
> +
> +	return err;
> +}
> +
>  static int niwatchdog_ping(struct watchdog_device *wdd)
>  {
>  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> @@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
>  	.start		= niwatchdog_wdd_open,
>  	.stop		= niwatchdog_wdd_release,
>  	.ping		= niwatchdog_ping,
> +	.ioctl		= niwatchdog_wdd_ioctl,
>  	.set_timeout	= niwatchdog_wdd_set_timeout,
>  	.get_timeleft	= niwatchdog_wdd_get_timeleft,
>  };
> diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
> index 9d3613d..4fb8d47 100644
> --- a/include/uapi/linux/niwatchdog.h
> +++ b/include/uapi/linux/niwatchdog.h
> @@ -25,6 +25,16 @@
>  #define NIWATCHDOG_STATE_EXPIRED	1
>  #define NIWATCHDOG_STATE_DISABLED	2
>  
> +#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
> +#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
> +#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
> +#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
> +#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
> +#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
> +#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
> +#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
> +#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
> +
>  #define NIWATCHDOG_NAME			"niwatchdog"
>  
>  #endif /* _LINUX_NIWATCHDOG_H_ */

Re: [1/2] watchdog: add NI 903x/913x watchdog support

[Kyle Roeschley]

Hi Guenter,

If I haven't commented on anything below, it's because I agree and will
change/fix whatever is needed.

On Sat, Jan 16, 2016 at 08:25:35PM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Jan 11, 2016 at 06:23:00PM -0600, Kyle Roeschley wrote:
> > This patch adds support for the watchdog timer on NI cRIO-903x and
> > cDAQ-913x real-time controllers.
> > 
> 
> Add support" ... is sufficient.
> 
> > Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> > Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> > ---
> >  drivers/watchdog/Kconfig        |  11 +
> >  drivers/watchdog/Makefile       |   1 +
> >  drivers/watchdog/niwatchdog.c   | 507 ++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/Kbuild       |   1 +
> >  include/uapi/linux/niwatchdog.h |  30 +++
> >  5 files changed, 550 insertions(+)
> >  create mode 100644 drivers/watchdog/niwatchdog.c
> >  create mode 100644 include/uapi/linux/niwatchdog.h
> > 
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 4f0e7be..cf7f2c2 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1212,6 +1212,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 NI_WATCHDOG
> > +	bool "NI Watchdog support for NI 903x/913x"
> > +	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 niwatchdog.
> > +
> >  # M32R Architecture
> >  
> >  # M68K Architecture
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index f566753..3cb8ebf 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_NI_WATCHDOG) += niwatchdog.o
> >  
> >  # M32R Architecture
> >  
> > diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
> > new file mode 100644
> > index 0000000..3908846
> > --- /dev/null
> > +++ b/drivers/watchdog/niwatchdog.c
> > @@ -0,0 +1,507 @@
> > +/*
> > + * 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/niwatchdog.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_INTERRUPT	0x40
> > +#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_MAX_COUNTER	0x00FFFFFF
> 
> Not used anywhere.
> 
> > +#define NIWD_MIN_TIMEOUT	1
> > +#define NIWD_MAX_TIMEOUT	515
> > +#define NIWD_DEFAULT_TIMEOUT	60
> > +
> > +#define to_niwatchdog(_wdog)	container_of(_wdog, struct niwatchdog, wdog)
> > +
> > +struct niwatchdog {
> > +	struct acpi_device *acpi_device;
> > +	u16 io_base;
> > +	u16 io_size;
> > +	u32 irq;
> > +	spinlock_t lock;
> > +	struct watchdog_device wdog;
> > +	atomic_t available;
> 
> I do not see the point of this variable.
> 
> > +	wait_queue_head_t irq_event;
> 
> I do not see the point of this wait queue.
> 
> > +	u32 running:1;
> > +	u32 expired:1;
> 
> Should be bool, though I don't see the point of the variables
> in the first place.
> 
> There is no standardized means to return "expired" as response
> to a watchdog ping, and you can not just invent one. Please stick
> with the ABI.
> 
> Overall, the driver seems way more complex than needed, to quite some
> degree because of non-standard functionality. I would suggest to reduce
> it to standard functionality to simplify both the driver and its review.
> 

All of these were bits of the watchdog that I didn't catch when switching from
miscdevice to watchdog_device.

As far as complexity, I agree. I'm working on making the driver a watchdog core
implementation with an NI patch rather than an NI driver manipulated to work
with the core.

> > +};
> > +
> > +static ssize_t niwatchdog_wdmode_get(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     char *buf)
> > +{
> > +	struct acpi_device *acpi_device = to_acpi_device(dev);
> > +	struct niwatchdog *niwatchdog = acpi_device->driver_data;
> 
> Please use acpi_driver_data().
> 
> > +	u8 data;
> > +
> > +	data = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	data &= NIWD_CONTROL_MODE;
> > +
> > +	return sprintf(buf, "%s\n", data ? "boot" : "user");
> > +}
> > +
> > +static ssize_t niwatchdog_wdmode_set(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct acpi_device *acpi_device = to_acpi_device(dev);
> > +	struct niwatchdog *niwatchdog = acpi_device->driver_data;
> > +	u8 data;
> > +
> > +	/* you can only switch boot->user */
> > +	if (strcmp(buf, "user"))
> > +		return -EINVAL;
> > +
> > +	data = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	/* Check if we're already in user mode. */
> > +	if (!(data & NIWD_CONTROL_MODE))
> > +		return count;
> > +
> > +	data = NIWD_CONTROL_MODE | NIWD_CONTROL_RESET;
> > +
> > +	outb(data, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	return count;
> > +}
> 
> I don't immediately see where this attribute adds value.
> You might as well switch to 'user' mode when the watchdog
> is started (opened) for the first time.
> 

The value here is that the watchdog is switched from boot to user by an init
script on our controllers, the idea being that if our software doesn't start
for some reason, the controller will reboot. However, this application would be
of limited utility in the kernel, so I could move the mode switch into
niwatchdog_acpi_add().

> > +
> > +static DEVICE_ATTR(watchdog_mode, S_IRUSR|S_IWUSR, niwatchdog_wdmode_get,
> > +	niwatchdog_wdmode_set);
> > +
> Please align continuation lines consistently with '('.
> 
> > +static const struct attribute *niwatchdog_attrs[] = {
> > +	&dev_attr_watchdog_mode.attr,
> > +	NULL
> > +};
> > +
> > +static int niwatchdog_counter_set(struct niwatchdog *niwatchdog, u32 counter)
> > +{
> > +	int ret;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	/* Prevent changing the counter while the watchdog is running. */
> > +	if (niwatchdog->running) {
> > +		ret = -EBUSY;
> > +		goto out_unlock;
> > +	}
> > +
> This is unusual and problematic, since standard applications
> _will_ change the timeout while the watchdog is running.
> 
> If the counter can not be changed while the watchdog is running,
> you'll have to stop it while updating the timer.
> 
> > +	outb(((0x00FF0000 & counter) >> 16), niwatchdog->io_base + NIWD_SEED2);
> > +	outb(((0x0000FF00 & counter) >> 8), niwatchdog->io_base + NIWD_SEED1);
> > +	outb((0x000000FF & counter), niwatchdog->io_base + NIWD_SEED0);
> > +
> > +	ret = 0;
> > +
> > +out_unlock:
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> > +static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
> > +{
> > +	u8 action_mask;
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	if (action == NIWATCHDOG_ACTION_INTERRUPT)
> > +		action_mask = NIWD_CONTROL_PROC_INTERRUPT;
> 
> This function is never called with NIWATCHDOG_ACTION_INTERRUPT
> as parameter. Which makes me wonder what the interrupt handler
> is all about.
> 
> > +	else if (action == NIWATCHDOG_ACTION_RESET)
> > +		action_mask = NIWD_CONTROL_PROC_RESET;
> > +	else
> > +		return -ENOTSUPP;
> > +
> 
> If you want to use separate actions 'RESET' and 'INTERRUPT',
> you have a number of options.
> 
> - provide a module parameter to determine the desired action.
> - provide a sysfs attribute to determien the desired action
>   (less preferred)
> - use the pretimeout functionality, and have the driver generate
>   an interrupt if a pretimeout is configured (followed by a hard reset
>   if the watchdog times out again).
> 

These options will be moved to patch 2. To your point though, I don't think a
parameter would be the best option--the action can be changed after any
reset/stop of the watchdog, so requiring a module reload seems like overkill.
The pretimeout doesn't work either, since the interrupt can be triggered
instead of as well as in addition to a reset. I would tend toward a sysfs
option, since that would allow chaning the watchdog on the fly.

However, the point of the INTERRUPT option is to poke our software stack on
timeout. Normally I would say that's not a good reason to include something in
a drivers, but I was hoping you'd chime in on this instance. My thinking is
that any user of this watchdog would also be using our software (since the
watchdog is only used on NI controllers and they aren't really usable without
our software), so it would make sense to expose this feature despite it being
non-standard.

> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +	control |= action_mask;
> > +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void niwatchdog_start(struct niwatchdog *niwatchdog)
> > +{
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	niwatchdog->running = true;
> > +	niwatchdog->expired = false;
> > +
> > +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +	outb(control | NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
> > +	outb(control | NIWD_CONTROL_PET, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +}
> > +
> > +static void niwatchdog_pet(struct niwatchdog *niwatchdog, u32 *state)
> > +{
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	if (niwatchdog->expired) {
> > +		*state = NIWATCHDOG_STATE_EXPIRED;
> > +	} else if (!niwatchdog->running) {
> > +		*state = NIWATCHDOG_STATE_DISABLED;
> 
> This function will never be called if the watchdog is not running.
> 
> > +	} else {
> > +		control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +		control |= NIWD_CONTROL_PET;
> > +		outb(control, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +		*state = NIWATCHDOG_STATE_RUNNING;
> > +	}
> > +
> 
> Please consider using just standard watchdog functionality.
> If the watchdog core requests a ping, execute a ping. All the other
> functionality is non-standard and just adds unnecessary complexity
> to the driver.
> 
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +}
> > +
> > +static void niwatchdog_reset(struct niwatchdog *niwatchdog)
> > +{
> 
> May be better named niwatchdog_stop().
> 
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	niwatchdog->running = false;
> > +	niwatchdog->expired = false;
> > +
> As mentioned above, those variables are not needed.
> 
> > +	outb(NIWD_CONTROL_RESET, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +}
> > +
> > +static void niwatchdog_counter_get(struct niwatchdog *niwatchdog, u32 *counter)
> > +{
> 
> There is no reason to return the value in a pointer.
> Plese use the function return value.
> 
> > +	u8 control, counter2, counter1, counter0;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +	control |= NIWD_CONTROL_CAPTURECOUNTER;
> > +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	counter2 = inb(niwatchdog->io_base + NIWD_COUNTER2);
> > +	counter1 = inb(niwatchdog->io_base + NIWD_COUNTER1);
> > +	counter0 = inb(niwatchdog->io_base + NIWD_COUNTER0);
> > +
> > +	*counter = (counter2 << 16) | (counter1 << 8) | counter0;
> > +
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +}
> > +
> > +static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
> > +				      unsigned int timeout)
> > +{
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> > +
> > +	niwatchdog_reset(niwatchdog);
> > +	niwatchdog_counter_set(niwatchdog, counter);
> > +	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> > +
> > +	return niwatchdog_start(niwatchdog);
> > +}
> > +
> > +static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
> > +{
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +	u32 counter;
> > +
> > +	niwatchdog_counter_get(niwatchdog, &counter);
> > +	return (unsigned int)((counter * (unsigned long)NIWD_PERIOD_NS) /
> > +			      1000000000);
> 
> This can overflow if sizeof(unsigned long) == 4.
> Please use u64 for the calculations.
> 
> > +}
> > +
> > +static irqreturn_t niwatchdog_irq(int irq, void *data)
> > +{
> > +	struct niwatchdog *niwatchdog = data;
> > +	irqreturn_t ret = IRQ_NONE;
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	if (!(NIWD_CONTROL_ALARM & control)) {
> > +		dev_err(&niwatchdog->acpi_device->dev,
> > +			"Spurious watchdog interrupt, 0x%02X\n", control);
> > +		goto out_unlock;
> > +	}
> > +
> > +	niwatchdog->running = false;
> > +	niwatchdog->expired = true;
> > +
> > +	/* Acknowledge the interrupt. */
> > +	control |= NIWD_CONTROL_RESET;
> > +	outb(control, niwatchdog->io_base + NIWD_CONTROL);
> > +
> > +	/* Signal the watchdog event. */
> > +	wake_up_all(&niwatchdog->irq_event);
> > +
> Nothing is ever waiting here.
> 
> The functionality of this interrupt handler is unusual.
> Normal expectation for it would be to reset the system.
> It is highly unusual (and unacceptable) to do nothing
> and (try to) return the 'expired' status with the next ping.
> 

This interrupt handler originally was used to catch the watchdog timeout in
cases where reset is not enabled and pass the expiration information on to the
poll() miscdevice function. We used this because we had our software poll the
watchdog to wait for expiration--not really sure how that's going to work now,
but that would be a patch 2 issue.

> > +	ret = IRQ_HANDLED;
> > +
> > +out_unlock:
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +
> > +	return ret;
> > +}
> > +
> > +static int niwatchdog_wdd_open(struct watchdog_device *wdd)
> > +{
> 
> 'niwatchdog_wdd_open' is misleading and unrelated to opening the watchdog
> device. Please change the function name to sonething like niwatchdog_wdd_start.
> 
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +	u32 counter;
> > +
> > +	if (!atomic_dec_and_test(&niwatchdog->available)) {
> > +		atomic_inc(&niwatchdog->available);
> > +		return -EBUSY;
> > +	}
> 
> This function won't be called if the watchdog is already running.
> The variable and the check are therefore not necessary.
> 
> > +
> > +	niwatchdog_reset(niwatchdog);
> > +
> Strictly speaking it is not necessary to stop the watchdog here,
> as it won't be running.
> 

Reset is here to make sure we don't have any unexpected configuration on the
watchdog device itself.

> > +	counter = wdd->timeout * (1000000000 / NIWD_PERIOD_NS);
> > +	niwatchdog_counter_set(niwatchdog, counter);
> > +	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> > +
> Curiously, there is never a del_action() call.
> 

Actions are deleted on reset, but add_action() is going into patch 2 anyway.

> > +	niwatchdog_start(niwatchdog);
> > +
> > +	return request_irq(niwatchdog->irq, niwatchdog_irq, 0,
> > +			   NIWATCHDOG_NAME, niwatchdog);
> 
> It is highly unusual to request an interrupt line when the watchdog is
> started. Please explain why it would make sense to do this here
> instead of in the probe function like other drivers.
> 

Looking at this more closely, I don't think it does.

> > +}
> > +
> > +static int niwatchdog_wdd_release(struct watchdog_device *wdd)
> > +{
> 
> This would be more appropriately named niwatchdog_wdd_stop.
> 
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +
> > +	niwatchdog_reset(niwatchdog);
> > +
> > +	free_irq(niwatchdog->irq, niwatchdog);
> > +	atomic_inc(&niwatchdog->available);
> > +	return 0;
> > +}
> > +
> > +static int niwatchdog_ping(struct watchdog_device *wdd)
> > +{
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +	__u32 state;
> > +
> > +	niwatchdog_pet(niwatchdog, &state);
> > +	if (state != NIWATCHDOG_STATE_RUNNING)
> > +		return -state;
> > +
> 
> The function won't be called if the watchdog is not running,
> so this check is unnecessary. Besides, the error return values are
> non-standard, which is absolutely unacceptable.
> 
> > +	return 0;
> > +}
> > +
> > +static acpi_status niwatchdog_resources(struct acpi_resource *res, void *data)
> > +{
> > +	struct niwatchdog *niwatchdog = data;
> > +
> > +	switch (res->type) {
> > +	case ACPI_RESOURCE_TYPE_IO:
> > +		if ((niwatchdog->io_base != 0) ||
> > +		    (niwatchdog->io_size != 0)) {
> > +			dev_err(&niwatchdog->acpi_device->dev,
> > +			"too many IO resources\n");
> 
> Inconsistent alignment, and unneceaasry ().
> 
> > +			return AE_ERROR;
> > +		}
> > +
> > +		niwatchdog->io_base = res->data.io.minimum;
> > +		niwatchdog->io_size = res->data.io.address_length;
> > +
> > +		return AE_OK;
> > +
> > +	case ACPI_RESOURCE_TYPE_IRQ:
> > +		if (niwatchdog->irq != 0) {
> > +			dev_err(&niwatchdog->acpi_device->dev,
> > +				"too many IRQ resources\n");
> > +			return AE_ERROR;
> > +		}
> > +
> > +		niwatchdog->irq = res->data.irq.interrupts[0];
> > +
> > +		return AE_OK;
> > +
> > +	case ACPI_RESOURCE_TYPE_END_TAG:
> > +		return AE_OK;
> > +
> > +	default:
> > +		dev_err(&niwatchdog->acpi_device->dev,
> > +			"unsupported resource type %d\n",
> > +			res->type);
> > +		return AE_ERROR;
> > +	}
> > +
> > +	return AE_OK;
> > +}
> > +
> > +static int niwatchdog_acpi_remove(struct acpi_device *device)
> > +{
> > +	struct niwatchdog *niwatchdog = device->driver_data;
> > +
> > +	watchdog_unregister_device(&niwatchdog->wdog);
> > +
> > +	sysfs_remove_files(&niwatchdog->acpi_device->dev.kobj,
> > +			   niwatchdog_attrs);
> > +
> > +	if ((niwatchdog->io_base != 0) &&
> > +	    (niwatchdog->io_size == NIWD_IO_SIZE))
> 
> Please no unneceassary ().
> 
> > +		release_region(niwatchdog->io_base, niwatchdog->io_size);
> > +
> > +	device->driver_data = NULL;
> > +
> Unnecessary.
> 
> > +	kfree(niwatchdog);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info niwatchdog_wdd_info = {
> > +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> > +	.identity = "NI Watchdog",
> > +};
> > +
> > +static const struct watchdog_ops niwatchdog_wdd_ops = {
> > +	.owner		= THIS_MODULE,
> > +	.start		= niwatchdog_wdd_open,
> > +	.stop		= niwatchdog_wdd_release,
> > +	.ping		= niwatchdog_ping,
> > +	.set_timeout	= niwatchdog_wdd_set_timeout,
> > +	.get_timeleft	= niwatchdog_wdd_get_timeleft,
> > +};
> > +
> > +static int niwatchdog_acpi_add(struct acpi_device *device)
> > +{
> > +	struct niwatchdog *niwatchdog;
> > +	acpi_status acpi_ret;
> > +	int err;
> > +
> > +	niwatchdog = kzalloc(sizeof(*niwatchdog), GFP_KERNEL);
> > +
> No empty line here please.
> Please use devm_kzalloc() unless you have a good reason not to.
> 
> > +	if (!niwatchdog)
> > +		return -ENOMEM;
> > +
> > +	device->driver_data = niwatchdog;
> > +
> > +	niwatchdog->acpi_device = device;
> > +
> > +	acpi_ret = acpi_walk_resources(device->handle, METHOD_NAME__CRS,
> > +				       niwatchdog_resources, niwatchdog);
> > +
> > +	if (ACPI_FAILURE(acpi_ret) ||
> > +	    (niwatchdog->io_base == 0) ||
> > +	    (niwatchdog->io_size != NIWD_IO_SIZE) ||
> > +	    (niwatchdog->irq == 0)) {
> 
> Unnecessary ( ).
> 
> > +		niwatchdog_acpi_remove(device);
> 
> Please use proper return handling. 
> Releaseing everything blindly even if it was not yet requested
> or registered is not acceptable.
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (!request_region(niwatchdog->io_base, niwatchdog->io_size,
> 
> Please use devm_request_region().
> 
> > +	    NIWATCHDOG_NAME)) {
> > +		niwatchdog_acpi_remove(device);
> > +		return -EBUSY;
> > +	}
> > +
> > +	err = sysfs_create_files(&niwatchdog->acpi_device->dev.kobj,
> > +				 niwatchdog_attrs);
> > +	if (err) {
> > +		niwatchdog_acpi_remove(device);
> > +		return err;
> > +	}
> 
> The watchdog core now supports a means to attach sysfs attributes
> to the watchdog device. Please use this mechanism if needed
> (though I would want to be convinced that the sysfs attribute
> is needed in the first place).
> 
> > +
> > +	spin_lock_init(&niwatchdog->lock);
> > +
> > +	atomic_set(&niwatchdog->available, 1);
> > +	init_waitqueue_head(&niwatchdog->irq_event);
> > +	niwatchdog->expired = false;
> > +
> > +	niwatchdog->wdog.info = &niwatchdog_wdd_info;
> > +	niwatchdog->wdog.ops = &niwatchdog_wdd_ops;
> > +	niwatchdog->wdog.min_timeout = NIWD_MIN_TIMEOUT;
> > +	niwatchdog->wdog.max_timeout = NIWD_MAX_TIMEOUT;
> > +	niwatchdog->wdog.timeout = NIWD_DEFAULT_TIMEOUT;
> > +	niwatchdog->wdog.parent = &device->dev;
> > +	err = watchdog_register_device(&niwatchdog->wdog);
> > +
> 
> Please no empty lines between assignments and associated error checks.
> 
> > +	if (err) {
> > +		niwatchdog_acpi_remove(device);
> > +		return err;
> > +	}
> > +
> > +	dev_info(&niwatchdog->acpi_device->dev,
> > +		 "IO range 0x%04X-0x%04X, IRQ %d\n",
> > +		 niwatchdog->io_base,
> > +		 niwatchdog->io_base + niwatchdog->io_size - 1,
> > +		 niwatchdog->irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct acpi_device_id niwatchdog_device_ids[] = {
> > +	{"NIC775C", 0},
> > +	{"", 0},
> > +};
> > +
> > +static struct acpi_driver niwatchdog_acpi_driver = {
> > +	.name = NIWATCHDOG_NAME,
> > +	.ids = niwatchdog_device_ids,
> > +	.ops = {
> > +		.add = niwatchdog_acpi_add,
> > +		.remove = niwatchdog_acpi_remove,
> > +		},
> > +};
> > +
> > +static int __init niwatchdog_init(void)
> > +{
> > +	return acpi_bus_register_driver(&niwatchdog_acpi_driver);
> > +}
> > +
> > +static void __exit niwatchdog_exit(void)
> > +{
> > +	acpi_bus_unregister_driver(&niwatchdog_acpi_driver);
> > +}
> > +
> > +module_init(niwatchdog_init);
> > +module_exit(niwatchdog_exit);
> > +
> > +MODULE_DEVICE_TABLE(acpi, niwatchdog_device_ids);
> > +MODULE_DESCRIPTION("NI Watchdog");
> > +MODULE_AUTHOR("Jeff Westfahl <jeff.westfahl@ni.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index 628e6e6..8c5dde3 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -303,6 +303,7 @@ header-y += nfs_fs.h
> >  header-y += nfs.h
> >  header-y += nfs_idmap.h
> >  header-y += nfs_mount.h
> > +header-y += niwatchdog.h
> >  header-y += nl80211.h
> >  header-y += n_r3964.h
> >  header-y += nubus.h
> > diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
> > new file mode 100644
> > index 0000000..9d3613d
> > --- /dev/null
> > +++ b/include/uapi/linux/niwatchdog.h
> 
> This file is unnecessary.
> 
> First, it is only used with patch 2, and if anything should be introduced there.
> Second, I do not agree that patch 2 should exist in the first place.
> I'll comment on that in my comments to patch 2.
> 
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Copyright (C) 2012 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.
> > + */
> > +
> > +#ifndef _LINUX_NIWATCHDOG_H_
> > +#define _LINUX_NIWATCHDOG_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +#define NIWATCHDOG_ACTION_INTERRUPT	0
> > +#define NIWATCHDOG_ACTION_RESET		1
> > +
> > +#define NIWATCHDOG_STATE_RUNNING	0
> > +#define NIWATCHDOG_STATE_EXPIRED	1
> > +#define NIWATCHDOG_STATE_DISABLED	2
> > +
> > +#define NIWATCHDOG_NAME			"niwatchdog"
> > +
> > +#endif /* _LINUX_NIWATCHDOG_H_ */

I'm also going to remove the manual locking since (as far as I can tell) the
watchdog core mutex locks make redundant. That is, except in the case of the
IRQ handler.

-- 
Kyle Roeschley
Software Engineer
National Instruments

Re: [2/2] niwatchdog: add support for custom ioctls

[Kyle Roeschley]

Hi Guenter,

On Sat, Jan 16, 2016 at 08:29:41PM -0800, Guenter Roeck wrote:
> On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
> > Add custom ioctls for features specific to the NI watchdog, including
> > disabling the watchdog, changing timeout behavior, and setting timeouts
> > with sub-second resolution.
> > 
> I don't really agree with any of the added functionality.
> Most of the added ioctls duplicate standard functionality.
> 
> 

The main thing that I think could be useful is the increased timeout
resolution. Aside from that, the other changes are specifically implemented for
the sake of maintaining compatability with our watchdog API--I think carrying
some (hopefully not all) of those changes out-of-tree is reasonable.

> > Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> > Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> > ---
> >  drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
> >  include/uapi/linux/niwatchdog.h |  10 ++++
> >  2 files changed, 123 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
> > index 3908846..fe637a3 100644
> > --- a/drivers/watchdog/niwatchdog.c
> > +++ b/drivers/watchdog/niwatchdog.c
> > @@ -131,6 +131,35 @@ out_unlock:
> >  	return ret;
> >  }
> >  
> > +static int niwatchdog_check_action(u32 action)
> > +{
> > +	int err = 0;
> > +
> > +	switch (action) {
> > +	case NIWD_CONTROL_PROC_INTERRUPT:
> > +	case NIWD_CONTROL_PROC_RESET:
> > +		break;
> > +	default:
> > +		err = -ENOTSUPP;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
> > +{
> > +	u8 control;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&niwatchdog->lock, flags);
> > +
> > +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> > +	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
> > +			      NIWD_CONTROL_PROC_RESET);
> > +
> > +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> > +}
> > +
> >  static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
> >  {
> >  	u8 action_mask;
> > @@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
> >  {
> >  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> >  	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> > +	u8 actions;
> >  
> > +	niwatchdog_get_actions(niwatchdog, &actions);
> >  	niwatchdog_reset(niwatchdog);
> >  	niwatchdog_counter_set(niwatchdog, counter);
> > -	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> >  
> > -	return niwatchdog_start(niwatchdog);
> > +	if (actions & NIWD_CONTROL_PROC_RESET)
> > +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> > +	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
> > +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
> > +
> > +	niwatchdog_start(niwatchdog);
> > +
> > +	return 0;
> >  }
> >  
> >  static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
> > @@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
> >  	return 0;
> >  }
> >  
> > +static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
> > +				 unsigned long arg)
> > +{
> > +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > +	int err = 0;
> > +
> > +	switch (cmd) {
> > +	case NIWATCHDOG_IOCTL_PERIOD_NS: {
> > +		__u32 period = NIWD_PERIOD_NS;
> > +
> > +		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
> > +		break;
> > +	}
> > +	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
> > +		__u32 counter = NIWD_MAX_COUNTER;
> > +
> > +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> > +		break;
> > +	}
> 
> Those are constants and can as well be defined in some documentation.
> 

True. The only reason they're here is that our API expects this function to be
implemented for all of our watchdog timers. As such, carrying this out-of-tree
would be reasonable.

> > +	case NIWATCHDOG_IOCTL_COUNTER_SET: {
> > +		__u32 counter;
> > +
> > +		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
> > +		if (!err)
> > +			err = niwatchdog_counter_set(niwatchdog, counter);
> > +		break;
> > +	}
> Duplicates standard functionality
> 

It duplicates set_timeout(), but with much greater resolution. What would be an
appropriate way to expose this capability to users?

> > +	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
> > +		__u32 action;
> > +
> > +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> > +		if (!err)
> > +			err = niwatchdog_check_action(action);
> > +		break;
> > +	}
> > +	case NIWATCHDOG_IOCTL_ADD_ACTION: {
> > +		__u32 action;
> > +
> > +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> > +		if (!err)
> > +			err = niwatchdog_add_action(niwatchdog, action);
> > +		break;
> > +	}
> 
> Use a module parameter or sysfs attribute for those.
> 

I'm a fan of using a sysfs attribute, specifically because the options can be
changed after opening the watchdog and you can have one, both, or neither of
the two timeout actions set.

> > +	case NIWATCHDOG_IOCTL_START: {
> > +		niwatchdog_start(niwatchdog);
> > +		break;
> > +	}
> 
> Duplicates standard functionality.
> 
> > +	case NIWATCHDOG_IOCTL_PET: {
> > +		__u32 state;
> > +
> > +		niwatchdog_pet(niwatchdog, &state);
> > +		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
> > +		break;
> > +	}
> 
> Duplicates standard functionality.
> 
> > +	case NIWATCHDOG_IOCTL_RESET: {
> > +		niwatchdog_reset(niwatchdog);
> > +		break;
> > +	}
> 
> Duplicates standard functionality.
> 

These are just maps from old, non-standard iotcls to the standard ones. They
can be removed in favor of an out-of-tree commit.

> > +	case NIWATCHDOG_IOCTL_COUNTER_GET: {
> > +		__u32 counter;
> > +
> > +		niwatchdog_counter_get(niwatchdog, &counter);
> > +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> > +		break;
> > +	}
> 
> Duplicates standard functionality.
> 

Yes, but again I'm not sure how we should give the user a more accurate
remaining time.

> > +	default:
> > +		err = -ENOIOCTLCMD;
> > +		break;
> > +	};
> > +
> > +	return err;
> > +}
> > +
> >  static int niwatchdog_ping(struct watchdog_device *wdd)
> >  {
> >  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> > @@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
> >  	.start		= niwatchdog_wdd_open,
> >  	.stop		= niwatchdog_wdd_release,
> >  	.ping		= niwatchdog_ping,
> > +	.ioctl		= niwatchdog_wdd_ioctl,
> >  	.set_timeout	= niwatchdog_wdd_set_timeout,
> >  	.get_timeleft	= niwatchdog_wdd_get_timeleft,
> >  };
> > diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
> > index 9d3613d..4fb8d47 100644
> > --- a/include/uapi/linux/niwatchdog.h
> > +++ b/include/uapi/linux/niwatchdog.h
> > @@ -25,6 +25,16 @@
> >  #define NIWATCHDOG_STATE_EXPIRED	1
> >  #define NIWATCHDOG_STATE_DISABLED	2
> >  
> > +#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
> > +#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
> > +#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
> > +#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
> > +#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
> > +#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
> > +#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
> > +#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
> > +#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
> > +
> >  #define NIWATCHDOG_NAME			"niwatchdog"
> >  
> >  #endif /* _LINUX_NIWATCHDOG_H_ */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks again for the reviews.

-- 
Kyle Roeschley
Software Engineer
National Instruments

Re: [2/2] niwatchdog: add support for custom ioctls

[Guenter Roeck]

On 01/25/2016 03:31 PM, Kyle Roeschley wrote:
> Hi Guenter,
>
> On Sat, Jan 16, 2016 at 08:29:41PM -0800, Guenter Roeck wrote:
>> On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
>>> Add custom ioctls for features specific to the NI watchdog, including
>>> disabling the watchdog, changing timeout behavior, and setting timeouts
>>> with sub-second resolution.
>>>
>> I don't really agree with any of the added functionality.
>> Most of the added ioctls duplicate standard functionality.
>>
>>
>
> The main thing that I think could be useful is the increased timeout
> resolution. Aside from that, the other changes are specifically implemented for
> the sake of maintaining compatability with our watchdog API--I think carrying
> some (hopefully not all) of those changes out-of-tree is reasonable.
>
We have been discussing to increase the timeout resolution to milli-seconds for a while.
A patch supporting a hardware timeout maximum in milli-seconds is in the works. I could
imagine adding milli-second resolution to the watchdog core. Nanoseconds is a bit of
an overkill, though.

Guenter

>>> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
>>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>>> ---
>>>   drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
>>>   include/uapi/linux/niwatchdog.h |  10 ++++
>>>   2 files changed, 123 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
>>> index 3908846..fe637a3 100644
>>> --- a/drivers/watchdog/niwatchdog.c
>>> +++ b/drivers/watchdog/niwatchdog.c
>>> @@ -131,6 +131,35 @@ out_unlock:
>>>   	return ret;
>>>   }
>>>
>>> +static int niwatchdog_check_action(u32 action)
>>> +{
>>> +	int err = 0;
>>> +
>>> +	switch (action) {
>>> +	case NIWD_CONTROL_PROC_INTERRUPT:
>>> +	case NIWD_CONTROL_PROC_RESET:
>>> +		break;
>>> +	default:
>>> +		err = -ENOTSUPP;
>>> +	}
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
>>> +{
>>> +	u8 control;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&niwatchdog->lock, flags);
>>> +
>>> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
>>> +	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
>>> +			      NIWD_CONTROL_PROC_RESET);
>>> +
>>> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
>>> +}
>>> +
>>>   static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
>>>   {
>>>   	u8 action_mask;
>>> @@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
>>>   {
>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>>   	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
>>> +	u8 actions;
>>>
>>> +	niwatchdog_get_actions(niwatchdog, &actions);
>>>   	niwatchdog_reset(niwatchdog);
>>>   	niwatchdog_counter_set(niwatchdog, counter);
>>> -	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>>
>>> -	return niwatchdog_start(niwatchdog);
>>> +	if (actions & NIWD_CONTROL_PROC_RESET)
>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>> +	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
>>> +
>>> +	niwatchdog_start(niwatchdog);
>>> +
>>> +	return 0;
>>>   }
>>>
>>>   static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
>>> @@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
>>>   	return 0;
>>>   }
>>>
>>> +static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
>>> +				 unsigned long arg)
>>> +{
>>> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>> +	int err = 0;
>>> +
>>> +	switch (cmd) {
>>> +	case NIWATCHDOG_IOCTL_PERIOD_NS: {
>>> +		__u32 period = NIWD_PERIOD_NS;
>>> +
>>> +		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
>>> +		break;
>>> +	}
>>> +	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
>>> +		__u32 counter = NIWD_MAX_COUNTER;
>>> +
>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Those are constants and can as well be defined in some documentation.
>>
>
> True. The only reason they're here is that our API expects this function to be
> implemented for all of our watchdog timers. As such, carrying this out-of-tree
> would be reasonable.
>
>>> +	case NIWATCHDOG_IOCTL_COUNTER_SET: {
>>> +		__u32 counter;
>>> +
>>> +		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_counter_set(niwatchdog, counter);
>>> +		break;
>>> +	}
>> Duplicates standard functionality
>>
>
> It duplicates set_timeout(), but with much greater resolution. What would be an
> appropriate way to expose this capability to users?
>
>>> +	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
>>> +		__u32 action;
>>> +
>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_check_action(action);
>>> +		break;
>>> +	}
>>> +	case NIWATCHDOG_IOCTL_ADD_ACTION: {
>>> +		__u32 action;
>>> +
>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>> +		if (!err)
>>> +			err = niwatchdog_add_action(niwatchdog, action);
>>> +		break;
>>> +	}
>>
>> Use a module parameter or sysfs attribute for those.
>>
>
> I'm a fan of using a sysfs attribute, specifically because the options can be
> changed after opening the watchdog and you can have one, both, or neither of
> the two timeout actions set.
>
>>> +	case NIWATCHDOG_IOCTL_START: {
>>> +		niwatchdog_start(niwatchdog);
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>>> +	case NIWATCHDOG_IOCTL_PET: {
>>> +		__u32 state;
>>> +
>>> +		niwatchdog_pet(niwatchdog, &state);
>>> +		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>>> +	case NIWATCHDOG_IOCTL_RESET: {
>>> +		niwatchdog_reset(niwatchdog);
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>
> These are just maps from old, non-standard iotcls to the standard ones. They
> can be removed in favor of an out-of-tree commit.
>
>>> +	case NIWATCHDOG_IOCTL_COUNTER_GET: {
>>> +		__u32 counter;
>>> +
>>> +		niwatchdog_counter_get(niwatchdog, &counter);
>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>> +		break;
>>> +	}
>>
>> Duplicates standard functionality.
>>
>
> Yes, but again I'm not sure how we should give the user a more accurate
> remaining time.
>
>>> +	default:
>>> +		err = -ENOIOCTLCMD;
>>> +		break;
>>> +	};
>>> +
>>> +	return err;
>>> +}
>>> +
>>>   static int niwatchdog_ping(struct watchdog_device *wdd)
>>>   {
>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>> @@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
>>>   	.start		= niwatchdog_wdd_open,
>>>   	.stop		= niwatchdog_wdd_release,
>>>   	.ping		= niwatchdog_ping,
>>> +	.ioctl		= niwatchdog_wdd_ioctl,
>>>   	.set_timeout	= niwatchdog_wdd_set_timeout,
>>>   	.get_timeleft	= niwatchdog_wdd_get_timeleft,
>>>   };
>>> diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
>>> index 9d3613d..4fb8d47 100644
>>> --- a/include/uapi/linux/niwatchdog.h
>>> +++ b/include/uapi/linux/niwatchdog.h
>>> @@ -25,6 +25,16 @@
>>>   #define NIWATCHDOG_STATE_EXPIRED	1
>>>   #define NIWATCHDOG_STATE_DISABLED	2
>>>
>>> +#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
>>> +#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
>>> +#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
>>> +#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
>>> +#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
>>> +#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
>>> +#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
>>> +#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
>>> +#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
>>> +
>>>   #define NIWATCHDOG_NAME			"niwatchdog"
>>>
>>>   #endif /* _LINUX_NIWATCHDOG_H_ */
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Thanks again for the reviews.
>

Re: [2/2] niwatchdog: add support for custom ioctls

[Kyle Roeschley]

On Mon, Jan 25, 2016 at 05:00:23PM -0800, Guenter Roeck wrote:
> On 01/25/2016 03:31 PM, Kyle Roeschley wrote:
> >Hi Guenter,
> >
> >On Sat, Jan 16, 2016 at 08:29:41PM -0800, Guenter Roeck wrote:
> >>On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
> >>>Add custom ioctls for features specific to the NI watchdog, including
> >>>disabling the watchdog, changing timeout behavior, and setting timeouts
> >>>with sub-second resolution.
> >>>
> >>I don't really agree with any of the added functionality.
> >>Most of the added ioctls duplicate standard functionality.
> >>
> >>
> >
> >The main thing that I think could be useful is the increased timeout
> >resolution. Aside from that, the other changes are specifically implemented for
> >the sake of maintaining compatability with our watchdog API--I think carrying
> >some (hopefully not all) of those changes out-of-tree is reasonable.
> >
> We have been discussing to increase the timeout resolution to milli-seconds for a while.
> A patch supporting a hardware timeout maximum in milli-seconds is in the works. I could
> imagine adding milli-second resolution to the watchdog core. Nanoseconds is a bit of
> an overkill, though.
> 
> Guenter
> 

Deciding on milliseconds over nanoseconds seems rather arbitrary, especially
since standard kernel timers use jiffies and hrtimers use nanoseconds. I do
realize that users may not care about differences of less than a second, but it
seems better to err on the side of caution and provide more accuracy than
they would be expected to need. For instance, this watchdog operates on a
real-time system with any number of industrial applications which could require
high accuracy even from the watchdog.

Seeing as the higher-resolution timeouts aren't yet in the kernel, would it be
acceptable to use a sysfs attribute for the time being at least? I can't come
up with any ways in which exposing the counter could cause problems. The code
to do so is trivial and involves only moving a bit of work out of set_timeout/
get_timeleft into separate functions.

That being said, I'd gone with a sysfs attribute for adding actions (as you
suggested below), but I'm running into a problem. The watchdog core lock isn't
used when you use sysfs attributes, and without that lock, we have the
possibility of writing or reading garbage data. The best workarounds would seem
to be adding some way to access the internal lock or adding my own lock. Using
my own lock, however, would result in double locking everywhere except in the
attribute show and store functions. Do you have any advice on this?

Regards,

Kyle Roeschley
Software Engineer
National Instruments

> >>>Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> >>>Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> >>>---
> >>>  drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
> >>>  include/uapi/linux/niwatchdog.h |  10 ++++
> >>>  2 files changed, 123 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
> >>>index 3908846..fe637a3 100644
> >>>--- a/drivers/watchdog/niwatchdog.c
> >>>+++ b/drivers/watchdog/niwatchdog.c
> >>>@@ -131,6 +131,35 @@ out_unlock:
> >>>  	return ret;
> >>>  }
> >>>
> >>>+static int niwatchdog_check_action(u32 action)
> >>>+{
> >>>+	int err = 0;
> >>>+
> >>>+	switch (action) {
> >>>+	case NIWD_CONTROL_PROC_INTERRUPT:
> >>>+	case NIWD_CONTROL_PROC_RESET:
> >>>+		break;
> >>>+	default:
> >>>+		err = -ENOTSUPP;
> >>>+	}
> >>>+
> >>>+	return err;
> >>>+}
> >>>+
> >>>+static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
> >>>+{
> >>>+	u8 control;
> >>>+	unsigned long flags;
> >>>+
> >>>+	spin_lock_irqsave(&niwatchdog->lock, flags);
> >>>+
> >>>+	control = inb(niwatchdog->io_base + NIWD_CONTROL);
> >>>+	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
> >>>+			      NIWD_CONTROL_PROC_RESET);
> >>>+
> >>>+	spin_unlock_irqrestore(&niwatchdog->lock, flags);
> >>>+}
> >>>+
> >>>  static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
> >>>  {
> >>>  	u8 action_mask;
> >>>@@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
> >>>  {
> >>>  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> >>>  	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
> >>>+	u8 actions;
> >>>
> >>>+	niwatchdog_get_actions(niwatchdog, &actions);
> >>>  	niwatchdog_reset(niwatchdog);
> >>>  	niwatchdog_counter_set(niwatchdog, counter);
> >>>-	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> >>>
> >>>-	return niwatchdog_start(niwatchdog);
> >>>+	if (actions & NIWD_CONTROL_PROC_RESET)
> >>>+		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
> >>>+	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
> >>>+		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
> >>>+
> >>>+	niwatchdog_start(niwatchdog);
> >>>+
> >>>+	return 0;
> >>>  }
> >>>
> >>>  static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
> >>>@@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
> >>>  	return 0;
> >>>  }
> >>>
> >>>+static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
> >>>+				 unsigned long arg)
> >>>+{
> >>>+	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> >>>+	int err = 0;
> >>>+
> >>>+	switch (cmd) {
> >>>+	case NIWATCHDOG_IOCTL_PERIOD_NS: {
> >>>+		__u32 period = NIWD_PERIOD_NS;
> >>>+
> >>>+		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
> >>>+		break;
> >>>+	}
> >>>+	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
> >>>+		__u32 counter = NIWD_MAX_COUNTER;
> >>>+
> >>>+		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> >>>+		break;
> >>>+	}
> >>
> >>Those are constants and can as well be defined in some documentation.
> >>
> >
> >True. The only reason they're here is that our API expects this function to be
> >implemented for all of our watchdog timers. As such, carrying this out-of-tree
> >would be reasonable.
> >
> >>>+	case NIWATCHDOG_IOCTL_COUNTER_SET: {
> >>>+		__u32 counter;
> >>>+
> >>>+		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
> >>>+		if (!err)
> >>>+			err = niwatchdog_counter_set(niwatchdog, counter);
> >>>+		break;
> >>>+	}
> >>Duplicates standard functionality
> >>
> >
> >It duplicates set_timeout(), but with much greater resolution. What would be an
> >appropriate way to expose this capability to users?
> >
> >>>+	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
> >>>+		__u32 action;
> >>>+
> >>>+		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> >>>+		if (!err)
> >>>+			err = niwatchdog_check_action(action);
> >>>+		break;
> >>>+	}
> >>>+	case NIWATCHDOG_IOCTL_ADD_ACTION: {
> >>>+		__u32 action;
> >>>+
> >>>+		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
> >>>+		if (!err)
> >>>+			err = niwatchdog_add_action(niwatchdog, action);
> >>>+		break;
> >>>+	}
> >>
> >>Use a module parameter or sysfs attribute for those.
> >>
> >
> >I'm a fan of using a sysfs attribute, specifically because the options can be
> >changed after opening the watchdog and you can have one, both, or neither of
> >the two timeout actions set.
> >
> >>>+	case NIWATCHDOG_IOCTL_START: {
> >>>+		niwatchdog_start(niwatchdog);
> >>>+		break;
> >>>+	}
> >>
> >>Duplicates standard functionality.
> >>
> >>>+	case NIWATCHDOG_IOCTL_PET: {
> >>>+		__u32 state;
> >>>+
> >>>+		niwatchdog_pet(niwatchdog, &state);
> >>>+		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
> >>>+		break;
> >>>+	}
> >>
> >>Duplicates standard functionality.
> >>
> >>>+	case NIWATCHDOG_IOCTL_RESET: {
> >>>+		niwatchdog_reset(niwatchdog);
> >>>+		break;
> >>>+	}
> >>
> >>Duplicates standard functionality.
> >>
> >
> >These are just maps from old, non-standard iotcls to the standard ones. They
> >can be removed in favor of an out-of-tree commit.
> >
> >>>+	case NIWATCHDOG_IOCTL_COUNTER_GET: {
> >>>+		__u32 counter;
> >>>+
> >>>+		niwatchdog_counter_get(niwatchdog, &counter);
> >>>+		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
> >>>+		break;
> >>>+	}
> >>
> >>Duplicates standard functionality.
> >>
> >
> >Yes, but again I'm not sure how we should give the user a more accurate
> >remaining time.
> >
> >>>+	default:
> >>>+		err = -ENOIOCTLCMD;
> >>>+		break;
> >>>+	};
> >>>+
> >>>+	return err;
> >>>+}
> >>>+
> >>>  static int niwatchdog_ping(struct watchdog_device *wdd)
> >>>  {
> >>>  	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
> >>>@@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
> >>>  	.start		= niwatchdog_wdd_open,
> >>>  	.stop		= niwatchdog_wdd_release,
> >>>  	.ping		= niwatchdog_ping,
> >>>+	.ioctl		= niwatchdog_wdd_ioctl,
> >>>  	.set_timeout	= niwatchdog_wdd_set_timeout,
> >>>  	.get_timeleft	= niwatchdog_wdd_get_timeleft,
> >>>  };
> >>>diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
> >>>index 9d3613d..4fb8d47 100644
> >>>--- a/include/uapi/linux/niwatchdog.h
> >>>+++ b/include/uapi/linux/niwatchdog.h
> >>>@@ -25,6 +25,16 @@
> >>>  #define NIWATCHDOG_STATE_EXPIRED	1
> >>>  #define NIWATCHDOG_STATE_DISABLED	2
> >>>
> >>>+#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
> >>>+#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
> >>>+#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
> >>>+#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
> >>>+#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
> >>>+#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
> >>>+#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
> >>>+#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
> >>>+#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
> >>>+
> >>>  #define NIWATCHDOG_NAME			"niwatchdog"
> >>>
> >>>  #endif /* _LINUX_NIWATCHDOG_H_ */
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> >Thanks again for the reviews.
> >
> 

Re: [2/2] niwatchdog: add support for custom ioctls

[Guenter Roeck]

On 02/02/2016 04:44 PM, Kyle Roeschley wrote:
> On Mon, Jan 25, 2016 at 05:00:23PM -0800, Guenter Roeck wrote:
>> On 01/25/2016 03:31 PM, Kyle Roeschley wrote:
>>> Hi Guenter,
>>>
>>> On Sat, Jan 16, 2016 at 08:29:41PM -0800, Guenter Roeck wrote:
>>>> On Mon, Jan 11, 2016 at 06:23:01PM -0600, Kyle Roeschley wrote:
>>>>> Add custom ioctls for features specific to the NI watchdog, including
>>>>> disabling the watchdog, changing timeout behavior, and setting timeouts
>>>>> with sub-second resolution.
>>>>>
>>>> I don't really agree with any of the added functionality.
>>>> Most of the added ioctls duplicate standard functionality.
>>>>
>>>>
>>>
>>> The main thing that I think could be useful is the increased timeout
>>> resolution. Aside from that, the other changes are specifically implemented for
>>> the sake of maintaining compatability with our watchdog API--I think carrying
>>> some (hopefully not all) of those changes out-of-tree is reasonable.
>>>
>> We have been discussing to increase the timeout resolution to milli-seconds for a while.
>> A patch supporting a hardware timeout maximum in milli-seconds is in the works. I could
>> imagine adding milli-second resolution to the watchdog core. Nanoseconds is a bit of
>> an overkill, though.
>>
>> Guenter
>>
>
> Deciding on milliseconds over nanoseconds seems rather arbitrary, especially
> since standard kernel timers use jiffies and hrtimers use nanoseconds. I do

Odd to bring up jiffies here. That would really be inappropriate for a user
space ABI.

> realize that users may not care about differences of less than a second, but it
> seems better to err on the side of caution and provide more accuracy than
> they would be expected to need. For instance, this watchdog operates on a
> real-time system with any number of industrial applications which could require
> high accuracy even from the watchdog.
>
I don't really believe that this is or will ever be the case. I would argue that
any system which requires such a high accuracy for a watchdog timeout has a severe
architectural problem. After all, we are not talking about reaction time to an
external or internal event here. We are talking about what should happen if
something goes wrong so badly that it results in an immediate system reboot
or hardware reset.

Sure, an industrial system may need high accuracy timers, and a low reaction time.
But a high accuracy watchdog timeout ? That would need some serious convincing
and real use case. Even if such a use case existed, using nanoseconds or even
microseconds would require internal calculations to use 64 bit arithmetic,
which can get expensive on 32 bit systems. So that use case would really have
to be a very convincing use case.

We introduced a millisecond accuracy for maximum timeouts, for the simple reason
that there are odd watchdogs out there which do have such low maximum timeouts.
For the timeout itself, there was no immediate use case to increase the accuracy
to milli-seconds. As mentioned earlier, I would be willing to accept a patch
or set of patches introducing it. I might even implement it myself if I find
the time, though it isn't really high priority for me. But microseconds or even
nanoseconds ? I neither think that there is a real use case, nor do I believe
that it even makes sense for a "last resort" reaction.

Thanks,
Guenter

> Seeing as the higher-resolution timeouts aren't yet in the kernel, would it be
> acceptable to use a sysfs attribute for the time being at least? I can't come
> up with any ways in which exposing the counter could cause problems. The code
> to do so is trivial and involves only moving a bit of work out of set_timeout/
> get_timeleft into separate functions.
>
> That being said, I'd gone with a sysfs attribute for adding actions (as you
> suggested below), but I'm running into a problem. The watchdog core lock isn't
> used when you use sysfs attributes, and without that lock, we have the
> possibility of writing or reading garbage data. The best workarounds would seem
> to be adding some way to access the internal lock or adding my own lock. Using
> my own lock, however, would result in double locking everywhere except in the
> attribute show and store functions. Do you have any advice on this?
>
> Regards,
>
> Kyle Roeschley
> Software Engineer
> National Instruments
>
>>>>> Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
>>>>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>>>>> ---
>>>>>   drivers/watchdog/niwatchdog.c   | 115 +++++++++++++++++++++++++++++++++++++++-
>>>>>   include/uapi/linux/niwatchdog.h |  10 ++++
>>>>>   2 files changed, 123 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/niwatchdog.c b/drivers/watchdog/niwatchdog.c
>>>>> index 3908846..fe637a3 100644
>>>>> --- a/drivers/watchdog/niwatchdog.c
>>>>> +++ b/drivers/watchdog/niwatchdog.c
>>>>> @@ -131,6 +131,35 @@ out_unlock:
>>>>>   	return ret;
>>>>>   }
>>>>>
>>>>> +static int niwatchdog_check_action(u32 action)
>>>>> +{
>>>>> +	int err = 0;
>>>>> +
>>>>> +	switch (action) {
>>>>> +	case NIWD_CONTROL_PROC_INTERRUPT:
>>>>> +	case NIWD_CONTROL_PROC_RESET:
>>>>> +		break;
>>>>> +	default:
>>>>> +		err = -ENOTSUPP;
>>>>> +	}
>>>>> +
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>> +static void niwatchdog_get_actions(struct niwatchdog *niwatchdog, u8 *actions)
>>>>> +{
>>>>> +	u8 control;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	spin_lock_irqsave(&niwatchdog->lock, flags);
>>>>> +
>>>>> +	control = inb(niwatchdog->io_base + NIWD_CONTROL);
>>>>> +	*actions = control & (NIWD_CONTROL_PROC_INTERRUPT +
>>>>> +			      NIWD_CONTROL_PROC_RESET);
>>>>> +
>>>>> +	spin_unlock_irqrestore(&niwatchdog->lock, flags);
>>>>> +}
>>>>> +
>>>>>   static int niwatchdog_add_action(struct niwatchdog *niwatchdog, u32 action)
>>>>>   {
>>>>>   	u8 action_mask;
>>>>> @@ -233,12 +262,20 @@ static int niwatchdog_wdd_set_timeout(struct watchdog_device *wdd,
>>>>>   {
>>>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>>>>   	u32 counter = timeout * (1000000000 / NIWD_PERIOD_NS);
>>>>> +	u8 actions;
>>>>>
>>>>> +	niwatchdog_get_actions(niwatchdog, &actions);
>>>>>   	niwatchdog_reset(niwatchdog);
>>>>>   	niwatchdog_counter_set(niwatchdog, counter);
>>>>> -	niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>>>>
>>>>> -	return niwatchdog_start(niwatchdog);
>>>>> +	if (actions & NIWD_CONTROL_PROC_RESET)
>>>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_RESET);
>>>>> +	if (actions & NIWD_CONTROL_PROC_INTERRUPT)
>>>>> +		niwatchdog_add_action(niwatchdog, NIWATCHDOG_ACTION_INTERRUPT);
>>>>> +
>>>>> +	niwatchdog_start(niwatchdog);
>>>>> +
>>>>> +	return 0;
>>>>>   }
>>>>>
>>>>>   static unsigned int niwatchdog_wdd_get_timeleft(struct watchdog_device *wdd)
>>>>> @@ -319,6 +356,79 @@ static int niwatchdog_wdd_release(struct watchdog_device *wdd)
>>>>>   	return 0;
>>>>>   }
>>>>>
>>>>> +static long niwatchdog_wdd_ioctl(struct watchdog_device *wdd, unsigned int cmd,
>>>>> +				 unsigned long arg)
>>>>> +{
>>>>> +	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>>>> +	int err = 0;
>>>>> +
>>>>> +	switch (cmd) {
>>>>> +	case NIWATCHDOG_IOCTL_PERIOD_NS: {
>>>>> +		__u32 period = NIWD_PERIOD_NS;
>>>>> +
>>>>> +		err = copy_to_user((__u32 *)arg, &period, sizeof(__u32));
>>>>> +		break;
>>>>> +	}
>>>>> +	case NIWATCHDOG_IOCTL_MAX_COUNTER: {
>>>>> +		__u32 counter = NIWD_MAX_COUNTER;
>>>>> +
>>>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Those are constants and can as well be defined in some documentation.
>>>>
>>>
>>> True. The only reason they're here is that our API expects this function to be
>>> implemented for all of our watchdog timers. As such, carrying this out-of-tree
>>> would be reasonable.
>>>
>>>>> +	case NIWATCHDOG_IOCTL_COUNTER_SET: {
>>>>> +		__u32 counter;
>>>>> +
>>>>> +		err = copy_from_user(&counter, (__u32 *)arg, sizeof(__u32));
>>>>> +		if (!err)
>>>>> +			err = niwatchdog_counter_set(niwatchdog, counter);
>>>>> +		break;
>>>>> +	}
>>>> Duplicates standard functionality
>>>>
>>>
>>> It duplicates set_timeout(), but with much greater resolution. What would be an
>>> appropriate way to expose this capability to users?
>>>
>>>>> +	case NIWATCHDOG_IOCTL_CHECK_ACTION: {
>>>>> +		__u32 action;
>>>>> +
>>>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>>>> +		if (!err)
>>>>> +			err = niwatchdog_check_action(action);
>>>>> +		break;
>>>>> +	}
>>>>> +	case NIWATCHDOG_IOCTL_ADD_ACTION: {
>>>>> +		__u32 action;
>>>>> +
>>>>> +		err = copy_from_user(&action, (__u32 *)arg, sizeof(__u32));
>>>>> +		if (!err)
>>>>> +			err = niwatchdog_add_action(niwatchdog, action);
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Use a module parameter or sysfs attribute for those.
>>>>
>>>
>>> I'm a fan of using a sysfs attribute, specifically because the options can be
>>> changed after opening the watchdog and you can have one, both, or neither of
>>> the two timeout actions set.
>>>
>>>>> +	case NIWATCHDOG_IOCTL_START: {
>>>>> +		niwatchdog_start(niwatchdog);
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Duplicates standard functionality.
>>>>
>>>>> +	case NIWATCHDOG_IOCTL_PET: {
>>>>> +		__u32 state;
>>>>> +
>>>>> +		niwatchdog_pet(niwatchdog, &state);
>>>>> +		err = copy_to_user((__u32 *)arg, &state, sizeof(__u32));
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Duplicates standard functionality.
>>>>
>>>>> +	case NIWATCHDOG_IOCTL_RESET: {
>>>>> +		niwatchdog_reset(niwatchdog);
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Duplicates standard functionality.
>>>>
>>>
>>> These are just maps from old, non-standard iotcls to the standard ones. They
>>> can be removed in favor of an out-of-tree commit.
>>>
>>>>> +	case NIWATCHDOG_IOCTL_COUNTER_GET: {
>>>>> +		__u32 counter;
>>>>> +
>>>>> +		niwatchdog_counter_get(niwatchdog, &counter);
>>>>> +		err = copy_to_user((__u32 *)arg, &counter, sizeof(__u32));
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Duplicates standard functionality.
>>>>
>>>
>>> Yes, but again I'm not sure how we should give the user a more accurate
>>> remaining time.
>>>
>>>>> +	default:
>>>>> +		err = -ENOIOCTLCMD;
>>>>> +		break;
>>>>> +	};
>>>>> +
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>>   static int niwatchdog_ping(struct watchdog_device *wdd)
>>>>>   {
>>>>>   	struct niwatchdog *niwatchdog = to_niwatchdog(wdd);
>>>>> @@ -403,6 +513,7 @@ static const struct watchdog_ops niwatchdog_wdd_ops = {
>>>>>   	.start		= niwatchdog_wdd_open,
>>>>>   	.stop		= niwatchdog_wdd_release,
>>>>>   	.ping		= niwatchdog_ping,
>>>>> +	.ioctl		= niwatchdog_wdd_ioctl,
>>>>>   	.set_timeout	= niwatchdog_wdd_set_timeout,
>>>>>   	.get_timeleft	= niwatchdog_wdd_get_timeleft,
>>>>>   };
>>>>> diff --git a/include/uapi/linux/niwatchdog.h b/include/uapi/linux/niwatchdog.h
>>>>> index 9d3613d..4fb8d47 100644
>>>>> --- a/include/uapi/linux/niwatchdog.h
>>>>> +++ b/include/uapi/linux/niwatchdog.h
>>>>> @@ -25,6 +25,16 @@
>>>>>   #define NIWATCHDOG_STATE_EXPIRED	1
>>>>>   #define NIWATCHDOG_STATE_DISABLED	2
>>>>>
>>>>> +#define NIWATCHDOG_IOCTL_PERIOD_NS	_IOR('W', 60, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_MAX_COUNTER	_IOR('W', 61, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_COUNTER_SET	_IOW('W', 62, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_CHECK_ACTION	_IOW('W', 63, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_ADD_ACTION	_IOW('W', 64, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_START		_IO('W', 65)
>>>>> +#define NIWATCHDOG_IOCTL_PET		_IOR('W', 66, __u32)
>>>>> +#define NIWATCHDOG_IOCTL_RESET		_IO('W', 67)
>>>>> +#define NIWATCHDOG_IOCTL_COUNTER_GET	_IOR('W', 68, __u32)
>>>>> +
>>>>>   #define NIWATCHDOG_NAME			"niwatchdog"
>>>>>
>>>>>   #endif /* _LINUX_NIWATCHDOG_H_ */
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> Thanks again for the reviews.
>>>
>>
>

Re: [2/2] niwatchdog: add support for custom ioctls

[Josh Cartwright]

On Thu, Feb 04, 2016 at 08:38:10AM -0800, Guenter Roeck wrote:
> >realize that users may not care about differences of less than a second, but it
> >seems better to err on the side of caution and provide more accuracy than
> >they would be expected to need. For instance, this watchdog operates on a
> >real-time system with any number of industrial applications which could require
> >high accuracy even from the watchdog.
>
> I don't really believe that this is or will ever be the case. I would argue that
> any system which requires such a high accuracy for a watchdog timeout has a severe
> architectural problem. After all, we are not talking about reaction time to an
> external or internal event here. We are talking about what should happen if
> something goes wrong so badly that it results in an immediate system reboot
> or hardware reset.

I think we're pushing on what the definition of a "watchdog" is, and
it's purpose is within a wider system.  Historically, the kernels' usage
of "watchdog" meant some hardware which would facilitate a reset of the
system if not fed/pet in a timely manner.  In this case, I'd definitely
agree with your statement that sub-millisecond timing is overkill and/or
a poor design.

For our "watchdog" hardware, however, resetting the hardware/CPU state
is only _one_ possible action that can be configured to occur when the
timer expires.

But other actions exist too.  In the most timing-sensitive case, our
"watchdog" is attached to to an external trigger bus, which carries
trigger signals equidistantly to a series of data acquisition devices
(or other plug-in measurement devices).  The "watchdog" can be
configured to signal one or several of these trigger lines (triggering
synchronized acquisition or whatever) in the case of expiration.

In this way, it's much more like a general user configurable countdown
timer than it is a "watchdog" in the Linux sense.  The question is
whether or not WATCHDOG_CORE should grow to include some of the
functionality required, or if that functionality should live somewhere
else.  (And if the answer is "somewhere else", how can we _also_
implement the standard watchdog interface for the case where we want
hardware reset to be the configured action).

Thanks,
  Josh

Re: [2/2] niwatchdog: add support for custom ioctls

[Kyle Roeschley]

On Thu, Feb 04, 2016 at 12:38:44PM -0600, Josh Cartwright wrote:
> On Thu, Feb 04, 2016 at 08:38:10AM -0800, Guenter Roeck wrote:
> > >realize that users may not care about differences of less than a second, but it
> > >seems better to err on the side of caution and provide more accuracy than
> > >they would be expected to need. For instance, this watchdog operates on a
> > >real-time system with any number of industrial applications which could require
> > >high accuracy even from the watchdog.
> >
> > I don't really believe that this is or will ever be the case. I would argue that
> > any system which requires such a high accuracy for a watchdog timeout has a severe
> > architectural problem. After all, we are not talking about reaction time to an
> > external or internal event here. We are talking about what should happen if
> > something goes wrong so badly that it results in an immediate system reboot
> > or hardware reset.
> 
> I think we're pushing on what the definition of a "watchdog" is, and
> it's purpose is within a wider system.  Historically, the kernels' usage
> of "watchdog" meant some hardware which would facilitate a reset of the
> system if not fed/pet in a timely manner.  In this case, I'd definitely
> agree with your statement that sub-millisecond timing is overkill and/or
> a poor design.
> 
> For our "watchdog" hardware, however, resetting the hardware/CPU state
> is only _one_ possible action that can be configured to occur when the
> timer expires.
> 
> But other actions exist too.  In the most timing-sensitive case, our
> "watchdog" is attached to to an external trigger bus, which carries
> trigger signals equidistantly to a series of data acquisition devices
> (or other plug-in measurement devices).  The "watchdog" can be
> configured to signal one or several of these trigger lines (triggering
> synchronized acquisition or whatever) in the case of expiration.
> 
> In this way, it's much more like a general user configurable countdown
> timer than it is a "watchdog" in the Linux sense.  The question is
> whether or not WATCHDOG_CORE should grow to include some of the
> functionality required, or if that functionality should live somewhere
> else.  (And if the answer is "somewhere else", how can we _also_
> implement the standard watchdog interface for the case where we want
> hardware reset to be the configured action).
> 

To this point, the current idea was to use sysfs attributes "action_reset"
(which is on by default), "action_interrupt" (which is off by default), and
"counter" (to directly read/write the counter value). However, we run into the
locking problem that I mentioned in my last email. Anyone have any ideas on
that?

Regards,

Kyle Roeschley