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

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

[Radu Rendec]

Hi Guenter,

Attaching v3 of the xen_wdt patch, including the dev_info() fix. Please
note that I had to make some additional changes to make this work - as
described in my previous email (included below for your convenience).

8<----------------------------------------------------------------------

On Sat, 2017-11-11 at 08:40 -0800, Guenter Roeck wrote:
> 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.

8<----------------------------------------------------------------------

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

 drivers/watchdog/xen_wdt.c | 258 +++++++++++----------------------------------
 1 file changed, 59 insertions(+), 199 deletions(-)

-- 
2.13.6

[PATCH v3 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 | 254 +++++++++++----------------------------------
 1 file changed, 59 insertions(+), 195 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index 5dd5c3494d55..4e00419bd47d 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -9,9 +9,8 @@
  *	2 of the License, or (at your option) any later version.
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#define DRV_NAME	"wdt"
+#define DRV_NAME	"xen_wdt"
+#define DEV_NAME	"watchdog"
 #define DRV_VERSION	"0.01"
 
 #include <linux/bug.h>
@@ -21,25 +20,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 +43,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 +66,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 +95,72 @@ 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)
+static unsigned int xen_wdt_get_timeleft(struct watchdog_device *wdd)
 {
-	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;
+	return wdt_expires - ktime_get_seconds();
 }
 
-static ssize_t xen_wdt_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;
-
-			/* 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;
-}
-
-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) {
+		dev_err(&pdev->dev, "watchdog not supported by hypervisor\n");
+		return -ENODEV;
 	}
 
-	return ret;
-}
+	if (ret != -EINVAL) {
+		dev_err(&pdev->dev, "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))
+		dev_info(&pdev->dev, "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) {
+		dev_err(&pdev->dev, "cannot register watchdog device (%d)\n",
+			ret);
+		return ret;
+	}
 
-	misc_deregister(&xen_wdt_miscdev);
+	dev_info(&pdev->dev, "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,18 +171,22 @@ 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 const struct platform_device_id xen_wdt_id_table[] = {
+	{ .name	= DEV_NAME },
+	{}
+};
+
 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         = {
 		.name   = DRV_NAME,
 	},
+	.id_table       = xen_wdt_id_table,
 };
 
 static int __init xen_wdt_init_module(void)
@@ -337,7 +202,7 @@ static int __init xen_wdt_init_module(void)
 	if (err)
 		return err;
 
-	platform_device = platform_device_register_simple(DRV_NAME,
+	platform_device = platform_device_register_simple(DEV_NAME,
 								  -1, NULL, 0);
 	if (IS_ERR(platform_device)) {
 		err = PTR_ERR(platform_device);
@@ -351,7 +216,6 @@ static void __exit xen_wdt_cleanup_module(void)
 {
 	platform_device_unregister(platform_device);
 	platform_driver_unregister(&xen_wdt_driver);
-	pr_info("module unloaded\n");
 }
 
 module_init(xen_wdt_init_module);
-- 
2.13.6

[PATCH v3 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 4e00419bd47d..1a40721716ef 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -11,7 +11,6 @@
 
 #define DRV_NAME	"xen_wdt"
 #define DEV_NAME	"watchdog"
-#define DRV_VERSION	"0.01"
 
 #include <linux/bug.h>
 #include <linux/errno.h>
@@ -196,8 +195,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;
@@ -223,5 +220,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: [PATCH v3 0/2] xen_wdt: use the watchdog subsystem

[Guenter Roeck]

On Tue, Nov 14, 2017 at 04:23:48PM +0000, Radu Rendec wrote:
> Hi Guenter,
> 
> Attaching v3 of the xen_wdt patch, including the dev_info() fix. Please
> note that I had to make some additional changes to make this work - as
> described in my previous email (included below for your convenience).
> 
> 8<----------------------------------------------------------------------
> 
> On Sat, 2017-11-11 at 08:40 -0800, Guenter Roeck wrote:
> > 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).
> 
Isn't that rather because MODULE_ALIAS is missing in the driver ?

Guenter

> 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.
> 
> 8<----------------------------------------------------------------------
> 
> Radu Rendec (2):
>   watchdog: xen_wdt: use the watchdog subsystem
>   watchdog: xen_wdt: remove info message and version number
> 
>  drivers/watchdog/xen_wdt.c | 258 +++++++++++----------------------------------
>  1 file changed, 59 insertions(+), 199 deletions(-)
> 
> -- 
> 2.13.6
> 

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

[Radu Rendec]

On Tue, 2017-11-14 at 09:31 -0800, Guenter Roeck wrote:
> On Tue, Nov 14, 2017 at 04:23:48PM +0000, Radu Rendec wrote:
> > Hi Guenter,
> > 
> > Attaching v3 of the xen_wdt patch, including the dev_info() fix. Please
> > note that I had to make some additional changes to make this work - as
> > described in my previous email (included below for your convenience).
> > 
> > 8<----------------------------------------------------------------------
> > 
> > On Sat, 2017-11-11 at 08:40 -0800, Guenter Roeck wrote:
> > > 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).
> > 
> 
> Isn't that rather because MODULE_ALIAS is missing in the driver ?

No, I don't think so. I think MODULE_ALIAS is pretty much what it says,
so it just provides a way to "probe" the module by a different name.
For example, if I define MODULE_ALIAS("foobar"), then I can use
`modprobe foobar` and it will load the module. Special syntax within
the alias string exists to load the module automatically under certain
circumstances (e.g. modules with an alias starting with "platform:"
will be automatically loaded when platform devices are scanned).

On the other hand, the code in __dev_printk() (which is ultimately used
by all dev_... macros) is pretty straightforward:

if (dev)
	dev_printk_emit(level[1] - '0', dev, "%s %s: %pV",
			dev_driver_string(dev), dev_name(dev), vaf);

So it will always print both the driver name and the device name. If
they are the same (which is required in order to match the device with
the driver, in the absence of an id_table), it will print the same name
twice.

Radu

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

[Guenter Roeck]

On Tue, Nov 14, 2017 at 04:23:49PM +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>
> ---
>  drivers/watchdog/xen_wdt.c | 254 +++++++++++----------------------------------
>  1 file changed, 59 insertions(+), 195 deletions(-)
> 
> diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> index 5dd5c3494d55..4e00419bd47d 100644
> --- a/drivers/watchdog/xen_wdt.c
> +++ b/drivers/watchdog/xen_wdt.c
> @@ -9,9 +9,8 @@
>   *	2 of the License, or (at your option) any later version.
>   */
>  
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> -#define DRV_NAME	"wdt"
> +#define DRV_NAME	"xen_wdt"
> +#define DEV_NAME	"watchdog"

