Re: [PATCH v3 6/6] Add Advantech iManager Watchdog driver

[Guenter Roeck]

On 01/10/2016 03:31 PM, richard.dorsch@xxxxxxxxx wrote:
> From: Richard Vidal-Dorsch <richard.dorsch@xxxxxxxxx>
>
> Signed-off-by: Richard Vidal-Dorsch <richard.dorsch@xxxxxxxxx>
> ---
>   drivers/watchdog/Kconfig           |  12 ++
>   drivers/watchdog/Makefile          |   2 +
>   drivers/watchdog/imanager-ec-wdt.c | 170 +++++++++++++++++++
>   drivers/watchdog/imanager-wdt.c    | 333 +++++++++++++++++++++++++++++++++++++
>   include/linux/mfd/imanager/wdt.h   |  37 +++++
>   5 files changed, 554 insertions(+)
>   create mode 100644 drivers/watchdog/imanager-ec-wdt.c
>   create mode 100644 drivers/watchdog/imanager-wdt.c
>   create mode 100644 include/linux/mfd/imanager/wdt.h
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1c427be..e555127 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -846,6 +846,18 @@ config ITCO_VENDOR_SUPPORT
>   	  devices. At this moment we only have additional support for some
>   	  SuperMicro Inc. motherboards.
>
> +config IMANAGER_WDT
> +	tristate "Advantech iManager Watchdog"
> +	depends on MFD_IMANAGER
> +	select WATCHDOG_CORE
> +	help
> +	  This driver provides support for Advantech iManager watchdog
> +	  of Advantech SOM, MIO, AIMB, and PCM modules/boards.
> +	  Requires mfd-core and imanager-core to function properly.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called imanager_wdt.
> +
>   config IT8712F_WDT
>   	tristate "IT8712F (Smart Guardian) Watchdog Timer"
>   	depends on X86
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 53d4827..647eca87 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -100,6 +100,8 @@ obj-$(CONFIG_ITCO_WDT) += iTCO_wdt.o
>   ifeq ($(CONFIG_ITCO_VENDOR_SUPPORT),y)
>   obj-$(CONFIG_ITCO_WDT) += iTCO_vendor_support.o
>   endif
> +imanager_wdt-objs := imanager-wdt.o imanager-ec-wdt.o
> +obj-$(CONFIG_IMANAGER_WDT) += imanager_wdt.o
>   obj-$(CONFIG_IT8712F_WDT) += it8712f_wdt.o
>   obj-$(CONFIG_IT87_WDT) += it87_wdt.o
>   obj-$(CONFIG_HP_WATCHDOG) += hpwdt.o
> diff --git a/drivers/watchdog/imanager-ec-wdt.c b/drivers/watchdog/imanager-ec-wdt.c
> new file mode 100644
> index 0000000..9c94b21
> --- /dev/null
> +++ b/drivers/watchdog/imanager-ec-wdt.c
> @@ -0,0 +1,170 @@
> +/*
> + * Advantech iManager Watchdog core
> + *
> + * Copyright (C) 2016 Advantech Co., Ltd., Irvine, CA, USA
> + * Author: Richard Vidal-Dorsch <richard.dorsch@xxxxxxxxxxxxx>
> + *
> + * 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.
> + *
> + * Note: Current release only implements RESET of the EC WDT.
> + *       In other words, no PWR button, NMI, SCI, IRQ, or WDPin support yet!
> + */
> +
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/bug.h>
> +#include <linux/io.h>
> +#include <linux/delay.h>
> +#include <linux/string.h>
> +#include <linux/byteorder/generic.h>
> +#include <linux/swab.h>
> +#include <linux/mfd/imanager/ec.h>
> +#include <linux/mfd/imanager/wdt.h>
> +
> +/* Timer resolution */
> +#define WDT_FREQ	10 /* Hz */
> +
> +enum wdt_ctrl {
> +	START = 1,
> +	STOP,
> +	RST,
> +	GET_TIMEOUT,
> +	SET_TIMEOUT,
> +	STOPBOOT = 8,
> +};
> +
> +struct wdt_event_delay {
> +	u16	delay,
> +		pwrbtn,
> +		nmi,
> +		reset,
> +		wdpin,
> +		sci,
> +		dummy;
> +};
> +
> +static const struct imanager_watchdog_device *wd;
> +

