[PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[Mark Balantzyan]

This patch rewrites the alim1535_wdt driver to use the watchdog subsystem. 
By virtue of this, it also fixes a (theoretical) race condition between the
formerly arranged ali_timeout_bits and ali_settimer() interoperation.

Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>

---
 drivers/watchdog/Kconfig        |   1 +
 drivers/watchdog/alim1535_wdt.c | 275 +++++---------------------------
 2 files changed, 37 insertions(+), 239 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9af07fd9..980b0c90 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -853,6 +853,7 @@ config ADVANTECH_WDT
 
 config ALIM1535_WDT
 	tristate "ALi M1535 PMU Watchdog Timer"
+	select WATCHDOG_CORE
 	depends on X86 && PCI
 	---help---
 	  This is the driver for the hardware watchdog on the ALi M1535 PMU.
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 60f0c2eb..55648ba8 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -12,26 +12,18 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>
 #include <linux/watchdog.h>
 #include <linux/ioport.h>
-#include <linux/notifier.h>
-#include <linux/reboot.h>
-#include <linux/init.h>
-#include <linux/fs.h>
-#include <linux/pci.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/pci.h>
 
 #define WATCHDOG_NAME "ALi_M1535"
 #define WATCHDOG_TIMEOUT 60	/* 60 sec default timeout */
 
 /* internal variables */
-static unsigned long ali_is_open;
-static char ali_expect_release;
 static struct pci_dev *ali_pci;
 static u32 ali_timeout_bits;		/* stores the computed timeout */
-static DEFINE_SPINLOCK(ali_lock);	/* Guards the hardware */
 
 /* module parameters */
 static int timeout = WATCHDOG_TIMEOUT;
@@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
  *	configuration set.
  */
 
-static void ali_start(void)
+static int ali_start(struct watchdog_device *wdd)
 {
 	u32 val;
 
-	spin_lock(&ali_lock);
-
 	pci_read_config_dword(ali_pci, 0xCC, &val);
 	val &= ~0x3F;	/* Mask count */
 	val |= (1 << 25) | ali_timeout_bits;
 	pci_write_config_dword(ali_pci, 0xCC, val);
-
-	spin_unlock(&ali_lock);
+	return 0;
 }
 
 /*
@@ -73,18 +62,15 @@ static void ali_start(void)
  *	Stop the ALi watchdog countdown
  */
 
-static void ali_stop(void)
+static int ali_stop(struct watchdog_device *wdd)
 {
 	u32 val;
 
-	spin_lock(&ali_lock);
-
 	pci_read_config_dword(ali_pci, 0xCC, &val);
 	val &= ~0x3F;		/* Mask count to zero (disabled) */
 	val &= ~(1 << 25);	/* and for safety mask the reset enable */
 	pci_write_config_dword(ali_pci, 0xCC, val);
-
-	spin_unlock(&ali_lock);
+	return 0;
 }
 
 /*
@@ -93,32 +79,24 @@ static void ali_stop(void)
  *	Send a keepalive to the timer (actually we restart the timer).
  */
 
-static void ali_keepalive(void)
+static int ali_keepalive(struct watchdog_device *wdd)
 {
-	ali_start();
+	ali_start(wdd);
+	return 0;
 }
 
 /*
- *	ali_settimer	-	compute the timer reload value
+ *	ali_set_timeout	-	compute the timer reload value
  *	@t: time in seconds
  *
  *	Computes the timeout values needed
  */
 
-static int ali_settimer(int t)
+static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
 {
-	if (t < 0)
-		return -EINVAL;
-	else if (t < 60)
-		ali_timeout_bits = t|(1 << 6);
-	else if (t < 3600)
-		ali_timeout_bits = (t / 60)|(1 << 7);
-	else if (t < 18000)
-		ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
-	else
-		return -EINVAL;
-
-	timeout = t;
+	wdd->max_timeout = 60;
+	wdd->min_timeout = 1;
+	wdd->timeout = t;
 	return 0;
 }
 
@@ -126,172 +104,6 @@ static int ali_settimer(int t)
  *	/dev/watchdog handling
  */
 
-/*
- *	ali_write	-	writes to ALi watchdog
- *	@file: file from VFS
- *	@data: user address of data
- *	@len: length of data
- *	@ppos: pointer to the file offset
- *
- *	Handle a write to the ALi watchdog. Writing to the file pings
- *	the watchdog and resets it. Writing the magic 'V' sequence allows
- *	the next close to turn off the watchdog.
- */
-
-static ssize_t ali_write(struct file *file, const char __user *data,
-						size_t len, loff_t *ppos)
-{
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* note: just in case someone wrote the
-			   magic character five months ago... */
-			ali_expect_release = 0;
-
-			/* scan to see whether or not we got
-			   the magic character */
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					ali_expect_release = 42;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		ali_start();
-	}
-	return len;
-}
-
-/*
- *	ali_ioctl	-	handle watchdog ioctls
- *	@file: VFS file pointer
- *	@cmd: ioctl number
- *	@arg: arguments to the ioctl
- *
- *	Handle the watchdog ioctls supported by the ALi driver. Really
- *	we want an extension to enable irq ack monitoring and the like
- */
-
-static long ali_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_KEEPALIVEPING |
-					WDIOF_SETTIMEOUT |
-					WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		"ALi M1535 WatchDog Timer",
-	};
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-	case WDIOC_SETOPTIONS:
-	{
-		int new_options, retval = -EINVAL;
-
-		if (get_user(new_options, p))
-			return -EFAULT;
-		if (new_options & WDIOS_DISABLECARD) {
-			ali_stop();
-			retval = 0;
-		}
-		if (new_options & WDIOS_ENABLECARD) {
-			ali_start();
-			retval = 0;
-		}
-		return retval;
-	}
-	case WDIOC_KEEPALIVE:
-		ali_keepalive();
-		return 0;
-	case WDIOC_SETTIMEOUT:
-	{
-		int new_timeout;
-		if (get_user(new_timeout, p))
-			return -EFAULT;
-		if (ali_settimer(new_timeout))
-			return -EINVAL;
-		ali_keepalive();
-	}
-		/* fall through */
-	case WDIOC_GETTIMEOUT:
-		return put_user(timeout, p);
-	default:
-		return -ENOTTY;
-	}
-}
-
-/*
- *	ali_open	-	handle open of ali watchdog
- *	@inode: inode from VFS
- *	@file: file from VFS
- *
- *	Open the ALi watchdog device. Ensure only one person opens it
- *	at a time. Also start the watchdog running.
- */
-
-static int ali_open(struct inode *inode, struct file *file)
-{
-	/* /dev/watchdog can only be opened once */
-	if (test_and_set_bit(0, &ali_is_open))
-		return -EBUSY;
-
-	/* Activate */
-	ali_start();
-	return nonseekable_open(inode, file);
-}
-
-/*
- *	ali_release	-	close an ALi watchdog
- *	@inode: inode from VFS
- *	@file: file from VFS
- *
- *	Close the ALi watchdog device. Actual shutdown of the timer
- *	only occurs if the magic sequence has been set.
- */
-
-static int ali_release(struct inode *inode, struct file *file)
-{
-	/*
-	 *      Shut off the timer.
-	 */
-	if (ali_expect_release == 42)
-		ali_stop();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		ali_keepalive();
-	}
-	clear_bit(0, &ali_is_open);
-	ali_expect_release = 0;
-	return 0;
-}
-
-/*
- *	ali_notify_sys	-	System down notifier
- *
- *	Notifier for system down
- */
-
-
-static int ali_notify_sys(struct notifier_block *this,
-					unsigned long code, void *unused)
-{
-	if (code == SYS_DOWN || code == SYS_HALT)
-		ali_stop();		/* Turn the WDT off */
-	return NOTIFY_DONE;
-}
-
 /*
  *	Data for PCI driver interface
  *
@@ -361,23 +173,17 @@ static int __init ali_find_watchdog(void)
  *	Kernel Interfaces
  */
 