[ ... ]

>  
> +static const struct platform_device_id xen_wdt_id_table[] = {
> +	{ .name	= DEV_NAME },

I understand we are way into bikeshedding territory, and I was inclined
to let this just go, but this is way too generic for a platform device
ID, sorry.

Guenter

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

[Radu Rendec]

On Wed, 2017-11-15 at 06:58 -0800, Guenter Roeck wrote:
> On Tue, Nov 14, 2017 at 04:23:49PM +0000, Radu Rendec wrote:
> > diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> > index 5dd5c3494d55..4e00419bd47d 100644
> > --- a/drivers/watchdog/xen_wdt.c
> > +++ b/drivers/watchdog/xen_wdt.c
> > @@ -9,9 +9,8 @@
> >   *	2 of the License, or (at your option) any later version.
> >   */
> >  
> > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > -
> > -#define DRV_NAME	"wdt"
> > +#define DRV_NAME	"xen_wdt"
> > +#define DEV_NAME	"watchdog"
> 
> [ ... ]
> 
> >  
> > +static const struct platform_device_id xen_wdt_id_table[] = {
> > +	{ .name	= DEV_NAME },
> 
> I understand we are way into bikeshedding territory, and I was inclined
> to let this just go, but this is way too generic for a platform device
> ID, sorry.

Yes, that's the part I don't like about it either. So I think we're on
the same page here :)

FWIW, the old (current) version has the same problem, because in the
absence of an id_table (or a matching entry), platform_match() matches
against the driver name, which is just "wdt".

Anyway, how about using "xwdt0" for DEV_NAME? The driver supports only
one device by design, so adding "0" should be fine.

Thanks,
Radu

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

[Guenter Roeck]

On Wed, Nov 15, 2017 at 03:34:40PM +0000, Radu Rendec wrote:
> On Wed, 2017-11-15 at 06:58 -0800, Guenter Roeck wrote:
> > On Tue, Nov 14, 2017 at 04:23:49PM +0000, Radu Rendec wrote:
> > > diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> > > index 5dd5c3494d55..4e00419bd47d 100644
> > > --- a/drivers/watchdog/xen_wdt.c
> > > +++ b/drivers/watchdog/xen_wdt.c
> > > @@ -9,9 +9,8 @@
> > >   *	2 of the License, or (at your option) any later version.
> > >   */
> > >  
> > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > -
> > > -#define DRV_NAME	"wdt"
> > > +#define DRV_NAME	"xen_wdt"
> > > +#define DEV_NAME	"watchdog"
> > 
> > [ ... ]
> > 
> > >  
> > > +static const struct platform_device_id xen_wdt_id_table[] = {
> > > +	{ .name	= DEV_NAME },
> > 
> > I understand we are way into bikeshedding territory, and I was inclined
> > to let this just go, but this is way too generic for a platform device
> > ID, sorry.
> 
> Yes, that's the part I don't like about it either. So I think we're on
> the same page here :)
> 
> FWIW, the old (current) version has the same problem, because in the
> absence of an id_table (or a matching entry), platform_match() matches
> against the driver name, which is just "wdt".
> 
> Anyway, how about using "xwdt0" for DEV_NAME? The driver supports only
> one device by design, so adding "0" should be fine.
> 
A numeric suffix to the device name suggests that there can be more than one,
which would be misleading.

And all that because you don't like the output of dev_info(). Hope you don't
start to mess with all the other drivers having the same "problem" of a less
than perfect output from dev_ functions.

Guenter

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

[Radu Rendec]

On Wed, 2017-11-15 at 08:26 -0800, Guenter Roeck wrote:
> On Wed, Nov 15, 2017 at 03:34:40PM +0000, Radu Rendec wrote:
> > On Wed, 2017-11-15 at 06:58 -0800, Guenter Roeck wrote:
> > > On Tue, Nov 14, 2017 at 04:23:49PM +0000, Radu Rendec wrote:
> > > > diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> > > > index 5dd5c3494d55..4e00419bd47d 100644
> > > > --- a/drivers/watchdog/xen_wdt.c
> > > > +++ b/drivers/watchdog/xen_wdt.c
> > > > @@ -9,9 +9,8 @@
> > > >   *	2 of the License, or (at your option) any later version.
> > > >   */
> > > >  
> > > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > -
> > > > -#define DRV_NAME	"wdt"
> > > > +#define DRV_NAME	"xen_wdt"
> > > > +#define DEV_NAME	"watchdog"
> > > 
> > > [ ... ]
> > > 
> > > >  
> > > > +static const struct platform_device_id xen_wdt_id_table[] = {
> > > > +	{ .name	= DEV_NAME },
> > > 
> > > I understand we are way into bikeshedding territory, and I was inclined
> > > to let this just go, but this is way too generic for a platform device
> > > ID, sorry.
> > 
> > Yes, that's the part I don't like about it either. So I think we're on
> > the same page here :)
> > 
> > FWIW, the old (current) version has the same problem, because in the
> > absence of an id_table (or a matching entry), platform_match() matches
> > against the driver name, which is just "wdt".
> > 
> > Anyway, how about using "xwdt0" for DEV_NAME? The driver supports only
> > one device by design, so adding "0" should be fine.
> > 
> 
> A numeric suffix to the device name suggests that there can be more than one,
> which would be misleading.
> 
> And all that because you don't like the output of dev_info(). Hope you don't
> start to mess with all the other drivers having the same "problem" of a less
> than perfect output from dev_ functions.

I was merely trying to make the output look better than just a repeated
string (which doesn't make much sense, IMHO).

At this point I see the following options:

   1. Revert to the previous patch version, but use dev_ instead of pr_ as
      you suggested. Messages would look like "wdt wdt: ...". Note that
      this still has the problem of registering a platform "id" of "wdt"
      which *is* quite generic and may collide with other drivers.
   2. Same as above, but change DRV_NAME to "xen_wdt", which is less
      generic. Messages would look like "xen_wdt xen_wdt: ...".
   3. Keep the id_table and change DEV_NAME to something without a numeric
      suffix, e.g. "xwdt".

At the end of the day, what I really want is to port the xen_wdt driver
to use the watchdog subsystem.

Please let me know which of the above options you prefer and I will
send an updated v4.

Thanks,
Radu

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

[Guenter Roeck]

On Wed, Nov 15, 2017 at 04:46:28PM +0000, Radu Rendec wrote:
> On Wed, 2017-11-15 at 08:26 -0800, Guenter Roeck wrote:
> > On Wed, Nov 15, 2017 at 03:34:40PM +0000, Radu Rendec wrote:
> > > On Wed, 2017-11-15 at 06:58 -0800, Guenter Roeck wrote:
> > > > On Tue, Nov 14, 2017 at 04:23:49PM +0000, Radu Rendec wrote:
> > > > > diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
> > > > > index 5dd5c3494d55..4e00419bd47d 100644
> > > > > --- a/drivers/watchdog/xen_wdt.c
> > > > > +++ b/drivers/watchdog/xen_wdt.c
> > > > > @@ -9,9 +9,8 @@
> > > > >   *	2 of the License, or (at your option) any later version.
> > > > >   */
> > > > >  
> > > > > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > > -
> > > > > -#define DRV_NAME	"wdt"
> > > > > +#define DRV_NAME	"xen_wdt"
> > > > > +#define DEV_NAME	"watchdog"
> > > > 
> > > > [ ... ]
> > > > 
> > > > >  
> > > > > +static const struct platform_device_id xen_wdt_id_table[] = {
> > > > > +	{ .name	= DEV_NAME },
> > > > 
> > > > I understand we are way into bikeshedding territory, and I was inclined
> > > > to let this just go, but this is way too generic for a platform device
> > > > ID, sorry.
> > > 
> > > Yes, that's the part I don't like about it either. So I think we're on
> > > the same page here :)
> > > 
> > > FWIW, the old (current) version has the same problem, because in the
> > > absence of an id_table (or a matching entry), platform_match() matches
> > > against the driver name, which is just "wdt".
> > > 
> > > Anyway, how about using "xwdt0" for DEV_NAME? The driver supports only
> > > one device by design, so adding "0" should be fine.
> > > 
> > 
> > A numeric suffix to the device name suggests that there can be more than one,
> > which would be misleading.
> > 
> > And all that because you don't like the output of dev_info(). Hope you don't
> > start to mess with all the other drivers having the same "problem" of a less
> > than perfect output from dev_ functions.
> 
> I was merely trying to make the output look better than just a repeated
> string (which doesn't make much sense, IMHO).
> 
> At this point I see the following options:
> 
>    1. Revert to the previous patch version, but use dev_ instead of pr_ as
>       you suggested. Messages would look like "wdt wdt: ...". Note that
>       this still has the problem of registering a platform "id" of "wdt"
>       which *is* quite generic and may collide with other drivers.
>    2. Same as above, but change DRV_NAME to "xen_wdt", which is less
>       generic. Messages would look like "xen_wdt xen_wdt: ...".
>    3. Keep the id_table and change DEV_NAME to something without a numeric
>       suffix, e.g. "xwdt".
> 
> At the end of the day, what I really want is to port the xen_wdt driver
> to use the watchdog subsystem.
> 
> Please let me know which of the above options you prefer and I will
> send an updated v4.
> 
I would suggest the second option.

Guenter