I am not entirely happy with those static variables. Any chance
to just read it in the wdt file and pass it around ?

> +static inline int set_timer(enum wdt_ctrl ctrl)
> +{
> +	if (WARN_ON(ctrl == SET_TIMEOUT))
> +		return -EINVAL;
> +

Unnecessary error message and warning.

> +	return imanager_msg_write(EC_CMD_WDT_CTRL, ctrl, NULL);
> +}
> +
> +static int
> +wdt_ctrl(enum wdt_ctrl ctrl, enum wdt_event type, unsigned int timeout)
> +{
> +	u16 val;
> +	int ret;
> +	struct ec_message msg = {
> +		.rlen = 0,
> +		.wlen = 0,
> +	};
> +	u8 *fevent = &msg.u.data[0];
> +	struct wdt_event_delay *event =
> +				(struct wdt_event_delay *)&msg.u.data[1];
> +
> +	switch (ctrl) {
> +	case SET_TIMEOUT:
> +		memset(event, 0xff, sizeof(*event));
> +		msg.wlen = sizeof(*event);
> +		*fevent = 0;
> +		val = (!timeout) ? 0xffff : swab16(timeout * WDT_FREQ);

cpu_to_be16()

> +
> +		switch (type) {
> +		case DELAY:
> +			event->delay = val;
> +			break;
> +		case PWRBTN:
> +			event->pwrbtn = val;
> +			break;
> +		case NMI:
> +			event->nmi = val;
> +			break;
> +		case RESET:
> +			event->reset = val;
> +			break;
> +		case WDPIN:
> +			event->wdpin = val;
> +			break;
> +		case SCI:
> +			event->sci = val;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		ret = imanager_msg_write(EC_CMD_WDT_CTRL, SET_TIMEOUT, &msg);
> +		if (ret < 0) {
> +			pr_err("Failed to set timeout\n");

Please consider dropping the error messages.

> +			return ret;
> +		}
> +		break;
> +	case START:
> +	case STOP:
> +	case RST:
> +	case STOPBOOT:
> +		/* simple command, no data */
> +		return imanager_msg_write(EC_CMD_WDT_CTRL, ctrl, NULL);

Why not use simple(r) wrappers for those or just call imanager_msg_write()
directly, as you do it for set_timer() ?

Actually, you never call wdt_ctrl() with anything but SET_TIMEOUT as parameter,
so everything else in this function seems to be unused.

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return timeout;
> +}
> +
> +int wdt_core_start_timer(void)
> +{
> +	return set_timer(START);
> +}
> +
> +int wdt_core_stop_timer(void)
> +{
> +	return set_timer(STOP);
> +}
> +
> +int wdt_core_reset_timer(void)
> +{
> +	return set_timer(RST);
> +}
> +
> +int wdt_core_stop_boot_timer(void)
> +{
> +	return set_timer(STOPBOOT);
> +}
> +
> +inline int wdt_core_set_timeout(enum wdt_event type, u32 timeout)
> +{
> +	return wdt_ctrl(SET_TIMEOUT, type, timeout);
> +}
> +
> +int wdt_core_disable_all(void)

What is the point of a return value which is never checked ?

> +{
> +	struct ec_message msg = {
> +		.rlen = 0,
> +		.wlen = sizeof(struct wdt_event_delay),
> +	};
> +	struct wdt_event_delay *event =
> +				(struct wdt_event_delay *)&msg.u.data[1];
> +
> +	memset(event, 0xff, sizeof(*event));
> +
> +	return  (wdt_core_stop_timer() ||
> +		 wdt_core_stop_boot_timer() ||
> +		 imanager_msg_write(EC_CMD_WDT_CTRL, SET_TIMEOUT, &msg));

Please use individual error checks here.

> +}
> +
> +int wdt_core_init(void)
> +{
> +	wd = imanager_get_watchdog_device();
> +	if (!wd)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> diff --git a/drivers/watchdog/imanager-wdt.c b/drivers/watchdog/imanager-wdt.c
> new file mode 100644
> index 0000000..10f8e22
> --- /dev/null
> +++ b/drivers/watchdog/imanager-wdt.c
> @@ -0,0 +1,333 @@
> +/*
> + * Advantech iManager Watchdog driver
> + *
> + * Copyright (C) 2016 Advantech Co., Ltd., Irvine, CA, USA
> + * Author: Richard Vidal-Dorsch <richard.dorsch@xxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
> +#include <linux/uaccess.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/imanager/core.h>
> +#include <linux/mfd/imanager/wdt.h>
> +
> +#define WATCHDOG_TIMEOUT 30 /* in seconds */
> +
> +static unsigned long last_updated = -1;
> +
> +static uint timeout = WATCHDOG_TIMEOUT;
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +	"Watchdog timeout in seconds. 1 <= timeout <= 65534, default="
> +	__MODULE_STRING(WATCHDOG_TIMEOUT) ".");
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +	"Watchdog cannot be stopped once started (default="
> +	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static int wdt_start_timer(void)
> +{
> +	int ret;
> +
> +	ret = wdt_core_start_timer();
> +	if (ret < 0)
> +		return ret;
> +
> +	last_updated = jiffies;
> +
> +	return 0;
> +}
> +
> +static int wdt_stop_timer(void)
> +{
> +	int ret;
> +
> +	ret = wdt_core_stop_timer();
> +	if (ret < 0)
> +		return ret;
> +
> +	last_updated = 0;
> +
> +	return 0;
> +}
> +
> +static int wdt_ping(void)
> +{
> +	int ret;
> +
> +	ret = wdt_core_reset_timer();
> +	if (ret < 0)
> +		return ret;
> +
> +	last_updated = jiffies;
> +
> +	return 0;
> +}
> +
> +static int imanager_wdt_set(unsigned int _timeout)
> +{
> +	int ret;
> +
> +	if (timeout != _timeout)
> +		timeout = _timeout;
> +
> +	ret = wdt_core_set_timeout(PWRBTN, timeout);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (last_updated != 0)
> +		last_updated = jiffies;
> +
> +	return 0;
> +}
> +
> +static int imanager_wdt_set_timeout(struct watchdog_device *wdt_dev,
> +				    unsigned int timeout)
> +{
> +	struct imanager_device_data *idev = watchdog_get_drvdata(wdt_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&idev->lock);
> +
> +	ret = imanager_wdt_set(timeout);
> +	if (ret < 0)
> +		dev_err(wdt_dev->dev, "Failed to set timeout\n");
> +	mutex_unlock(&idev->lock);
> +
> +	return ret;
> +}
> +
> +static unsigned int imanager_wdt_get_timeleft(struct watchdog_device *wdt_dev)
> +{
> +	unsigned int timeleft = 0;
> +	unsigned long time_diff = ((jiffies - last_updated) / HZ);
> +
> +	if (last_updated && (timeout > time_diff))
> +		timeleft = timeout - time_diff;
> +

This is supposed to be provided by the hardware. Please do not implement
get_timeleft in software (if we wanted to do that we could do it in the
watchdog core).

> +	return timeleft;
> +}
> +
> +static int imanager_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	struct imanager_device_data *idev = watchdog_get_drvdata(wdt_dev);
> +	int ret = 0;

Unnecessary variable initialization.

> +
> +	mutex_lock(&idev->lock);
> +

The watchdog core handles locking for watchdog devices.

> +	ret = wdt_start_timer();
> +	if (ret < 0)
> +		dev_err(wdt_dev->dev, "Failed to start timer\n");
> +
> +	mutex_unlock(&idev->lock);
> +
> +	return ret;
> +}
> +
> +static int imanager_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	struct imanager_device_data *idev = watchdog_get_drvdata(wdt_dev);
> +	int ret = 0;

Unnecessary variable initialization.

> +
> +	mutex_lock(&idev->lock);
> +
> +	ret = wdt_stop_timer();
> +	if (ret < 0)
> +		dev_err(wdt_dev->dev, "Failed to stop timer\n");
> +
> +	mutex_unlock(&idev->lock);
> +
> +	return ret;
> +}
> +
> +static int imanager_wdt_ping(struct watchdog_device *wdt_dev)
> +{
> +	struct imanager_device_data *idev = watchdog_get_drvdata(wdt_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&idev->lock);
> +
> +	ret = wdt_ping();
> +	if (ret < 0)
> +		dev_err(wdt_dev->dev, "Failed to reset timer\n");

Please consider dropping the error messages.

> +
> +	mutex_unlock(&idev->lock);
> +
> +	return ret;
> +}
> +
> +static long imanager_wdt_ioctl(struct watchdog_device *wdt_dev,
> +			       unsigned int cmd, unsigned long arg)
> +{

Why do you re-implement this function ?

> +	struct imanager_device_data *idev = watchdog_get_drvdata(wdt_dev);
> +	void __user *argp = (void __user *)arg;
> +	int __user *p = argp;
> +	int ret = 0;
> +	int timeval, options;
> +	static const struct watchdog_info ident = {
> +		.options = WDIOF_KEEPALIVEPING |
> +			   WDIOF_SETTIMEOUT |
> +			   WDIOF_MAGICCLOSE,
> +		.firmware_version = 0,
> +		.identity = "imanager_wdt",
> +	};
> +
> +	mutex_lock(&idev->lock);
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		if (copy_to_user(argp, &ident, sizeof(ident)))
> +			ret = -EFAULT;
> +		break;
> +	case WDIOC_GETSTATUS:
> +	case WDIOC_GETBOOTSTATUS:
> +		ret = put_user(0, p);
> +		break;
> +	case WDIOC_SETOPTIONS:
> +		if (get_user(options, p)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +		if (options & WDIOS_DISABLECARD)
> +			wdt_stop_timer();
> +		if (options & WDIOS_ENABLECARD) {
> +			wdt_ping();
> +			wdt_start_timer();
> +		}
> +		break;
> +	case WDIOC_KEEPALIVE:
> +		wdt_ping();
> +		break;
> +	case WDIOC_SETTIMEOUT:
> +		if (get_user(timeval, p)) {
> +			ret = -EFAULT;
> +			goto out;
> +		}
> +		if (imanager_wdt_set(timeval)) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		wdt_ping();
> +		/* Fall through */
> +	case WDIOC_GETTIMEOUT:
> +		ret = put_user(timeout, p);
> +		break;
> +	case WDIOC_GETTIMELEFT:
> +		timeval = imanager_wdt_get_timeleft(wdt_dev);
> +		ret = put_user(timeval, p);
> +		break;
> +	default:
> +		ret = -ENOTTY;
> +	}
> +
> +out:
> +	mutex_unlock(&idev->lock);
> +
> +	return ret;
> +}
> +
> +static const struct watchdog_info imanager_wdt_info = {
> +	.options		= WDIOF_SETTIMEOUT |
> +				  WDIOF_KEEPALIVEPING |
> +				  WDIOF_MAGICCLOSE,
> +	.identity		= "imanager_wdt",
> +	.firmware_version	= 0,
> +};
> +
> +static const struct watchdog_ops imanager_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= imanager_wdt_start,
> +	.stop		= imanager_wdt_stop,
> +	.ping		= imanager_wdt_ping,
> +	.set_timeout	= imanager_wdt_set_timeout,
> +	.get_timeleft	= imanager_wdt_get_timeleft,
> +	.ioctl		= imanager_wdt_ioctl,
> +};
> +
> +static struct watchdog_device imanager_wdt_dev = {
> +	.info		= &imanager_wdt_info,
> +	.ops		= &imanager_wdt_ops,
> +	.timeout	= WATCHDOG_TIMEOUT,
> +	.min_timeout	= 1,
> +	.max_timeout	= 0xfffe,
> +};
> +
> +static int imanager_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imanager_device_data *idev = dev_get_drvdata(dev->parent);
> +	int ret;
> +
> +	if (!idev) {
> +		dev_err(dev, "Invalid platform data\n");

-ENODEV, and please no error message here.

> +		return -EINVAL;
> +	}
> +
> +	watchdog_set_nowayout(&imanager_wdt_dev, nowayout);
> +	watchdog_set_drvdata(&imanager_wdt_dev, idev);
> +
> +	ret = watchdog_register_device(&imanager_wdt_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	ret = wdt_core_init();

Shouldn't that come first, before you register the watchdog device ?

> +	if (ret) {
> +		dev_err(dev, "Failed to initialize watchdog core\n");

Please no error message here (the error indicates that there is no watchdog device,
which is controlled by the mfd driver and would be on purpose if it happens).

> +		goto unregister_driver;
> +	}
> +
> +	wdt_core_disable_all();

After registration there is a distinct possibility that the watchdog has already
been enabled by user space by the time this code executes. Disabling it here
might be racy.

> +
> +	imanager_wdt_set_timeout(&imanager_wdt_dev, timeout);
> +
Same here - in theory, user space could already have opened the watchdog device
and changed the timeout. I would suggest to use watchdog_init_timeout() prior
to registering the watchdog.

> +	dev_info(dev, "Driver loaded (timeout=%d seconds)\n", timeout);
> +
> +	return 0;
> +
> +unregister_driver:
> +	watchdog_unregister_device(&imanager_wdt_dev);
> +	platform_set_drvdata(pdev, NULL);

Unnecessary.

> +
> +	return ret;
> +}
> +
> +static int imanager_wdt_remove(struct platform_device *pdev)
> +{
> +	wdt_core_disable_all();
> +
> +	watchdog_unregister_device(&imanager_wdt_dev);
> +	platform_set_drvdata(pdev, NULL);

Unnecessary.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver imanager_wdt_driver = {
> +	.driver = {
> +		.name	= "imanager_wdt",
> +	},
> +	.probe	= imanager_wdt_probe,
> +	.remove	= imanager_wdt_remove,
> +};
> +
> +module_platform_driver(imanager_wdt_driver);
> +
> +MODULE_DESCRIPTION("Advantech iManager Watchdog Driver");
> +MODULE_AUTHOR("Richard Vidal-Dorsch <richard.dorsch at advantech.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:imanager_wdt");
> diff --git a/include/linux/mfd/imanager/wdt.h b/include/linux/mfd/imanager/wdt.h
> new file mode 100644
> index 0000000..eb709b7
> --- /dev/null
> +++ b/include/linux/mfd/imanager/wdt.h
> @@ -0,0 +1,37 @@
> +/*
> + * Advantech iManager Watchdog core
> + *
> + * Copyright (C) 2016 Advantech Co., Ltd., Irvine, CA, USA
> + * Author: Richard Vidal-Dorsch <richard.dorsch@xxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#ifndef __WDT_H__
> +#define __WDT_H__
> +
> +#include <linux/types.h>
> +
> +enum wdt_event {
> +	DELAY,
> +	PWRBTN,
> +	NMI,
> +	RESET,
> +	WDPIN,
> +	SCI,
> +};
> +
> +int wdt_core_init(void);
> +
> +int wdt_core_set_timeout(enum wdt_event type, u32 timeout);
> +
> +int wdt_core_disable_all(void);
> +int wdt_core_start_timer(void);
> +int wdt_core_stop_timer(void);
> +int wdt_core_reset_timer(void);
> +int wdt_core_stop_boot_timer(void);
> +
> +#endif