* [PATCH 1/2] watchdog: add NI 903x/913x watchdog support @ 2016-01-12 0:23 Kyle Roeschley 2016-01-12 0:23 ` [PATCH 2/2] niwatchdog: add support for custom ioctls Kyle Roeschley 2016-01-17 4:25 ` [1/2] watchdog: add NI 903x/913x watchdog support Guenter Roeck 0 siblings, 2 replies; 11+ messages in thread From: Kyle Roeschley @ 2016-01-12 0:23 UTC (permalink / raw) To: wim; +Cc: linux-watchdog 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] niwatchdog: add support for custom ioctls 2016-01-12 0:23 [PATCH 1/2] watchdog: add NI 903x/913x watchdog support Kyle Roeschley @ 2016-01-12 0:23 ` Kyle Roeschley 2016-01-17 4:29 ` [2/2] " Guenter Roeck 2016-01-17 4:25 ` [1/2] watchdog: add NI 903x/913x watchdog support Guenter Roeck 1 sibling, 1 reply; 11+ messages in thread From: Kyle Roeschley @ 2016-01-12 0:23 UTC (permalink / raw) To: wim; +Cc: linux-watchdog 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-01-12 0:23 ` [PATCH 2/2] niwatchdog: add support for custom ioctls Kyle Roeschley @ 2016-01-17 4:29 ` Guenter Roeck 2016-01-25 23:31 ` Kyle Roeschley 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2016-01-17 4:29 UTC (permalink / raw) To: Kyle Roeschley; +Cc: wim, linux-watchdog 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_ */ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-01-17 4:29 ` [2/2] " Guenter Roeck @ 2016-01-25 23:31 ` Kyle Roeschley 2016-01-26 1:00 ` Guenter Roeck 0 siblings, 1 reply; 11+ messages in thread From: Kyle Roeschley @ 2016-01-25 23:31 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-watchdog 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-01-25 23:31 ` Kyle Roeschley @ 2016-01-26 1:00 ` Guenter Roeck 2016-02-03 0:44 ` Kyle Roeschley 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2016-01-26 1:00 UTC (permalink / raw) To: Kyle Roeschley; +Cc: linux-watchdog 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-01-26 1:00 ` Guenter Roeck @ 2016-02-03 0:44 ` Kyle Roeschley 2016-02-04 16:38 ` Guenter Roeck 0 siblings, 1 reply; 11+ messages in thread From: Kyle Roeschley @ 2016-02-03 0:44 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-watchdog 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. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-02-03 0:44 ` Kyle Roeschley @ 2016-02-04 16:38 ` Guenter Roeck 2016-02-04 18:38 ` Josh Cartwright 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2016-02-04 16:38 UTC (permalink / raw) To: Kyle Roeschley; +Cc: linux-watchdog 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. >>> >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-02-04 16:38 ` Guenter Roeck @ 2016-02-04 18:38 ` Josh Cartwright 2016-02-04 20:47 ` Kyle Roeschley 0 siblings, 1 reply; 11+ messages in thread From: Josh Cartwright @ 2016-02-04 18:38 UTC (permalink / raw) To: Guenter Roeck; +Cc: Kyle Roeschley, linux-watchdog 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] niwatchdog: add support for custom ioctls 2016-02-04 18:38 ` Josh Cartwright @ 2016-02-04 20:47 ` Kyle Roeschley 0 siblings, 0 replies; 11+ messages in thread From: Kyle Roeschley @ 2016-02-04 20:47 UTC (permalink / raw) To: Josh Cartwright; +Cc: linux-watchdog 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [1/2] watchdog: add NI 903x/913x watchdog support 2016-01-12 0:23 [PATCH 1/2] watchdog: add NI 903x/913x watchdog support Kyle Roeschley 2016-01-12 0:23 ` [PATCH 2/2] niwatchdog: add support for custom ioctls Kyle Roeschley @ 2016-01-17 4:25 ` Guenter Roeck 2016-01-25 23:21 ` Kyle Roeschley 1 sibling, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2016-01-17 4:25 UTC (permalink / raw) To: Kyle Roeschley; +Cc: wim, linux-watchdog 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_ */ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [1/2] watchdog: add NI 903x/913x watchdog support 2016-01-17 4:25 ` [1/2] watchdog: add NI 903x/913x watchdog support Guenter Roeck @ 2016-01-25 23:21 ` Kyle Roeschley 0 siblings, 0 replies; 11+ messages in thread From: Kyle Roeschley @ 2016-01-25 23:21 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-watchdog 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-02-04 21:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-12 0:23 [PATCH 1/2] watchdog: add NI 903x/913x watchdog support Kyle Roeschley 2016-01-12 0:23 ` [PATCH 2/2] niwatchdog: add support for custom ioctls Kyle Roeschley 2016-01-17 4:29 ` [2/2] " Guenter Roeck 2016-01-25 23:31 ` Kyle Roeschley 2016-01-26 1:00 ` Guenter Roeck 2016-02-03 0:44 ` Kyle Roeschley 2016-02-04 16:38 ` Guenter Roeck 2016-02-04 18:38 ` Josh Cartwright 2016-02-04 20:47 ` Kyle Roeschley 2016-01-17 4:25 ` [1/2] watchdog: add NI 903x/913x watchdog support Guenter Roeck 2016-01-25 23:21 ` Kyle Roeschley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.