[PATCH 0/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[PATCH 0/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[Guenter Roeck]

The lifetime of the watchdog device pointer is different from the lifetime
of its character device. Remove it entirely to avoid race conditions, and
to make sure that drivers don't start to use it again.

[PATCH 1/3] watchdog: Add support for creating driver specific sysfs attributes

[Guenter Roeck]

The Zodiac watchdog driver attaches additional sysfs attributes to the
watchdog device. This has a number of problems: The watchdog device
lifetime differs from the driver lifetime, and the device structure
should therefore not be accessed from drivers. Also, creating sysfs
attributes after driver registration results in a potential race condition
if user space expects the attributes to exist but they don't exist yet.

Add support for creating driver specific sysfs attributes to the watchdog
core to solve the problems.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/watchdog/watchdog-kernel-api.txt | 3 +++
 drivers/watchdog/watchdog_dev.c                | 5 +++--
 include/linux/watchdog.h                       | 3 +++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 72a009478b15..312f60009c3e 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -46,6 +46,7 @@ struct watchdog_device {
 	int id;
 	struct device *dev;
 	struct device *parent;
+	const struct attribute_group **groups;
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
@@ -68,6 +69,8 @@ It contains following fields:
 * dev: device under the watchdog class (created by watchdog_register_device).
 * parent: set this to the parent device (or NULL) before calling
   watchdog_register_device.
+* groups: List of sysfs attribute groups to create when creating the watchdog
+  device.
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 3cab6f6e7f1c..e89ccb2e9603 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -744,8 +744,9 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 	if (ret)
 		return ret;
 
-	dev = device_create(&watchdog_class, wdd->parent, devno, wdd,
-			    "watchdog%d", wdd->id);
+	dev = device_create_with_groups(&watchdog_class, wdd->parent,
+					devno, wdd, wdd->groups,
+					"watchdog%d", wdd->id);
 	if (IS_ERR(dev)) {
 		watchdog_cdev_unregister(wdd);
 		return PTR_ERR(dev);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 850af04fe0c7..2a68370411a9 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -57,6 +57,8 @@ struct watchdog_ops {
  * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
  * @dev:	The device for our watchdog
  * @parent:	The parent bus device
+ * @groups:	List of sysfs attribute groups to create when creating the
+ *		watchdog device.
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
  * @bootstatus:	Status of the watchdog device at boot.
@@ -84,6 +86,7 @@ struct watchdog_device {
 	int id;
 	struct device *dev;
 	struct device *parent;
+	const struct attribute_group **groups;
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
-- 
2.1.4

[PATCH 2/3] watchdog: ziirave: Use watchdog infrastructure to create sysfs attributes

[Guenter Roeck]

The watchdog core now supports creating driver specific sysfs attributes
when creating the watchdog device.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/ziirave_wdt.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index b498fdcc231a..0c7cb7302cf0 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -207,10 +207,7 @@ static struct attribute *ziirave_wdt_attrs[] = {
 	&dev_attr_reset_reason.attr,
 	NULL
 };
-
-static const struct attribute_group ziirave_wdt_sysfs_files = {
-	.attrs  = ziirave_wdt_attrs,
-};
+ATTRIBUTE_GROUPS(ziirave_wdt);
 
 static int ziirave_wdt_init_duration(struct i2c_client *client)
 {
@@ -260,6 +257,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	w_priv->wdd.min_timeout = ZIIRAVE_TIMEOUT_MIN;
 	w_priv->wdd.max_timeout = ZIIRAVE_TIMEOUT_MAX;
 	w_priv->wdd.parent = &client->dev;
+	w_priv->wdd.groups = ziirave_wdt_groups;
 
 	ret = watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
 	if (ret) {
@@ -327,26 +325,14 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		return -ENODEV;
 
 	ret = watchdog_register_device(&w_priv->wdd);
-	if (ret)
-		return ret;
-
-	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
-				 &ziirave_wdt_sysfs_files);
-	if (ret) {
-		watchdog_unregister_device(&w_priv->wdd);
 
-		return ret;
-	}
-
-	return 0;
+	return ret;
 }
 
 static int ziirave_wdt_remove(struct i2c_client *client)
 {
 	struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
 
-	sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files);
-
 	watchdog_unregister_device(&w_priv->wdd);
 
 	return 0;
-- 
2.1.4

Re: [PATCH 2/3] watchdog: ziirave: Use watchdog infrastructure to create sysfs attributes

[Martyn Welch]

On 03/01/16 23:11, Guenter Roeck wrote:
> The watchdog core now supports creating driver specific sysfs attributes
> when creating the watchdog device.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Martyn Welch <martyn.welch@collabora.co.uk>

(Sorry for the delayed response, this email got caught by my spam filtering)

> ---
>   drivers/watchdog/ziirave_wdt.c | 20 +++-----------------
>   1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index b498fdcc231a..0c7cb7302cf0 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -207,10 +207,7 @@ static struct attribute *ziirave_wdt_attrs[] = {
>   	&dev_attr_reset_reason.attr,
>   	NULL
>   };
> -
> -static const struct attribute_group ziirave_wdt_sysfs_files = {
> -	.attrs  = ziirave_wdt_attrs,
> -};
> +ATTRIBUTE_GROUPS(ziirave_wdt);
>
>   static int ziirave_wdt_init_duration(struct i2c_client *client)
>   {
> @@ -260,6 +257,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>   	w_priv->wdd.min_timeout = ZIIRAVE_TIMEOUT_MIN;
>   	w_priv->wdd.max_timeout = ZIIRAVE_TIMEOUT_MAX;
>   	w_priv->wdd.parent = &client->dev;
> +	w_priv->wdd.groups = ziirave_wdt_groups;
>
>   	ret = watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
>   	if (ret) {
> @@ -327,26 +325,14 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>   		return -ENODEV;
>
>   	ret = watchdog_register_device(&w_priv->wdd);
> -	if (ret)
> -		return ret;
> -
> -	ret = sysfs_create_group(&w_priv->wdd.dev->kobj,
> -				 &ziirave_wdt_sysfs_files);
> -	if (ret) {
> -		watchdog_unregister_device(&w_priv->wdd);
>
> -		return ret;
> -	}
> -
> -	return 0;
> +	return ret;
>   }
>
>   static int ziirave_wdt_remove(struct i2c_client *client)
>   {
>   	struct ziirave_wdt_data *w_priv = i2c_get_clientdata(client);
>
> -	sysfs_remove_group(&client->dev.kobj, &ziirave_wdt_sysfs_files);
> -
>   	watchdog_unregister_device(&w_priv->wdd);
>
>   	return 0;
>

[PATCH 3/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[Guenter Roeck]

The lifetime of the watchdog device pointer is different from the lifetime
of its character device. Remove it entirely to avoid race conditions.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 Documentation/watchdog/watchdog-kernel-api.txt | 2 --
 drivers/watchdog/watchdog_core.c               | 8 ++++----
 drivers/watchdog/watchdog_dev.c                | 9 ++++-----
 include/linux/watchdog.h                       | 2 --
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 312f60009c3e..55120a055a14 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ The watchdog device structure looks like this:
 
 struct watchdog_device {
 	int id;
-	struct device *dev;
 	struct device *parent;
 	const struct attribute_group **groups;
 	const struct watchdog_info *info;
@@ -66,7 +65,6 @@ It contains following fields:
   /dev/watchdog0 cdev (dynamic major, minor 0) as well as the old
   /dev/watchdog miscdev. The id is set automatically when calling
   watchdog_register_device.
-* dev: device under the watchdog class (created by watchdog_register_device).
 * parent: set this to the parent device (or NULL) before calling
   watchdog_register_device.
 * groups: List of sysfs attribute groups to create when creating the watchdog
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index ec1ab6c1a80b..e600fd93b7de 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -249,8 +249,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 
 		ret = register_reboot_notifier(&wdd->reboot_nb);
 		if (ret) {
-			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
-				ret);
+			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
+			       wdd->id, ret);
 			watchdog_dev_unregister(wdd);
 			ida_simple_remove(&watchdog_ida, wdd->id);
 			return ret;
@@ -262,8 +262,8 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 
 		ret = register_restart_handler(&wdd->restart_nb);
 		if (ret)
-			dev_warn(wdd->dev, "Cannot register restart handler (%d)\n",
-				 ret);
+			pr_warn("watchog%d: Cannot register restart handler (%d)\n",
+				wdd->id, ret);
 	}
 
 	return 0;
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e89ccb2e9603..ba2ecce4aae6 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -143,7 +143,8 @@ static int watchdog_stop(struct watchdog_device *wdd)
 		return 0;
 
 	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
-		dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
+		pr_info("watchdog%d: nowayout prevents watchdog being stopped!\n",
+			wdd->id);
 		return -EBUSY;
 	}
 
@@ -604,7 +605,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/* If the watchdog was not stopped, send a keepalive ping */
 	if (err < 0) {
-		dev_crit(wdd->dev, "watchdog did not stop!\n");
+		pr_crit("watchdog%d: watchdog did not stop!\n", wdd->id);
 		watchdog_ping(wdd);
 	}
 
@@ -751,7 +752,6 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 		watchdog_cdev_unregister(wdd);
 		return PTR_ERR(dev);
 	}
-	wdd->dev = dev;
 
 	return ret;
 }
@@ -766,8 +766,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 
 void watchdog_dev_unregister(struct watchdog_device *wdd)
 {
-	device_destroy(&watchdog_class, wdd->dev->devt);
-	wdd->dev = NULL;
+	device_destroy(&watchdog_class, wdd->wd_data->cdev.dev);
 	watchdog_cdev_unregister(wdd);
 }
 
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 2a68370411a9..1fb11c5a49c6 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -55,7 +55,6 @@ struct watchdog_ops {
 /** struct watchdog_device - The structure that defines a watchdog device
  *
  * @id:		The watchdog's ID. (Allocated by watchdog_register_device)
- * @dev:	The device for our watchdog
  * @parent:	The parent bus device
  * @groups:	List of sysfs attribute groups to create when creating the
  *		watchdog device.
@@ -84,7 +83,6 @@ struct watchdog_ops {
  */
 struct watchdog_device {
 	int id;
-	struct device *dev;
 	struct device *parent;
 	const struct attribute_group **groups;
 	const struct watchdog_info *info;
-- 
2.1.4

Re: [PATCH 0/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[Guenter Roeck]

On 01/03/2016 03:11 PM, Guenter Roeck wrote:
> The lifetime of the watchdog device pointer is different from the lifetime
> of its character device. Remove it entirely to avoid race conditions, and
> to make sure that drivers don't start to use it again.
>

I should have mentioned: The series applies on top of watchdog-next.

Guenter

Re: [PATCH 0/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[Wim Van Sebroeck]

Hi Guenter,

> The lifetime of the watchdog device pointer is different from the lifetime
> of its character device. Remove it entirely to avoid race conditions, and
> to make sure that drivers don't start to use it again.

This patchset has been added to linux-watchdog-next.

Kind regards,
Wim.

Re: [PATCH 0/3] watchdog: Drop pointer to watchdog device from struct watchdog_device

[Guenter Roeck]

On Mon, Jan 11, 2016 at 10:29:18PM +0100, Wim Van Sebroeck wrote:
> Hi Guenter,
> 
> > The lifetime of the watchdog device pointer is different from the lifetime
> > of its character device. Remove it entirely to avoid race conditions, and
> > to make sure that drivers don't start to use it again.
> 
> This patchset has been added to linux-watchdog-next.
> 
Thanks!

Guenter