[PATCH 0/2] xen_wdt: use the watchdog subsystem

[PATCH 0/2] xen_wdt: use the watchdog subsystem

[Radu Rendec]

Hi Guenter,

I just figured out we have a similar use case (of two watchdogs at the
same time) that involves the xen_wdt driver. I thought "same problem,
same fix", so I decided to port xen_wdt to use the watchdog subsystem.

This patch series contains similar changes to what I have recently done
for i6300esb. The changes are simpler though, because the Xen hypervisor
cannot expose multiple watchdogs to the guest, and so the driver doesn't
need to allocate watchdog structures dynamically.

I tried to avoid all the problems I had with the i6300esb patches and
hopefully these new patches don't have as many issues (or none at all).

Thanks,
Radu

Radu Rendec (2):
  watchdog: xen_wdt: use the watchdog subsystem
  watchdog: xen_wdt: remove info message and version number

 drivers/watchdog/xen_wdt.c | 208 +++++++++------------------------------------
 1 file changed, 38 insertions(+), 170 deletions(-)

-- 
2.13.6

[PATCH 1/2] watchdog: xen_wdt: use the watchdog subsystem

[Radu Rendec]

Change the xen_wdt driver to use the watchdog subsystem instead of
registering and manipulating the char device directly through the misc
API. This is mainly getting rid of the "write" and "ioctl" methods and
part of the watchdog control logic (which are all implemented by the
watchdog subsystem).

Even though the watchdog subsystem supports registering and handling
multiple watchdog devices at the same time, the xen_wdt driver has an
inherent limitation of only one device due to the way the Xen hypervisor
exposes watchdog functionality. However, the driver can now coexist with
other watchdog devices (supported by different drivers).

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/xen_wdt.c | 204 +++++++++------------------------------------
 1 file changed, 38 insertions(+), 166 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index cf0e650c2015..8bea946dfdc7 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -33,13 +33,11 @@
 #include <xen/interface/sched.h>
 
 static struct platform_device *platform_device;
-static DEFINE_SPINLOCK(wdt_lock);
 static struct sched_watchdog wdt;
 static __kernel_time_t wdt_expires;
-static bool is_active, expect_release;
 
 #define WATCHDOG_TIMEOUT 60 /* in seconds */
-static unsigned int timeout = WATCHDOG_TIMEOUT;
+static unsigned int timeout;
 module_param(timeout, uint, S_IRUGO);
 MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds "
 	"(default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
@@ -49,20 +47,18 @@ module_param(nowayout, bool, S_IRUGO);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static inline __kernel_time_t set_timeout(void)
+static inline __kernel_time_t set_timeout(struct watchdog_device *wdd)
 {
-	wdt.timeout = timeout;
-	return ktime_to_timespec(ktime_get()).tv_sec + timeout;
+	wdt.timeout = wdd->timeout;
+	return ktime_to_timespec(ktime_get()).tv_sec + wdd->timeout;
 }
 
-static int xen_wdt_start(void)
+static int xen_wdt_start(struct watchdog_device *wdd)
 {
 	__kernel_time_t expires;
 	int err;
 
-	spin_lock(&wdt_lock);
-
-	expires = set_timeout();
+	expires = set_timeout(wdd);
 	if (!wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	else
@@ -74,36 +70,28 @@ static int xen_wdt_start(void)
 	} else
 		BUG_ON(!err);
 
-	spin_unlock(&wdt_lock);
-
 	return err;
 }
 
-static int xen_wdt_stop(void)
+static int xen_wdt_stop(struct watchdog_device *wdd)
 {
 	int err = 0;
 
-	spin_lock(&wdt_lock);
-
 	wdt.timeout = 0;
 	if (wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	if (!err)
 		wdt.id = 0;
 
-	spin_unlock(&wdt_lock);
-
 	return err;
 }
 
-static int xen_wdt_kick(void)
+static int xen_wdt_kick(struct watchdog_device *wdd)
 {
 	__kernel_time_t expires;
 	int err;
 
-	spin_lock(&wdt_lock);
-
-	expires = set_timeout();
+	expires = set_timeout(wdd);
 	if (wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	else
@@ -111,137 +99,31 @@ static int xen_wdt_kick(void)
 	if (!err)
 		wdt_expires = expires;
 
-	spin_unlock(&wdt_lock);
-
-	return err;
-}
-
-static int xen_wdt_open(struct inode *inode, struct file *file)
-{
-	int err;
-
-	/* /dev/watchdog can only be opened once */
-	if (xchg(&is_active, true))
-		return -EBUSY;
-
-	err = xen_wdt_start();
-	if (err == -EBUSY)
-		err = xen_wdt_kick();
-	return err ?: nonseekable_open(inode, file);
-}
-
-static int xen_wdt_release(struct inode *inode, struct file *file)
-{
-	int err = 0;
-
-	if (expect_release)
-		err = xen_wdt_stop();
-	else {
-		pr_crit("unexpected close, not stopping watchdog!\n");
-		xen_wdt_kick();
-	}
-	is_active = err;
-	expect_release = false;
 	return err;
 }
 
-static ssize_t xen_wdt_write(struct file *file, const char __user *data,
-			     size_t len, loff_t *ppos)
+static unsigned int xen_wdt_get_timeleft(struct watchdog_device *wdd)
 {
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* in case it was set long ago */
-			expect_release = false;
-
-			/* 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')
-					expect_release = true;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		xen_wdt_kick();
-	}
-	return len;
+	return wdt_expires - ktime_to_timespec(ktime_get()).tv_sec;
 }
 
-static long xen_wdt_ioctl(struct file *file, unsigned int cmd,
-			  unsigned long arg)
-{
-	int new_options, retval = -EINVAL;
-	int new_timeout;
-	int __user *argp = (void __user *)arg;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		DRV_NAME,
-	};
-
-	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, argp);
-
-	case WDIOC_SETOPTIONS:
-		if (get_user(new_options, argp))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD)
-			retval = xen_wdt_stop();
-		if (new_options & WDIOS_ENABLECARD) {
-			retval = xen_wdt_start();
-			if (retval == -EBUSY)
-				retval = xen_wdt_kick();
-		}
-		return retval;
-
-	case WDIOC_KEEPALIVE:
-		xen_wdt_kick();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_timeout, argp))
-			return -EFAULT;
-		if (!new_timeout)
-			return -EINVAL;
-		timeout = new_timeout;
-		xen_wdt_kick();
-		/* fall through */
-	case WDIOC_GETTIMEOUT:
-		return put_user(timeout, argp);
-
-	case WDIOC_GETTIMELEFT:
-		retval = wdt_expires - ktime_to_timespec(ktime_get()).tv_sec;
-		return put_user(retval, argp);
-	}
-
-	return -ENOTTY;
-}
+static struct watchdog_info xen_wdt_info = {
+	.identity = DRV_NAME,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
 
-static const struct file_operations xen_wdt_fops = {
-	.owner =		THIS_MODULE,
-	.llseek =		no_llseek,
-	.write =		xen_wdt_write,
-	.unlocked_ioctl =	xen_wdt_ioctl,
-	.open =			xen_wdt_open,
-	.release =		xen_wdt_release,
+static const struct watchdog_ops xen_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = xen_wdt_start,
+	.stop = xen_wdt_stop,
+	.ping = xen_wdt_kick,
+	.get_timeleft = xen_wdt_get_timeleft,
 };
 
-static struct miscdevice xen_wdt_miscdev = {
-	.minor =	WATCHDOG_MINOR,
-	.name =		"watchdog",
-	.fops =		&xen_wdt_fops,
+static struct watchdog_device xen_wdt_dev = {
+	.info = &xen_wdt_info,
+	.ops = &xen_wdt_ops,
+	.timeout = WATCHDOG_TIMEOUT,
 };
 
 static int xen_wdt_probe(struct platform_device *dev)
@@ -251,20 +133,21 @@ static int xen_wdt_probe(struct platform_device *dev)
 
 	switch (ret) {
 	case -EINVAL:
-		if (!timeout) {
-			timeout = WATCHDOG_TIMEOUT;
-			pr_info("timeout value invalid, using %d\n", timeout);
-		}
-
-		ret = misc_register(&xen_wdt_miscdev);
+		if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
+			pr_info("timeout value invalid, using %d\n",
+				xen_wdt_dev.timeout);
+		watchdog_set_nowayout(&xen_wdt_dev, nowayout);
+		watchdog_stop_on_reboot(&xen_wdt_dev);
+		watchdog_stop_on_unregister(&xen_wdt_dev);
+
+		ret = watchdog_register_device(&xen_wdt_dev);
 		if (ret) {
-			pr_err("cannot register miscdev on minor=%d (%d)\n",
-			       WATCHDOG_MINOR, ret);
+			pr_err("cannot register watchdog device (%d)\n", ret);
 			break;
 		}
 
 		pr_info("initialized (timeout=%ds, nowayout=%d)\n",
-			timeout, nowayout);
+			xen_wdt_dev.timeout, nowayout);
 		break;
 
 	case -ENOSYS:
@@ -282,24 +165,14 @@ static int xen_wdt_probe(struct platform_device *dev)
 
 static int xen_wdt_remove(struct platform_device *dev)
 {
-	/* Stop the timer before we leave */
-	if (!nowayout)
-		xen_wdt_stop();
-
-	misc_deregister(&xen_wdt_miscdev);
-
+	watchdog_unregister_device(&xen_wdt_dev);
 	return 0;
 }
 
-static void xen_wdt_shutdown(struct platform_device *dev)
-{
-	xen_wdt_stop();
-}
-
 static int xen_wdt_suspend(struct platform_device *dev, pm_message_t state)
 {
 	typeof(wdt.id) id = wdt.id;
-	int rc = xen_wdt_stop();
+	int rc = xen_wdt_stop(&xen_wdt_dev);
 
 	wdt.id = id;
 	return rc;
@@ -310,13 +183,12 @@ static int xen_wdt_resume(struct platform_device *dev)
 	if (!wdt.id)
 		return 0;
 	wdt.id = 0;
-	return xen_wdt_start();
+	return xen_wdt_start(&xen_wdt_dev);
 }
 
 static struct platform_driver xen_wdt_driver = {
 	.probe          = xen_wdt_probe,
 	.remove         = xen_wdt_remove,
-	.shutdown       = xen_wdt_shutdown,
 	.suspend        = xen_wdt_suspend,
 	.resume         = xen_wdt_resume,
 	.driver         = {
-- 
2.13.6

[PATCH 2/2] watchdog: xen_wdt: remove info message and version number

[Radu Rendec]

The initial info message (early in the xen_wdt_init_module() function)
is not very useful and we already have a message on successful probe. If
the probe fails, additional messages are printed anyway.

The version number serves no useful purpose and it ran out of favor
upstream anyway.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/xen_wdt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index 8bea946dfdc7..e1aeff0ea6eb 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -12,7 +12,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #define DRV_NAME	"wdt"
-#define DRV_VERSION	"0.01"
 
 #include <linux/bug.h>
 #include <linux/errno.h>
@@ -203,8 +202,6 @@ static int __init xen_wdt_init_module(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	pr_info("Xen WatchDog Timer Driver v%s\n", DRV_VERSION);
-
 	err = platform_driver_register(&xen_wdt_driver);
 	if (err)
 		return err;
@@ -231,5 +228,4 @@ module_exit(xen_wdt_cleanup_module);
 
 MODULE_AUTHOR("Jan Beulich <jbeulich@novell.com>");
 MODULE_DESCRIPTION("Xen WatchDog Timer Driver");
-MODULE_VERSION(DRV_VERSION);
 MODULE_LICENSE("GPL");
-- 
2.13.6

Re: [1/2] watchdog: xen_wdt: use the watchdog subsystem

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:31:52PM +0000, Radu Rendec wrote:
> Change the xen_wdt driver to use the watchdog subsystem instead of
> registering and manipulating the char device directly through the misc
> API. This is mainly getting rid of the "write" and "ioctl" methods and
> part of the watchdog control logic (which are all implemented by the
> watchdog subsystem).
> 
> Even though the watchdog subsystem supports registering and handling
> multiple watchdog devices at the same time, the xen_wdt driver has an
> inherent limitation of only one device due to the way the Xen hypervisor
> exposes watchdog functionality. However, the driver can now coexist with
> other watchdog devices (supported by different drivers).
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

Can you rebase the patch on top of Arnd's patch ("watchdog: xen: use time64_t
for timeouts") ?

> ---
>  drivers/watchdog/xen_wdt.c | 204 +++++++++------------------------------------
>  1 file changed, 38 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> index cf0e650c2015..8bea946dfdc7 100644
> --- a/drivers/watchdog/xen_wdt.c
> +++ b/drivers/watchdog/xen_wdt.c
> @@ -33,13 +33,11 @@
>  #include <xen/interface/sched.h>
>  
#include <linux/miscdevice.h>
#include <linux/spinlock.h>
#include <linux/uaccess.h>

should now be unnecessary.

>  static struct platform_device *platform_device;
> -static DEFINE_SPINLOCK(wdt_lock);
>  static struct sched_watchdog wdt;
>  static __kernel_time_t wdt_expires;
> -static bool is_active, expect_release;
>  
>  #define WATCHDOG_TIMEOUT 60 /* in seconds */
> -static unsigned int timeout = WATCHDOG_TIMEOUT;
> +static unsigned int timeout;
>  module_param(timeout, uint, S_IRUGO);
>  MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds "
>  	"(default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
> @@ -49,20 +47,18 @@ module_param(nowayout, bool, S_IRUGO);
>  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
>  	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>  
> -static inline __kernel_time_t set_timeout(void)
> +static inline __kernel_time_t set_timeout(struct watchdog_device *wdd)
>  {
> -	wdt.timeout = timeout;
> -	return ktime_to_timespec(ktime_get()).tv_sec + timeout;
> +	wdt.timeout = wdd->timeout;
> +	return ktime_to_timespec(ktime_get()).tv_sec + wdd->timeout;
>  }
>  
> -static int xen_wdt_start(void)
> +static int xen_wdt_start(struct watchdog_device *wdd)
>  {
>  	__kernel_time_t expires;
>  	int err;
>  
> -	spin_lock(&wdt_lock);
> -
> -	expires = set_timeout();
> +	expires = set_timeout(wdd);
>  	if (!wdt.id)
>  		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
>  	else
> @@ -74,36 +70,28 @@ static int xen_wdt_start(void)
>  	} else
>  		BUG_ON(!err);
>  
> -	spin_unlock(&wdt_lock);
> -
>  	return err;
>  }
>  
> -static int xen_wdt_stop(void)
> +static int xen_wdt_stop(struct watchdog_device *wdd)
>  {
>  	int err = 0;
>  
> -	spin_lock(&wdt_lock);
> -
>  	wdt.timeout = 0;
>  	if (wdt.id)
>  		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
>  	if (!err)
>  		wdt.id = 0;
>  
> -	spin_unlock(&wdt_lock);
> -
>  	return err;
>  }
>  
> -static int xen_wdt_kick(void)
> +static int xen_wdt_kick(struct watchdog_device *wdd)
>  {
>  	__kernel_time_t expires;
>  	int err;
>  
> -	spin_lock(&wdt_lock);
> -
> -	expires = set_timeout();
> +	expires = set_timeout(wdd);
>  	if (wdt.id)
>  		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
>  	else
> @@ -111,137 +99,31 @@ static int xen_wdt_kick(void)
>  	if (!err)
>  		wdt_expires = expires;
>  
> -	spin_unlock(&wdt_lock);
> -
> -	return err;
> -}
> -
> -static int xen_wdt_open(struct inode *inode, struct file *file)
> -{
> -	int err;
> -
> -	/* /dev/watchdog can only be opened once */
> -	if (xchg(&is_active, true))
> -		return -EBUSY;
> -
> -	err = xen_wdt_start();
> -	if (err == -EBUSY)
> -		err = xen_wdt_kick();
> -	return err ?: nonseekable_open(inode, file);
> -}
> -
> -static int xen_wdt_release(struct inode *inode, struct file *file)
> -{
> -	int err = 0;
> -
> -	if (expect_release)
> -		err = xen_wdt_stop();
> -	else {
> -		pr_crit("unexpected close, not stopping watchdog!\n");
> -		xen_wdt_kick();
> -	}
> -	is_active = err;
> -	expect_release = false;
>  	return err;
>  }
>  
> -static ssize_t xen_wdt_write(struct file *file, const char __user *data,
> -			     size_t len, loff_t *ppos)
> +static unsigned int xen_wdt_get_timeleft(struct watchdog_device *wdd)
>  {
> -	/* See if we got the magic character 'V' and reload the timer */
> -	if (len) {
> -		if (!nowayout) {
> -			size_t i;
> -
> -			/* in case it was set long ago */
> -			expect_release = false;
> -
> -			/* 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')
> -					expect_release = true;
> -			}
> -		}
> -
> -		/* someone wrote to us, we should reload the timer */
> -		xen_wdt_kick();
> -	}
> -	return len;
> +	return wdt_expires - ktime_to_timespec(ktime_get()).tv_sec;
>  }
>  
> -static long xen_wdt_ioctl(struct file *file, unsigned int cmd,
> -			  unsigned long arg)
> -{
> -	int new_options, retval = -EINVAL;
> -	int new_timeout;
> -	int __user *argp = (void __user *)arg;
> -	static const struct watchdog_info ident = {
> -		.options =		WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
> -		.firmware_version =	0,
> -		.identity =		DRV_NAME,
> -	};
> -
> -	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, argp);
> -
> -	case WDIOC_SETOPTIONS:
> -		if (get_user(new_options, argp))
> -			return -EFAULT;
> -
> -		if (new_options & WDIOS_DISABLECARD)
> -			retval = xen_wdt_stop();
> -		if (new_options & WDIOS_ENABLECARD) {
> -			retval = xen_wdt_start();
> -			if (retval == -EBUSY)
> -				retval = xen_wdt_kick();
> -		}
> -		return retval;
> -
> -	case WDIOC_KEEPALIVE:
> -		xen_wdt_kick();
> -		return 0;
> -
> -	case WDIOC_SETTIMEOUT:
> -		if (get_user(new_timeout, argp))
> -			return -EFAULT;
> -		if (!new_timeout)
> -			return -EINVAL;
> -		timeout = new_timeout;
> -		xen_wdt_kick();
> -		/* fall through */
> -	case WDIOC_GETTIMEOUT:
> -		return put_user(timeout, argp);
> -
> -	case WDIOC_GETTIMELEFT:
> -		retval = wdt_expires - ktime_to_timespec(ktime_get()).tv_sec;
> -		return put_user(retval, argp);
> -	}
> -
> -	return -ENOTTY;
> -}
> +static struct watchdog_info xen_wdt_info = {
> +	.identity = DRV_NAME,
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> +};
>  
> -static const struct file_operations xen_wdt_fops = {
> -	.owner =		THIS_MODULE,
> -	.llseek =		no_llseek,
> -	.write =		xen_wdt_write,
> -	.unlocked_ioctl =	xen_wdt_ioctl,
> -	.open =			xen_wdt_open,
> -	.release =		xen_wdt_release,
> +static const struct watchdog_ops xen_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = xen_wdt_start,
> +	.stop = xen_wdt_stop,
> +	.ping = xen_wdt_kick,
> +	.get_timeleft = xen_wdt_get_timeleft,
>  };
>  
> -static struct miscdevice xen_wdt_miscdev = {
> -	.minor =	WATCHDOG_MINOR,
> -	.name =		"watchdog",
> -	.fops =		&xen_wdt_fops,
> +static struct watchdog_device xen_wdt_dev = {
> +	.info = &xen_wdt_info,
> +	.ops = &xen_wdt_ops,
> +	.timeout = WATCHDOG_TIMEOUT,
>  };
>  
>  static int xen_wdt_probe(struct platform_device *dev)
> @@ -251,20 +133,21 @@ static int xen_wdt_probe(struct platform_device *dev)
>  
>  	switch (ret) {
>  	case -EINVAL:
> -		if (!timeout) {
> -			timeout = WATCHDOG_TIMEOUT;
> -			pr_info("timeout value invalid, using %d\n", timeout);
> -		}
> -
> -		ret = misc_register(&xen_wdt_miscdev);
> +		if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
> +			pr_info("timeout value invalid, using %d\n",
> +				xen_wdt_dev.timeout);
> +		watchdog_set_nowayout(&xen_wdt_dev, nowayout);
> +		watchdog_stop_on_reboot(&xen_wdt_dev);
> +		watchdog_stop_on_unregister(&xen_wdt_dev);
> +
> +		ret = watchdog_register_device(&xen_wdt_dev);

Please consider using devm_watchdog_register_device() and drop the remove
function entirely.

>  		if (ret) {
> -			pr_err("cannot register miscdev on minor=%d (%d)\n",
> -			       WATCHDOG_MINOR, ret);
> +			pr_err("cannot register watchdog device (%d)\n", ret);
>  			break;
>  		}
>  
>  		pr_info("initialized (timeout=%ds, nowayout=%d)\n",
> -			timeout, nowayout);
> +			xen_wdt_dev.timeout, nowayout);

Maybe convert messages to dev_err() and dev_info().

>  		break;
>  
>  	case -ENOSYS:
> @@ -282,24 +165,14 @@ static int xen_wdt_probe(struct platform_device *dev)
>  

The probe function is quite odd. It may return 0 (no error) but not
initialize the watchdog. It might be better to reorganize the code
along the line of

	int ret = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wd);

	if (ret != -EINVAL) {
		/* pick your preferred error message */
		dev_err(&pdev->dev, "Hypervisor error (%d)\n", ret);
		return -ENODEV;
	}

	...

>  static int xen_wdt_remove(struct platform_device *dev)
>  {
> -	/* Stop the timer before we leave */
> -	if (!nowayout)
> -		xen_wdt_stop();
> -
> -	misc_deregister(&xen_wdt_miscdev);
> -
> +	watchdog_unregister_device(&xen_wdt_dev);
>  	return 0;
>  }
>  
> -static void xen_wdt_shutdown(struct platform_device *dev)
> -{
> -	xen_wdt_stop();
> -}
> -
>  static int xen_wdt_suspend(struct platform_device *dev, pm_message_t state)
>  {
>  	typeof(wdt.id) id = wdt.id;
> -	int rc = xen_wdt_stop();
> +	int rc = xen_wdt_stop(&xen_wdt_dev);
>  
>  	wdt.id = id;
>  	return rc;
> @@ -310,13 +183,12 @@ static int xen_wdt_resume(struct platform_device *dev)
>  	if (!wdt.id)
>  		return 0;
>  	wdt.id = 0;
> -	return xen_wdt_start();
> +	return xen_wdt_start(&xen_wdt_dev);
>  }
>  
>  static struct platform_driver xen_wdt_driver = {
>  	.probe          = xen_wdt_probe,
>  	.remove         = xen_wdt_remove,
> -	.shutdown       = xen_wdt_shutdown,
>  	.suspend        = xen_wdt_suspend,
>  	.resume         = xen_wdt_resume,
>  	.driver         = {

For module initialization: Any chance to use module_platform_driver_probe() ?

Re: [2/2] watchdog: xen_wdt: remove info message and version number

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:31:53PM +0000, Radu Rendec wrote:
> The initial info message (early in the xen_wdt_init_module() function)
> is not very useful and we already have a message on successful probe. If
> the probe fails, additional messages are printed anyway.
> 
> The version number serves no useful purpose and it ran out of favor
> upstream anyway.
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>
> ---
>  drivers/watchdog/xen_wdt.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> index 8bea946dfdc7..e1aeff0ea6eb 100644
> --- a/drivers/watchdog/xen_wdt.c
> +++ b/drivers/watchdog/xen_wdt.c
> @@ -12,7 +12,6 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #define DRV_NAME	"wdt"
> -#define DRV_VERSION	"0.01"
>  
>  #include <linux/bug.h>
>  #include <linux/errno.h>
> @@ -203,8 +202,6 @@ static int __init xen_wdt_init_module(void)
>  	if (!xen_domain())
>  		return -ENODEV;
>  
> -	pr_info("Xen WatchDog Timer Driver v%s\n", DRV_VERSION);
> -
>  	err = platform_driver_register(&xen_wdt_driver);
>  	if (err)
>  		return err;
> @@ -231,5 +228,4 @@ module_exit(xen_wdt_cleanup_module);
>  
As I mentioned in the other patch, any chance to use
module_platform_driver_probe() ? The

	if (!xen_domain())
		return -ENODEV;

could be moved there.

Thanks,
Guenter

>  MODULE_AUTHOR("Jan Beulich <jbeulich@novell.com>");
>  MODULE_DESCRIPTION("Xen WatchDog Timer Driver");
> -MODULE_VERSION(DRV_VERSION);
>  MODULE_LICENSE("GPL");

Re: [2/2] watchdog: xen_wdt: remove info message and version number

[Radu Rendec]

On Sun, 2017-11-05 at 07:39 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:31:53PM +0000, Radu Rendec wrote:
> > The initial info message (early in the xen_wdt_init_module() function)
> > is not very useful and we already have a message on successful probe. If
> > the probe fails, additional messages are printed anyway.
> > 
> > The version number serves no useful purpose and it ran out of favor
> > upstream anyway.
> > 
> 
> As I mentioned in the other patch, any chance to use
> module_platform_driver_probe() ? The
> 
> 	if (!xen_domain())
> 		return -ENODEV;
> 
> could be moved there.

I see your point, but I think module_platform_driver_probe() can't be
used here. The problem is that there's nothing else to create the
platform device, so if xen_wdt doesn't create it on its own, there will
be no (matching) platform device to be probed by the driver that
xen_wdt registers.

I looked at module_platform_driver_probe() and it's a convenience macro
that ultimately calls __platform_driver_probe(). The latter is just
__platform_driver_register() with some additional tricks around the
probe function.

The current version works because platform_device_register_simple()
(called by the module init function) conveniently allocates and
registers a matching platform device.

Looking at platform_device.h, I don't see any other macros that we
could use instead.

I already fixed the other issues in the previous patch so, unless you
think init/exit can be simplified in a different way, I am ready to
resend the (updated) patches.

Thanks,
Radu

Re: [2/2] watchdog: xen_wdt: remove info message and version number

[Guenter Roeck]

On Mon, Nov 06, 2017 at 05:39:03PM +0000, Radu Rendec wrote:
> On Sun, 2017-11-05 at 07:39 -0800, Guenter Roeck wrote:
> > On Fri, Nov 03, 2017 at 03:31:53PM +0000, Radu Rendec wrote:
> > > The initial info message (early in the xen_wdt_init_module() function)
> > > is not very useful and we already have a message on successful probe. If
> > > the probe fails, additional messages are printed anyway.
> > > 
> > > The version number serves no useful purpose and it ran out of favor
> > > upstream anyway.
> > > 
> > 
> > As I mentioned in the other patch, any chance to use
> > module_platform_driver_probe() ? The
> > 
> > 	if (!xen_domain())
> > 		return -ENODEV;
> > 
> > could be moved there.
> 
> I see your point, but I think module_platform_driver_probe() can't be
> used here. The problem is that there's nothing else to create the
> platform device, so if xen_wdt doesn't create it on its own, there will
> be no (matching) platform device to be probed by the driver that
> xen_wdt registers.
> 
> I looked at module_platform_driver_probe() and it's a convenience macro
> that ultimately calls __platform_driver_probe(). The latter is just
> __platform_driver_register() with some additional tricks around the
> probe function.
> 
> The current version works because platform_device_register_simple()
> (called by the module init function) conveniently allocates and
> registers a matching platform device.
> 
> Looking at platform_device.h, I don't see any other macros that we
> could use instead.
> 
> I already fixed the other issues in the previous patch so, unless you
> think init/exit can be simplified in a different way, I am ready to
> resend the (updated) patches.
> 
Ok; thanks for having a look.

Guenter

[PATCH v2 0/2] xen_wdt: use the watchdog subsystem

[Radu Rendec]

On Mon, 2017-11-06 at 10:04 -0800, Guenter Roeck wrote:
> On Mon, Nov 06, 2017 at 05:39:03PM +0000, Radu Rendec wrote:
> > I see your point, but I think module_platform_driver_probe() can't be
> > used here. The problem is that there's nothing else to create the
> > platform device, so if xen_wdt doesn't create it on its own, there will
> > be no (matching) platform device to be probed by the driver that
> > xen_wdt registers.
[snip]
> > I already fixed the other issues in the previous patch so, unless you
> > think init/exit can be simplified in a different way, I am ready to
> > resend the (updated) patches.
> >
>
> Ok; thanks for having a look.

You're welcome. Attaching the updated patches.

By the way, I tried to replace pr_* with dev_* but then the messages
look kind of funny, e.g.
wdt wdt: initialized (timeout=60s, nowayout=0)
instead of:
xen_wdt: initialized (timeout=60s, nowayout=0)

I thought this would be more confusing than useful to the user, so I
reverted back to pr_*.

Thanks,
Radu


Radu Rendec (2):
  watchdog: xen_wdt: use the watchdog subsystem
  watchdog: xen_wdt: remove info message and version number

 drivers/watchdog/xen_wdt.c | 243 +++++++++------------------------------------
 1 file changed, 49 insertions(+), 194 deletions(-)

-- 
2.13.6

[PATCH v2 1/2] watchdog: xen_wdt: use the watchdog subsystem

[Radu Rendec]

Change the xen_wdt driver to use the watchdog subsystem instead of
registering and manipulating the char device directly through the misc
API. This is mainly getting rid of the "write" and "ioctl" methods and
part of the watchdog control logic (which are all implemented by the
watchdog subsystem).

Even though the watchdog subsystem supports registering and handling
multiple watchdog devices at the same time, the xen_wdt driver has an
inherent limitation of only one device due to the way the Xen hypervisor
exposes watchdog functionality. However, the driver can now coexist with
other watchdog devices (supported by different drivers).

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/xen_wdt.c | 239 ++++++++++-----------------------------------
 1 file changed, 49 insertions(+), 190 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index 5dd5c3494d55..5513047b70c9 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -21,25 +21,20 @@
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/init.h>
-#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/uaccess.h>
 #include <linux/watchdog.h>
 #include <xen/xen.h>
 #include <asm/xen/hypercall.h>
 #include <xen/interface/sched.h>
 
 static struct platform_device *platform_device;
-static DEFINE_SPINLOCK(wdt_lock);
 static struct sched_watchdog wdt;
 static time64_t wdt_expires;
-static bool is_active, expect_release;
 
 #define WATCHDOG_TIMEOUT 60 /* in seconds */
-static unsigned int timeout = WATCHDOG_TIMEOUT;
+static unsigned int timeout;
 module_param(timeout, uint, S_IRUGO);
 MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds "
 	"(default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
@@ -49,20 +44,18 @@ module_param(nowayout, bool, S_IRUGO);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
 	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-static inline time64_t set_timeout(void)
+static inline time64_t set_timeout(struct watchdog_device *wdd)
 {
-	wdt.timeout = timeout;
-	return ktime_get_seconds() + timeout;
+	wdt.timeout = wdd->timeout;
+	return ktime_get_seconds() + wdd->timeout;
 }
 
-static int xen_wdt_start(void)
+static int xen_wdt_start(struct watchdog_device *wdd)
 {
 	time64_t expires;
 	int err;
 
-	spin_lock(&wdt_lock);
-
-	expires = set_timeout();
+	expires = set_timeout(wdd);
 	if (!wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	else
@@ -74,36 +67,28 @@ static int xen_wdt_start(void)
 	} else
 		BUG_ON(!err);
 
-	spin_unlock(&wdt_lock);
-
 	return err;
 }
 
-static int xen_wdt_stop(void)
+static int xen_wdt_stop(struct watchdog_device *wdd)
 {
 	int err = 0;
 
-	spin_lock(&wdt_lock);
-
 	wdt.timeout = 0;
 	if (wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	if (!err)
 		wdt.id = 0;
 
-	spin_unlock(&wdt_lock);
-
 	return err;
 }
 
-static int xen_wdt_kick(void)
+static int xen_wdt_kick(struct watchdog_device *wdd)
 {
 	time64_t expires;
 	int err;
 
-	spin_lock(&wdt_lock);
-
-	expires = set_timeout();
+	expires = set_timeout(wdd);
 	if (wdt.id)
 		err = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wdt);
 	else
@@ -111,195 +96,71 @@ static int xen_wdt_kick(void)
 	if (!err)
 		wdt_expires = expires;
 
-	spin_unlock(&wdt_lock);
-
-	return err;
-}
-
-static int xen_wdt_open(struct inode *inode, struct file *file)
-{
-	int err;
-
-	/* /dev/watchdog can only be opened once */
-	if (xchg(&is_active, true))
-		return -EBUSY;
-
-	err = xen_wdt_start();
-	if (err == -EBUSY)
-		err = xen_wdt_kick();
-	return err ?: nonseekable_open(inode, file);
-}
-
-static int xen_wdt_release(struct inode *inode, struct file *file)
-{
-	int err = 0;
-
-	if (expect_release)
-		err = xen_wdt_stop();
-	else {
-		pr_crit("unexpected close, not stopping watchdog!\n");
-		xen_wdt_kick();
-	}
-	is_active = err;
-	expect_release = false;
 	return err;
 }
 
-static ssize_t xen_wdt_write(struct file *file, const char __user *data,
-			     size_t len, loff_t *ppos)
+static unsigned int xen_wdt_get_timeleft(struct watchdog_device *wdd)
 {
-	/* See if we got the magic character 'V' and reload the timer */
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* in case it was set long ago */
-			expect_release = false;
-
-			/* 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')
-					expect_release = true;
-			}
-		}
-
-		/* someone wrote to us, we should reload the timer */
-		xen_wdt_kick();
-	}
-	return len;
+	return wdt_expires - ktime_get_seconds();
 }
 
-static long xen_wdt_ioctl(struct file *file, unsigned int cmd,
-			  unsigned long arg)
-{
-	int new_options, retval = -EINVAL;
-	int new_timeout;
-	int __user *argp = (void __user *)arg;
-	static const struct watchdog_info ident = {
-		.options =		WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
-		.firmware_version =	0,
-		.identity =		DRV_NAME,
-	};
-
-	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, argp);
-
-	case WDIOC_SETOPTIONS:
-		if (get_user(new_options, argp))
-			return -EFAULT;
-
-		if (new_options & WDIOS_DISABLECARD)
-			retval = xen_wdt_stop();
-		if (new_options & WDIOS_ENABLECARD) {
-			retval = xen_wdt_start();
-			if (retval == -EBUSY)
-				retval = xen_wdt_kick();
-		}
-		return retval;
-
-	case WDIOC_KEEPALIVE:
-		xen_wdt_kick();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_timeout, argp))
-			return -EFAULT;
-		if (!new_timeout)
-			return -EINVAL;
-		timeout = new_timeout;
-		xen_wdt_kick();
-		/* fall through */
-	case WDIOC_GETTIMEOUT:
-		return put_user(timeout, argp);
-
-	case WDIOC_GETTIMELEFT:
-		retval = wdt_expires - ktime_get_seconds();
-		return put_user(retval, argp);
-	}
-
-	return -ENOTTY;
-}
+static struct watchdog_info xen_wdt_info = {
+	.identity = DRV_NAME,
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
 
-static const struct file_operations xen_wdt_fops = {
-	.owner =		THIS_MODULE,
-	.llseek =		no_llseek,
-	.write =		xen_wdt_write,
-	.unlocked_ioctl =	xen_wdt_ioctl,
-	.open =			xen_wdt_open,
-	.release =		xen_wdt_release,
+static const struct watchdog_ops xen_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = xen_wdt_start,
+	.stop = xen_wdt_stop,
+	.ping = xen_wdt_kick,
+	.get_timeleft = xen_wdt_get_timeleft,
 };
 
-static struct miscdevice xen_wdt_miscdev = {
-	.minor =	WATCHDOG_MINOR,
-	.name =		"watchdog",
-	.fops =		&xen_wdt_fops,
+static struct watchdog_device xen_wdt_dev = {
+	.info = &xen_wdt_info,
+	.ops = &xen_wdt_ops,
+	.timeout = WATCHDOG_TIMEOUT,
 };
 
-static int xen_wdt_probe(struct platform_device *dev)
+static int xen_wdt_probe(struct platform_device *pdev)
 {
 	struct sched_watchdog wd = { .id = ~0 };
 	int ret = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wd);
 
-	switch (ret) {
-	case -EINVAL:
-		if (!timeout) {
-			timeout = WATCHDOG_TIMEOUT;
-			pr_info("timeout value invalid, using %d\n", timeout);
-		}
-
-		ret = misc_register(&xen_wdt_miscdev);
-		if (ret) {
-			pr_err("cannot register miscdev on minor=%d (%d)\n",
-			       WATCHDOG_MINOR, ret);
-			break;
-		}
-
-		pr_info("initialized (timeout=%ds, nowayout=%d)\n",
-			timeout, nowayout);
-		break;
-
-	case -ENOSYS:
-		pr_info("not supported\n");
-		ret = -ENODEV;
-		break;
-
-	default:
-		pr_info("bogus return value %d\n", ret);
-		break;
+	if (ret == -ENOSYS) {
+		pr_err("watchdog not supported by hypervisor\n");
+		return -ENODEV;
 	}
 
-	return ret;
-}
+	if (ret != -EINVAL) {
+		pr_err("unexpected hypervisor error (%d)\n", ret);
+		return -ENODEV;
+	}
 
-static int xen_wdt_remove(struct platform_device *dev)
-{
-	/* Stop the timer before we leave */
-	if (!nowayout)
-		xen_wdt_stop();
+	if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
+		pr_info("timeout value invalid, using %d\n",
+			xen_wdt_dev.timeout);
+	watchdog_set_nowayout(&xen_wdt_dev, nowayout);
+	watchdog_stop_on_reboot(&xen_wdt_dev);
+	watchdog_stop_on_unregister(&xen_wdt_dev);
+
+	ret = devm_watchdog_register_device(&pdev->dev, &xen_wdt_dev);
+	if (ret) {
+		pr_err("cannot register watchdog device (%d)\n", ret);
+		return ret;
+	}
 
-	misc_deregister(&xen_wdt_miscdev);
+	pr_info("initialized (timeout=%ds, nowayout=%d)\n",
+		xen_wdt_dev.timeout, nowayout);
 
 	return 0;
 }
 
-static void xen_wdt_shutdown(struct platform_device *dev)
-{
-	xen_wdt_stop();
-}
-
 static int xen_wdt_suspend(struct platform_device *dev, pm_message_t state)
 {
 	typeof(wdt.id) id = wdt.id;
-	int rc = xen_wdt_stop();
+	int rc = xen_wdt_stop(&xen_wdt_dev);
 
 	wdt.id = id;
 	return rc;
@@ -310,13 +171,11 @@ static int xen_wdt_resume(struct platform_device *dev)
 	if (!wdt.id)
 		return 0;
 	wdt.id = 0;
-	return xen_wdt_start();
+	return xen_wdt_start(&xen_wdt_dev);
 }
 
 static struct platform_driver xen_wdt_driver = {
 	.probe          = xen_wdt_probe,
-	.remove         = xen_wdt_remove,
-	.shutdown       = xen_wdt_shutdown,
 	.suspend        = xen_wdt_suspend,
 	.resume         = xen_wdt_resume,
 	.driver         = {
-- 
2.13.6

[PATCH v2 2/2] watchdog: xen_wdt: remove info message and version number

[Radu Rendec]

The initial info message (early in the xen_wdt_init_module() function)
is not very useful and we already have a message on successful probe. If
the probe fails, additional messages are printed anyway.

The version number serves no useful purpose and it ran out of favor
upstream anyway.

Signed-off-by: Radu Rendec <rrendec@arista.com>
---
 drivers/watchdog/xen_wdt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index 5513047b70c9..a8eda69e4c90 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -12,7 +12,6 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #define DRV_NAME	"wdt"
-#define DRV_VERSION	"0.01"
 
 #include <linux/bug.h>
 #include <linux/errno.h>
@@ -190,8 +189,6 @@ static int __init xen_wdt_init_module(void)
 	if (!xen_domain())
 		return -ENODEV;
 
-	pr_info("Xen WatchDog Timer Driver v%s\n", DRV_VERSION);
-
 	err = platform_driver_register(&xen_wdt_driver);
 	if (err)
 		return err;
@@ -218,5 +215,4 @@ module_exit(xen_wdt_cleanup_module);
 
 MODULE_AUTHOR("Jan Beulich <jbeulich@novell.com>");
 MODULE_DESCRIPTION("Xen WatchDog Timer Driver");
-MODULE_VERSION(DRV_VERSION);
 MODULE_LICENSE("GPL");
-- 
2.13.6

Re: [v2,1/2] watchdog: xen_wdt: use the watchdog subsystem

[Guenter Roeck]

On Mon, Nov 06, 2017 at 07:04:11PM +0000, Radu Rendec wrote:
> Change the xen_wdt driver to use the watchdog subsystem instead of
> registering and manipulating the char device directly through the misc
> API. This is mainly getting rid of the "write" and "ioctl" methods and
> part of the watchdog control logic (which are all implemented by the
> watchdog subsystem).
> 
> Even though the watchdog subsystem supports registering and handling
> multiple watchdog devices at the same time, the xen_wdt driver has an
> inherent limitation of only one device due to the way the Xen hypervisor
> exposes watchdog functionality. However, the driver can now coexist with
> other watchdog devices (supported by different drivers).
> 
> Signed-off-by: Radu Rendec <rrendec@arista.com>

I just noticed this escaped my attention, possibly because it was sent
as reply to v1. Doesn't it say samewhere that you should not do that ?

> ---
>  drivers/watchdog/xen_wdt.c | 239 ++++++++++-----------------------------------
>  1 file changed, 49 insertions(+), 190 deletions(-)
> 
> diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> index 5dd5c3494d55..5513047b70c9 100644
> --- a/drivers/watchdog/xen_wdt.c
> +++ b/drivers/watchdog/xen_wdt.c

[ ... ]

> -static int xen_wdt_probe(struct platform_device *dev)
> +static int xen_wdt_probe(struct platform_device *pdev)
>  {
>  	struct sched_watchdog wd = { .id = ~0 };
>  	int ret = HYPERVISOR_sched_op(SCHEDOP_watchdog, &wd);
>  
> -	switch (ret) {
> -	case -EINVAL:
> -		if (!timeout) {
> -			timeout = WATCHDOG_TIMEOUT;
> -			pr_info("timeout value invalid, using %d\n", timeout);
> -		}
> -
> -		ret = misc_register(&xen_wdt_miscdev);
> -		if (ret) {
> -			pr_err("cannot register miscdev on minor=%d (%d)\n",
> -			       WATCHDOG_MINOR, ret);
> -			break;
> -		}
> -
> -		pr_info("initialized (timeout=%ds, nowayout=%d)\n",
> -			timeout, nowayout);
> -		break;
> -
> -	case -ENOSYS:
> -		pr_info("not supported\n");
> -		ret = -ENODEV;
> -		break;
> -
> -	default:
> -		pr_info("bogus return value %d\n", ret);
> -		break;
> +	if (ret == -ENOSYS) {
> +		pr_err("watchdog not supported by hypervisor\n");
> +		return -ENODEV;
>  	}
>  
> -	return ret;
> -}
> +	if (ret != -EINVAL) {
> +		pr_err("unexpected hypervisor error (%d)\n", ret);
> +		return -ENODEV;
> +	}
>  
> -static int xen_wdt_remove(struct platform_device *dev)
> -{
> -	/* Stop the timer before we leave */
> -	if (!nowayout)
> -		xen_wdt_stop();
> +	if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
> +		pr_info("timeout value invalid, using %d\n",
> +			xen_wdt_dev.timeout);

dev_info has that odd "wdt: " header because DRV_NAME is set to "wdt",
which doesn't really make much sense. Please use something more sensible
for DRV_NAME and use dev_info.

Guenter

Re: [v2,1/2] watchdog: xen_wdt: use the watchdog subsystem

[Radu Rendec]

On Sat, 2017-11-11 at 08:40 -0800, Guenter Roeck wrote:
> On Mon, Nov 06, 2017 at 07:04:11PM +0000, Radu Rendec wrote:
> > Change the xen_wdt driver to use the watchdog subsystem instead of
> > registering and manipulating the char device directly through the misc
> > API. This is mainly getting rid of the "write" and "ioctl" methods and
> > part of the watchdog control logic (which are all implemented by the
> > watchdog subsystem).
> > 
> > Even though the watchdog subsystem supports registering and handling
> > multiple watchdog devices at the same time, the xen_wdt driver has an
> > inherent limitation of only one device due to the way the Xen hypervisor
> > exposes watchdog functionality. However, the driver can now coexist with
> > other watchdog devices (supported by different drivers).
> > 
> > Signed-off-by: Radu Rendec <rrendec@arista.com>
> 
> I just noticed this escaped my attention, possibly because it was sent
> as reply to v1. Doesn't it say samewhere that you should not do that ?

Sorry about that. In the future, I will make sure to start a new thread
for new patch versions.

> dev_info has that odd "wdt: " header because DRV_NAME is set to "wdt",
> which doesn't really make much sense. Please use something more sensible
> for DRV_NAME and use dev_info.

The trouble is (was) that the device is matched against the driver by
the name, so the device name has to be the same as the driver name.
Then, regardless of what the name is, __dev_printk() will print it
twice (once as the driver name and once as the device name).

As a workaround, after looking at platform_match(), I added an id_table
to the platform_driver struct and now the match is done by that. Then
the driver name can be different.

I set the driver name to "xen_wdt" and the device name to "watchdog".
Now the messages (using dev_info()) look like this:

xen_wdt watchdog: initialized (timeout=60s, nowayout=0)

At least this looks more meaningful; I hope it's ok.

I will submit v3 of the patch (with these changes) in a separate
thread.

Thanks,
Radu