-static const struct file_operations ali_fops = {
-	.owner		=	THIS_MODULE,
-	.llseek		=	no_llseek,
-	.write		=	ali_write,
-	.unlocked_ioctl =	ali_ioctl,
-	.open		=	ali_open,
-	.release	=	ali_release,
+static struct watchdog_ops alim1535wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = ali_start,
+	.stop = ali_stop,
+	.ping = ali_keepalive,
+	.set_timeout = ali_set_timeout,
 };
 
-static struct miscdevice ali_miscdev = {
-	.minor =	WATCHDOG_MINOR,
-	.name =		"watchdog",
-	.fops =		&ali_fops,
-};
-
-static struct notifier_block ali_notifier = {
-	.notifier_call =	ali_notify_sys,
+static struct watchdog_device alim1535wdt_wdd = {
+	.ops = &alim1535wdt_ops,
+	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
 };
 
 /*
@@ -403,30 +209,25 @@ static int __init watchdog_init(void)
 			timeout);
 	}
 
-	/* Calculate the watchdog's timeout */
-	ali_settimer(timeout);
+	watchdog_stop_on_reboot(&alim1535wdt_wdd);
+
+	watchdog_init_timeout(&alim1535wdt_wdd, timeout, NULL);
+
+	watchdog_set_nowayout(&alim1535wdt_wdd, nowayout);
 
-	ret = register_reboot_notifier(&ali_notifier);
-	if (ret != 0) {
-		pr_err("cannot register reboot notifier (err=%d)\n", ret);
-		goto out;
-	}
+	ret = watchdog_register_device(&alim1535wdt_wdd);
 
-	ret = misc_register(&ali_miscdev);
 	if (ret != 0) {
-		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
-		       WATCHDOG_MINOR, ret);
-		goto unreg_reboot;
+		goto reboot_unreg;
 	}
 
-	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
-		timeout, nowayout);
+	/* Calculate the watchdog's timeout */
+	ali_set_timeout(&alim1535wdt_wdd, timeout);
+
+	return 0;
 
-out:
+reboot_unreg:
 	return ret;
-unreg_reboot:
-	unregister_reboot_notifier(&ali_notifier);
-	goto out;
 }
 
 /*
@@ -437,12 +238,8 @@ unreg_reboot:
 
 static void __exit watchdog_exit(void)
 {
-	/* Stop the timer before we leave */
-	ali_stop();
-
 	/* Deregister */
-	misc_deregister(&ali_miscdev);
-	unregister_reboot_notifier(&ali_notifier);
+	watchdog_unregister_device(&alim1535wdt_wdd);
 	pci_dev_put(ali_pci);
 }
 
@@ -451,4 +248,4 @@ module_exit(watchdog_exit);
 
 MODULE_AUTHOR("Alan Cox");
 MODULE_DESCRIPTION("ALi M1535 PMU Watchdog Timer driver");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[Guenter Roeck]

On 8/1/19 8:26 PM, Mark Balantzyan wrote:
> This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
> By virtue of this, it also fixes a (theoretical) race condition between the
> formerly arranged ali_timeout_bits and ali_settimer() interoperation.
> 
> Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>


This is v4. A minute ago I received v1. Earlier today I got v2, which I
commented on. I didn't see v3 of this patch. I don't see a change log.
At first glance, I don't see a difference to v2.

Sorry, this is a waste of my time. I won't review or comment further.

Guenter

> 
> ---
>   drivers/watchdog/Kconfig        |   1 +
>   drivers/watchdog/alim1535_wdt.c | 275 +++++---------------------------
>   2 files changed, 37 insertions(+), 239 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9af07fd9..980b0c90 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -853,6 +853,7 @@ config ADVANTECH_WDT
>   
>   config ALIM1535_WDT
>   	tristate "ALi M1535 PMU Watchdog Timer"
> +	select WATCHDOG_CORE
>   	depends on X86 && PCI
>   	---help---
>   	  This is the driver for the hardware watchdog on the ALi M1535 PMU.
> diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
> index 60f0c2eb..55648ba8 100644
> --- a/drivers/watchdog/alim1535_wdt.c
> +++ b/drivers/watchdog/alim1535_wdt.c
> @@ -12,26 +12,18 @@
>   #include <linux/module.h>
>   #include <linux/moduleparam.h>
>   #include <linux/types.h>
> -#include <linux/miscdevice.h>
>   #include <linux/watchdog.h>
>   #include <linux/ioport.h>
> -#include <linux/notifier.h>
> -#include <linux/reboot.h>
> -#include <linux/init.h>
> -#include <linux/fs.h>
> -#include <linux/pci.h>
>   #include <linux/uaccess.h>
>   #include <linux/io.h>
> +#include <linux/pci.h>
>   
>   #define WATCHDOG_NAME "ALi_M1535"
>   #define WATCHDOG_TIMEOUT 60	/* 60 sec default timeout */
>   
>   /* internal variables */
> -static unsigned long ali_is_open;
> -static char ali_expect_release;
>   static struct pci_dev *ali_pci;
>   static u32 ali_timeout_bits;		/* stores the computed timeout */
> -static DEFINE_SPINLOCK(ali_lock);	/* Guards the hardware */
>   
>   /* module parameters */
>   static int timeout = WATCHDOG_TIMEOUT;
> @@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
>    *	configuration set.
>    */
>   
> -static void ali_start(void)
> +static int ali_start(struct watchdog_device *wdd)
>   {
>   	u32 val;
>   
> -	spin_lock(&ali_lock);
> -
>   	pci_read_config_dword(ali_pci, 0xCC, &val);
>   	val &= ~0x3F;	/* Mask count */
>   	val |= (1 << 25) | ali_timeout_bits;
>   	pci_write_config_dword(ali_pci, 0xCC, val);
> -
> -	spin_unlock(&ali_lock);
> +	return 0;
>   }
>   
>   /*
> @@ -73,18 +62,15 @@ static void ali_start(void)
>    *	Stop the ALi watchdog countdown
>    */
>   
> -static void ali_stop(void)
> +static int ali_stop(struct watchdog_device *wdd)
>   {
>   	u32 val;
>   
> -	spin_lock(&ali_lock);
> -
>   	pci_read_config_dword(ali_pci, 0xCC, &val);
>   	val &= ~0x3F;		/* Mask count to zero (disabled) */
>   	val &= ~(1 << 25);	/* and for safety mask the reset enable */
>   	pci_write_config_dword(ali_pci, 0xCC, val);
> -
> -	spin_unlock(&ali_lock);
> +	return 0;
>   }
>   
>   /*
> @@ -93,32 +79,24 @@ static void ali_stop(void)
>    *	Send a keepalive to the timer (actually we restart the timer).
>    */
>   
> -static void ali_keepalive(void)
> +static int ali_keepalive(struct watchdog_device *wdd)
>   {
> -	ali_start();
> +	ali_start(wdd);
> +	return 0;
>   }
>   
>   /*
> - *	ali_settimer	-	compute the timer reload value
> + *	ali_set_timeout	-	compute the timer reload value
>    *	@t: time in seconds
>    *
>    *	Computes the timeout values needed
>    */
>   
> -static int ali_settimer(int t)
> +static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
>   {
> -	if (t < 0)
> -		return -EINVAL;
> -	else if (t < 60)
> -		ali_timeout_bits = t|(1 << 6);
> -	else if (t < 3600)
> -		ali_timeout_bits = (t / 60)|(1 << 7);
> -	else if (t < 18000)
> -		ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
> -	else
> -		return -EINVAL;
> -
> -	timeout = t;
> +	wdd->max_timeout = 60;
> +	wdd->min_timeout = 1;
> +	wdd->timeout = t;
>   	return 0;
>   }
>   
> @@ -126,172 +104,6 @@ static int ali_settimer(int t)
>    *	/dev/watchdog handling
>    */
>   
> -/*
> - *	ali_write	-	writes to ALi watchdog
> - *	@file: file from VFS
> - *	@data: user address of data
> - *	@len: length of data
> - *	@ppos: pointer to the file offset
> - *
> - *	Handle a write to the ALi watchdog. Writing to the file pings
> - *	the watchdog and resets it. Writing the magic 'V' sequence allows
> - *	the next close to turn off the watchdog.
> - */
> -
> -static ssize_t ali_write(struct file *file, const char __user *data,
> -						size_t len, loff_t *ppos)
> -{
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* note: just in case someone wrote the
> -			   magic character five months ago... */
> -			ali_expect_release = 0;
> -
> -			/* scan to see whether or not we got
> -			   the magic character */
> -			for (i = 0; i != len; i++) {
> -				char c;
> -				if (get_user(c, data + i))
> -					return -EFAULT;
> -				if (c == 'V')
> -					ali_expect_release = 42;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		ali_start();
> -	}
> -	return len;
> -}
> -
> -/*
> - *	ali_ioctl	-	handle watchdog ioctls
> - *	@file: VFS file pointer
> - *	@cmd: ioctl number
> - *	@arg: arguments to the ioctl
> - *
> - *	Handle the watchdog ioctls supported by the ALi driver. Really
> - *	we want an extension to enable irq ack monitoring and the like
> - */
> -
> -static long ali_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> -{
> -	void __user *argp = (void __user *)arg;
> -	int __user *p = argp;
> -	static const struct watchdog_info ident = {
> -		.options =		WDIOF_KEEPALIVEPING |
> -					WDIOF_SETTIMEOUT |
> -					WDIOF_MAGICCLOSE,
> -		.firmware_version =	0,
> -		.identity =		"ALi M1535 WatchDog Timer",
> -	};
> -
> -	switch (cmd) {
> -	case WDIOC_GETSUPPORT:
> -		return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
> -
> -	case WDIOC_GETSTATUS:
> -	case WDIOC_GETBOOTSTATUS:
> -		return put_user(0, p);
> -	case WDIOC_SETOPTIONS:
> -	{
> -		int new_options, retval = -EINVAL;
> -
> -		if (get_user(new_options, p))
> -			return -EFAULT;
> -		if (new_options & WDIOS_DISABLECARD) {
> -			ali_stop();
> -			retval = 0;
> -		}
> -		if (new_options & WDIOS_ENABLECARD) {
> -			ali_start();
> -			retval = 0;
> -		}
> -		return retval;
> -	}
> -	case WDIOC_KEEPALIVE:
> -		ali_keepalive();
> -		return 0;
> -	case WDIOC_SETTIMEOUT:
> -	{
> -		int new_timeout;
> -		if (get_user(new_timeout, p))
> -			return -EFAULT;
> -		if (ali_settimer(new_timeout))
> -			return -EINVAL;
> -		ali_keepalive();
> -	}
> -		/* fall through */
> -	case WDIOC_GETTIMEOUT:
> -		return put_user(timeout, p);
> -	default:
> -		return -ENOTTY;
> -	}
> -}
> -
> -/*
> - *	ali_open	-	handle open of ali watchdog
> - *	@inode: inode from VFS
> - *	@file: file from VFS
> - *
> - *	Open the ALi watchdog device. Ensure only one person opens it
> - *	at a time. Also start the watchdog running.
> - */
> -
> -static int ali_open(struct inode *inode, struct file *file)
> -{
> -	/* /dev/watchdog can only be opened once */
> -	if (test_and_set_bit(0, &ali_is_open))
> -		return -EBUSY;
> -
> -	/* Activate */
> -	ali_start();
> -	return nonseekable_open(inode, file);
> -}
> -
> -/*
> - *	ali_release	-	close an ALi watchdog
> - *	@inode: inode from VFS
> - *	@file: file from VFS
> - *
> - *	Close the ALi watchdog device. Actual shutdown of the timer
> - *	only occurs if the magic sequence has been set.
> - */
> -
> -static int ali_release(struct inode *inode, struct file *file)
> -{
> -	/*
> -	 *      Shut off the timer.
> -	 */
> -	if (ali_expect_release == 42)
> -		ali_stop();
> -	else {
> -		pr_crit("Unexpected close, not stopping watchdog!\n");
> -		ali_keepalive();
> -	}
> -	clear_bit(0, &ali_is_open);
> -	ali_expect_release = 0;
> -	return 0;
> -}
> -
> -/*
> - *	ali_notify_sys	-	System down notifier
> - *
> - *	Notifier for system down
> - */
> -
> -
> -static int ali_notify_sys(struct notifier_block *this,
> -					unsigned long code, void *unused)
> -{
> -	if (code == SYS_DOWN || code == SYS_HALT)
> -		ali_stop();		/* Turn the WDT off */
> -	return NOTIFY_DONE;
> -}
> -
>   /*
>    *	Data for PCI driver interface
>    *
> @@ -361,23 +173,17 @@ static int __init ali_find_watchdog(void)
>    *	Kernel Interfaces
>    */
>   
> -static const struct file_operations ali_fops = {
> -	.owner		=	THIS_MODULE,
> -	.llseek		=	no_llseek,
> -	.write		=	ali_write,
> -	.unlocked_ioctl =	ali_ioctl,
> -	.open		=	ali_open,
> -	.release	=	ali_release,
> +static struct watchdog_ops alim1535wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = ali_start,
> +	.stop = ali_stop,
> +	.ping = ali_keepalive,
> +	.set_timeout = ali_set_timeout,
>   };
>   
> -static struct miscdevice ali_miscdev = {
> -	.minor =	WATCHDOG_MINOR,
> -	.name =		"watchdog",
> -	.fops =		&ali_fops,
> -};
> -
> -static struct notifier_block ali_notifier = {
> -	.notifier_call =	ali_notify_sys,
> +static struct watchdog_device alim1535wdt_wdd = {
> +	.ops = &alim1535wdt_ops,
> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>   };
>   
>   /*
> @@ -403,30 +209,25 @@ static int __init watchdog_init(void)
>   			timeout);
>   	}
>   
> -	/* Calculate the watchdog's timeout */
> -	ali_settimer(timeout);
> +	watchdog_stop_on_reboot(&alim1535wdt_wdd);
> +
> +	watchdog_init_timeout(&alim1535wdt_wdd, timeout, NULL);
> +
> +	watchdog_set_nowayout(&alim1535wdt_wdd, nowayout);
>   
> -	ret = register_reboot_notifier(&ali_notifier);
> -	if (ret != 0) {
> -		pr_err("cannot register reboot notifier (err=%d)\n", ret);
> -		goto out;
> -	}
> +	ret = watchdog_register_device(&alim1535wdt_wdd);
>   
> -	ret = misc_register(&ali_miscdev);
>   	if (ret != 0) {
> -		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
> -		       WATCHDOG_MINOR, ret);
> -		goto unreg_reboot;
> +		goto reboot_unreg;
>   	}
>   
> -	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
> -		timeout, nowayout);
> +	/* Calculate the watchdog's timeout */
> +	ali_set_timeout(&alim1535wdt_wdd, timeout);
> +
> +	return 0;
>   
> -out:
> +reboot_unreg:
>   	return ret;
> -unreg_reboot:
> -	unregister_reboot_notifier(&ali_notifier);
> -	goto out;
>   }
>   
>   /*
> @@ -437,12 +238,8 @@ unreg_reboot:
>   
>   static void __exit watchdog_exit(void)
>   {
> -	/* Stop the timer before we leave */
> -	ali_stop();
> -
>   	/* Deregister */
> -	misc_deregister(&ali_miscdev);
> -	unregister_reboot_notifier(&ali_notifier);
> +	watchdog_unregister_device(&alim1535wdt_wdd);
>   	pci_dev_put(ali_pci);
>   }
>   
> @@ -451,4 +248,4 @@ module_exit(watchdog_exit);
>   
>   MODULE_AUTHOR("Alan Cox");
>   MODULE_DESCRIPTION("ALi M1535 PMU Watchdog Timer driver");
> -MODULE_LICENSE("GPL");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[Mark Balantzyan]

Take a second look. It is different. And I had to amend the title hence 
the second send to include the subsystem, as you so duly tend to remind 
me. It just so happened that I mistyped and skipped a version by...1.

Mark

On Thu, 1 Aug 2019, Guenter Roeck wrote:

> On 8/1/19 8:26 PM, Mark Balantzyan wrote:
>> This patch rewrites the alim1535_wdt driver to use the watchdog subsystem.
>> By virtue of this, it also fixes a (theoretical) race condition between the
>> formerly arranged ali_timeout_bits and ali_settimer() interoperation.
>> 
>> Signed-off-by: Mark Balantzyan <mbalant3@gmail.com>
>
>
> This is v4. A minute ago I received v1. Earlier today I got v2, which I
> commented on. I didn't see v3 of this patch. I don't see a change log.
> At first glance, I don't see a difference to v2.
>
> Sorry, this is a waste of my time. I won't review or comment further.
>
> Guenter
>
>> 
>> ---
>>   drivers/watchdog/Kconfig        |   1 +
>>   drivers/watchdog/alim1535_wdt.c | 275 +++++---------------------------
>>   2 files changed, 37 insertions(+), 239 deletions(-)
>> 
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9af07fd9..980b0c90 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -853,6 +853,7 @@ config ADVANTECH_WDT
>>     config ALIM1535_WDT
>>   	tristate "ALi M1535 PMU Watchdog Timer"
>> +	select WATCHDOG_CORE
>>   	depends on X86 && PCI
>>   	---help---
>>   	  This is the driver for the hardware watchdog on the ALi M1535 PMU.
>> diff --git a/drivers/watchdog/alim1535_wdt.c 
>> b/drivers/watchdog/alim1535_wdt.c
>> index 60f0c2eb..55648ba8 100644
>> --- a/drivers/watchdog/alim1535_wdt.c
>> +++ b/drivers/watchdog/alim1535_wdt.c
>> @@ -12,26 +12,18 @@
>>   #include <linux/module.h>
>>   #include <linux/moduleparam.h>
>>   #include <linux/types.h>
>> -#include <linux/miscdevice.h>
>>   #include <linux/watchdog.h>
>>   #include <linux/ioport.h>
>> -#include <linux/notifier.h>
>> -#include <linux/reboot.h>
>> -#include <linux/init.h>
>> -#include <linux/fs.h>
>> -#include <linux/pci.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/io.h>
>> +#include <linux/pci.h>
>>     #define WATCHDOG_NAME "ALi_M1535"
>>   #define WATCHDOG_TIMEOUT 60	/* 60 sec default timeout */
>>     /* internal variables */
>> -static unsigned long ali_is_open;
>> -static char ali_expect_release;
>>   static struct pci_dev *ali_pci;
>>   static u32 ali_timeout_bits;		/* stores the computed 
>> timeout */
>> -static DEFINE_SPINLOCK(ali_lock);	/* Guards the hardware */
>>     /* module parameters */
>>   static int timeout = WATCHDOG_TIMEOUT;
>> @@ -53,18 +45,15 @@ MODULE_PARM_DESC(nowayout,
>>    *	configuration set.
>>    */
>>   -static void ali_start(void)
>> +static int ali_start(struct watchdog_device *wdd)
>>   {
>>   	u32 val;
>>   -	spin_lock(&ali_lock);
>> -
>>   	pci_read_config_dword(ali_pci, 0xCC, &val);
>>   	val &= ~0x3F;	/* Mask count */
>>   	val |= (1 << 25) | ali_timeout_bits;
>>   	pci_write_config_dword(ali_pci, 0xCC, val);
>> -
>> -	spin_unlock(&ali_lock);
>> +	return 0;
>>   }
>>     /*
>> @@ -73,18 +62,15 @@ static void ali_start(void)
>>    *	Stop the ALi watchdog countdown
>>    */
>>   -static void ali_stop(void)
>> +static int ali_stop(struct watchdog_device *wdd)
>>   {
>>   	u32 val;
>>   -	spin_lock(&ali_lock);
>> -
>>   	pci_read_config_dword(ali_pci, 0xCC, &val);
>>   	val &= ~0x3F;		/* Mask count to zero (disabled) */
>>   	val &= ~(1 << 25);	/* and for safety mask the reset enable */
>>   	pci_write_config_dword(ali_pci, 0xCC, val);
>> -
>> -	spin_unlock(&ali_lock);
>> +	return 0;
>>   }
>>     /*
>> @@ -93,32 +79,24 @@ static void ali_stop(void)
>>    *	Send a keepalive to the timer (actually we restart the timer).
>>    */
>>   -static void ali_keepalive(void)
>> +static int ali_keepalive(struct watchdog_device *wdd)
>>   {
>> -	ali_start();
>> +	ali_start(wdd);
>> +	return 0;
>>   }
>>     /*
>> - *	ali_settimer	-	compute the timer reload value
>> + *	ali_set_timeout	-	compute the timer reload value
>>    *	@t: time in seconds
>>    *
>>    *	Computes the timeout values needed
>>    */
>>   -static int ali_settimer(int t)
>> +static int ali_set_timeout(struct watchdog_device *wdd, unsigned int t)
>>   {
>> -	if (t < 0)
>> -		return -EINVAL;
>> -	else if (t < 60)
>> -		ali_timeout_bits = t|(1 << 6);
>> -	else if (t < 3600)
>> -		ali_timeout_bits = (t / 60)|(1 << 7);
>> -	else if (t < 18000)
>> -		ali_timeout_bits = (t / 300)|(1 << 6)|(1 << 7);
>> -	else
>> -		return -EINVAL;
>> -
>> -	timeout = t;
>> +	wdd->max_timeout = 60;
>> +	wdd->min_timeout = 1;
>> +	wdd->timeout = t;
>>   	return 0;
>>   }
>>   @@ -126,172 +104,6 @@ static int ali_settimer(int t)
>>    *	/dev/watchdog handling
>>    */
>>   -/*
>> - *	ali_write	-	writes to ALi watchdog
>> - *	@file: file from VFS
>> - *	@data: user address of data
>> - *	@len: length of data
>> - *	@ppos: pointer to the file offset
>> - *
>> - *	Handle a write to the ALi watchdog. Writing to the file pings
>> - *	the watchdog and resets it. Writing the magic 'V' sequence allows
>> - *	the next close to turn off the watchdog.
>> - */
>> -
>> -static ssize_t ali_write(struct file *file, const char __user *data,
>> -						size_t len, loff_t *ppos)
>> -{
>> -	/* See if we got the magic character 'V' and reload the timer */
>> -	if (len) {
>> -		if (!nowayout) {
>> -			size_t i;
>> -
>> -			/* note: just in case someone wrote the
>> -			   magic character five months ago... */
>> -			ali_expect_release = 0;
>> -
>> -			/* scan to see whether or not we got
>> -			   the magic character */
>> -			for (i = 0; i != len; i++) {
>> -				char c;
>> -				if (get_user(c, data + i))
>> -					return -EFAULT;
>> -				if (c == 'V')
>> -					ali_expect_release = 42;
>> -			}
>> -		}
>> -
>> -		/* someone wrote to us, we should reload the timer */
>> -		ali_start();
>> -	}
>> -	return len;
>> -}
>> -
>> -/*
>> - *	ali_ioctl	-	handle watchdog ioctls
>> - *	@file: VFS file pointer
>> - *	@cmd: ioctl number
>> - *	@arg: arguments to the ioctl
>> - *
>> - *	Handle the watchdog ioctls supported by the ALi driver. Really
>> - *	we want an extension to enable irq ack monitoring and the like
>> - */
>> -
>> -static long ali_ioctl(struct file *file, unsigned int cmd, unsigned long 
>> arg)
>> -{
>> -	void __user *argp = (void __user *)arg;
>> -	int __user *p = argp;
>> -	static const struct watchdog_info ident = {
>> -		.options =		WDIOF_KEEPALIVEPING |
>> -					WDIOF_SETTIMEOUT |
>> -					WDIOF_MAGICCLOSE,
>> -		.firmware_version =	0,
>> -		.identity =		"ALi M1535 WatchDog Timer",
>> -	};
>> -
>> -	switch (cmd) {
>> -	case WDIOC_GETSUPPORT:
>> -		return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 
>> 0;
>> -
>> -	case WDIOC_GETSTATUS:
>> -	case WDIOC_GETBOOTSTATUS:
>> -		return put_user(0, p);
>> -	case WDIOC_SETOPTIONS:
>> -	{
>> -		int new_options, retval = -EINVAL;
>> -
>> -		if (get_user(new_options, p))
>> -			return -EFAULT;
>> -		if (new_options & WDIOS_DISABLECARD) {
>> -			ali_stop();
>> -			retval = 0;
>> -		}
>> -		if (new_options & WDIOS_ENABLECARD) {
>> -			ali_start();
>> -			retval = 0;
>> -		}
>> -		return retval;
>> -	}
>> -	case WDIOC_KEEPALIVE:
>> -		ali_keepalive();
>> -		return 0;
>> -	case WDIOC_SETTIMEOUT:
>> -	{
>> -		int new_timeout;
>> -		if (get_user(new_timeout, p))
>> -			return -EFAULT;
>> -		if (ali_settimer(new_timeout))
>> -			return -EINVAL;
>> -		ali_keepalive();
>> -	}
>> -		/* fall through */
>> -	case WDIOC_GETTIMEOUT:
>> -		return put_user(timeout, p);
>> -	default:
>> -		return -ENOTTY;
>> -	}
>> -}
>> -
>> -/*
>> - *	ali_open	-	handle open of ali watchdog
>> - *	@inode: inode from VFS
>> - *	@file: file from VFS
>> - *
>> - *	Open the ALi watchdog device. Ensure only one person opens it
>> - *	at a time. Also start the watchdog running.
>> - */
>> -
>> -static int ali_open(struct inode *inode, struct file *file)
>> -{
>> -	/* /dev/watchdog can only be opened once */
>> -	if (test_and_set_bit(0, &ali_is_open))
>> -		return -EBUSY;
>> -
>> -	/* Activate */
>> -	ali_start();
>> -	return nonseekable_open(inode, file);
>> -}
>> -
>> -/*
>> - *	ali_release	-	close an ALi watchdog
>> - *	@inode: inode from VFS
>> - *	@file: file from VFS
>> - *
>> - *	Close the ALi watchdog device. Actual shutdown of the timer
>> - *	only occurs if the magic sequence has been set.
>> - */
>> -
>> -static int ali_release(struct inode *inode, struct file *file)
>> -{
>> -	/*
>> -	 *      Shut off the timer.
>> -	 */
>> -	if (ali_expect_release == 42)
>> -		ali_stop();
>> -	else {
>> -		pr_crit("Unexpected close, not stopping watchdog!\n");
>> -		ali_keepalive();
>> -	}
>> -	clear_bit(0, &ali_is_open);
>> -	ali_expect_release = 0;
>> -	return 0;
>> -}
>> -
>> -/*
>> - *	ali_notify_sys	-	System down notifier
>> - *
>> - *	Notifier for system down
>> - */
>> -
>> -
>> -static int ali_notify_sys(struct notifier_block *this,
>> -					unsigned long code, void *unused)
>> -{
>> -	if (code == SYS_DOWN || code == SYS_HALT)
>> -		ali_stop();		/* Turn the WDT off */
>> -	return NOTIFY_DONE;
>> -}
>> -
>>   /*
>>    *	Data for PCI driver interface
>>    *
>> @@ -361,23 +173,17 @@ static int __init ali_find_watchdog(void)
>>    *	Kernel Interfaces
>>    */
>>   -static const struct file_operations ali_fops = {
>> -	.owner		=	THIS_MODULE,
>> -	.llseek		=	no_llseek,
>> -	.write		=	ali_write,
>> -	.unlocked_ioctl =	ali_ioctl,
>> -	.open		=	ali_open,
>> -	.release	=	ali_release,
>> +static struct watchdog_ops alim1535wdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = ali_start,
>> +	.stop = ali_stop,
>> +	.ping = ali_keepalive,
>> +	.set_timeout = ali_set_timeout,
>>   };
>>   -static struct miscdevice ali_miscdev = {
>> -	.minor =	WATCHDOG_MINOR,
>> -	.name =		"watchdog",
>> -	.fops =		&ali_fops,
>> -};
>> -
>> -static struct notifier_block ali_notifier = {
>> -	.notifier_call =	ali_notify_sys,
>> +static struct watchdog_device alim1535wdt_wdd = {
>> +	.ops = &alim1535wdt_ops,
>> +	.status = WATCHDOG_NOWAYOUT_INIT_STATUS,
>>   };
>>     /*
>> @@ -403,30 +209,25 @@ static int __init watchdog_init(void)
>>   			timeout);
>>   	}
>>   -	/* Calculate the watchdog's timeout */
>> -	ali_settimer(timeout);
>> +	watchdog_stop_on_reboot(&alim1535wdt_wdd);
>> +
>> +	watchdog_init_timeout(&alim1535wdt_wdd, timeout, NULL);
>> +
>> +	watchdog_set_nowayout(&alim1535wdt_wdd, nowayout);
>>   -	ret = register_reboot_notifier(&ali_notifier);
>> -	if (ret != 0) {
>> -		pr_err("cannot register reboot notifier (err=%d)\n", ret);
>> -		goto out;
>> -	}
>> +	ret = watchdog_register_device(&alim1535wdt_wdd);
>>   -	ret = misc_register(&ali_miscdev);
>>   	if (ret != 0) {
>> -		pr_err("cannot register miscdev on minor=%d (err=%d)\n",
>> -		       WATCHDOG_MINOR, ret);
>> -		goto unreg_reboot;
>> +		goto reboot_unreg;
>>   	}
>>   -	pr_info("initialized. timeout=%d sec (nowayout=%d)\n",
>> -		timeout, nowayout);
>> +	/* Calculate the watchdog's timeout */
>> +	ali_set_timeout(&alim1535wdt_wdd, timeout);
>> +
>> +	return 0;
>>   -out:
>> +reboot_unreg:
>>   	return ret;
>> -unreg_reboot:
>> -	unregister_reboot_notifier(&ali_notifier);
>> -	goto out;
>>   }
>>     /*
>> @@ -437,12 +238,8 @@ unreg_reboot:
>>     static void __exit watchdog_exit(void)
>>   {
>> -	/* Stop the timer before we leave */
>> -	ali_stop();
>> -
>>   	/* Deregister */
>> -	misc_deregister(&ali_miscdev);
>> -	unregister_reboot_notifier(&ali_notifier);
>> +	watchdog_unregister_device(&alim1535wdt_wdd);
>>   	pci_dev_put(ali_pci);
>>   }
>>   @@ -451,4 +248,4 @@ module_exit(watchdog_exit);
>>     MODULE_AUTHOR("Alan Cox");
>>   MODULE_DESCRIPTION("ALi M1535 PMU Watchdog Timer driver");
>> -MODULE_LICENSE("GPL");
>> +MODULE_LICENSE("GPL");
>> 
>
>

Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[Mark Balantzyan]

There was some sort of filesystem error that cause the wrong file to be 
saved. Hence, yes, possibly there was no difference betwee the v2 and the 
'v4' (which should have been the 'v3'). Since the last one, though 
unchanged from v2, was a changelog-less 'v3', THIS is an actual 'v4'.

Thank you,
Mark

Re: [PATCH v4] watchdog: alim1535: Rewriting of alim1535 to use watchdog subsystem

[Mark Balantzyan]

Please see: https://lkml.org/lkml/2019/8/2/6

Thank you.