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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux