Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1295687 > unrolled thread

[PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c

Started byGuenter Roeck <linux@roeck-us.net>
First post2015-12-20 22:10 +0100
Last post2015-12-22 17:20 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c Guenter Roeck <linux@roeck-us.net> - 2015-12-20 22:10 +0100
    Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-21 18:40 +0100
      Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c Guenter Roeck <linux@roeck-us.net> - 2015-12-22 00:30 +0100
        Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c Damien Riegel <damien.riegel@savoirfairelinux.com> - 2015-12-22 16:50 +0100
          Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c Guenter Roeck <linux@roeck-us.net> - 2015-12-22 17:20 +0100

#1295687 — [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c

FromGuenter Roeck <linux@roeck-us.net>
Date2015-12-20 22:10 +0100
Subject[PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c
Message-ID<qHToR-5QV-1@gated-at.bofh.it>
The watchdog character device is currently created in watchdog_dev.c,
and the watchdog device in watchdog_core.c. This results in
cross-dependencies, since device creation needs to know the watchdog
character device number as well as the watchdog class, both of which
reside in watchdog_dev.c.

Create the watchdog device in watchdog_dev.c to simplify the code.

Inspired by earlier patch set from Damien Riegel.

Cc: Damien Riegel <damien.riegel@savoirfairelinux.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/watchdog_core.c | 33 ++++--------------
 drivers/watchdog/watchdog_core.h |  4 +--
 drivers/watchdog/watchdog_dev.c  | 73 +++++++++++++++++++++++++++++++++-------
 3 files changed, 69 insertions(+), 41 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 551af042867c..f0293f7d2b80 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -42,7 +42,6 @@
 #include "watchdog_core.h"	/* For watchdog_dev_register/... */
 
 static DEFINE_IDA(watchdog_ida);
-static struct class *watchdog_class;
 
 /*
  * Deferred Registration infrastructure.
@@ -194,7 +193,7 @@ EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
 
 static int __watchdog_register_device(struct watchdog_device *wdd)
 {
-	int ret, id = -1, devno;
+	int ret, id = -1;
 
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -EINVAL;
@@ -247,16 +246,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		}
 	}
 
-	devno = wdd->cdev.dev;
-	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
-					wdd, "watchdog%d", wdd->id);
-	if (IS_ERR(wdd->dev)) {
-		watchdog_dev_unregister(wdd);
-		ida_simple_remove(&watchdog_ida, id);
-		ret = PTR_ERR(wdd->dev);
-		return ret;
-	}
-
 	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
 		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
 
@@ -265,9 +254,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
 				ret);
 			watchdog_dev_unregister(wdd);
-			device_destroy(watchdog_class, devno);
 			ida_simple_remove(&watchdog_ida, wdd->id);
-			wdd->dev = NULL;
 			return ret;
 		}
 	}
@@ -311,9 +298,6 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
 
 static void __watchdog_unregister_device(struct watchdog_device *wdd)
 {
-	int ret;
-	int devno;
-
 	if (wdd == NULL)
 		return;
 
@@ -323,13 +307,8 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
 	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
 		unregister_reboot_notifier(&wdd->reboot_nb);
 
-	devno = wdd->cdev.dev;
-	ret = watchdog_dev_unregister(wdd);
-	if (ret)
-		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
-	device_destroy(watchdog_class, devno);
+	watchdog_dev_unregister(wdd);
 	ida_simple_remove(&watchdog_ida, wdd->id);
-	wdd->dev = NULL;
 }
 
 /**
@@ -370,9 +349,11 @@ static int __init watchdog_deferred_registration(void)
 
 static int __init watchdog_init(void)
 {
-	watchdog_class = watchdog_dev_init();
-	if (IS_ERR(watchdog_class))
-		return PTR_ERR(watchdog_class);
+	int err;
+
+	err = watchdog_dev_init();
+	if (err < 0)
+		return err;
 
 	watchdog_deferred_registration();
 	return 0;
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 1c8d6b0e68c7..86ff962d1e15 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -32,6 +32,6 @@
  *	Functions/procedures to be called by the core
  */
 extern int watchdog_dev_register(struct watchdog_device *);
-extern int watchdog_dev_unregister(struct watchdog_device *);
-extern struct class * __init watchdog_dev_init(void);
+extern void watchdog_dev_unregister(struct watchdog_device *);
+extern int __init watchdog_dev_init(void);
 extern void __exit watchdog_dev_exit(void);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d93118e97d11..c24392623e98 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -626,17 +626,18 @@ static struct miscdevice watchdog_miscdev = {
 };
 
 /*
- *	watchdog_dev_register: register a watchdog device
+ *	watchdog_cdev_register: register watchdog character device
  *	@wdd: watchdog device
+ *	@devno: character device number
  *
- *	Register a watchdog device including handling the legacy
+ *	Register a watchdog character device including handling the legacy
  *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
  *	thus we set it up like that.
  */
 
-int watchdog_dev_register(struct watchdog_device *wdd)
+static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 {
-	int err, devno;
+	int err;
 
 	if (wdd->id == 0) {
 		old_wdd = wdd;
@@ -654,7 +655,6 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 	}
 
 	/* Fill in the data structures */
-	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
 	cdev_init(&wdd->cdev, &watchdog_fops);
 	wdd->cdev.owner = wdd->ops->owner;
 
@@ -672,13 +672,14 @@ int watchdog_dev_register(struct watchdog_device *wdd)
 }
 
 /*
- *	watchdog_dev_unregister: unregister a watchdog device
+ *	watchdog_cdev_unregister: unregister watchdog character device
  *	@watchdog: watchdog device
  *
- *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
+ *	Unregister watchdog character device and if needed the legacy
+ *	/dev/watchdog device.
  */
 
-int watchdog_dev_unregister(struct watchdog_device *wdd)
+static void watchdog_cdev_unregister(struct watchdog_device *wdd)
 {
 	mutex_lock(&wdd->lock);
 	set_bit(WDOG_UNREGISTERED, &wdd->status);
@@ -689,7 +690,6 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
 		misc_deregister(&watchdog_miscdev);
 		old_wdd = NULL;
 	}
-	return 0;
 }
 
 static struct class watchdog_class = {
@@ -701,29 +701,76 @@ static struct class watchdog_class = {
 };
 
 /*
+ *	watchdog_dev_register: register a watchdog device
+ *	@wdd: watchdog device
+ *
+ *	Register a watchdog device including handling the legacy
+ *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
+ *	thus we set it up like that.
+ */
+
+int watchdog_dev_register(struct watchdog_device *wdd)
+{
+	struct device *dev;
+	dev_t devno;
+	int ret;
+
+	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
+
+	ret = watchdog_cdev_register(wdd, devno);
+	if (ret)
+		return ret;
+
+	dev = device_create(&watchdog_class, wdd->parent, devno, wdd,
+			    "watchdog%d", wdd->id);
+	if (IS_ERR(dev)) {
+		watchdog_cdev_unregister(wdd);
+		return PTR_ERR(dev);
+	}
+	wdd->dev = dev;
+
+	return ret;
+}
+
+/*
+ *	watchdog_dev_unregister: unregister a watchdog device
+ *	@watchdog: watchdog device
+ *
+ *	Unregister watchdog device and if needed the legacy
+ *	/dev/watchdog device.
+ */
+
+void watchdog_dev_unregister(struct watchdog_device *wdd)
+{
+	watchdog_cdev_unregister(wdd);
+	device_destroy(&watchdog_class, wdd->dev->devt);
+	wdd->dev = NULL;
+}
+
+/*
  *	watchdog_dev_init: init dev part of watchdog core
  *
  *	Allocate a range of chardev nodes to use for watchdog devices
  */
 
-struct class * __init watchdog_dev_init(void)
+int __init watchdog_dev_init(void)
 {
 	int err;
 
 	err = class_register(&watchdog_class);
 	if (err < 0) {
 		pr_err("couldn't register class\n");
-		return ERR_PTR(err);
+		return err;
 	}
 
 	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
 	if (err < 0) {
 		pr_err("watchdog: unable to allocate char dev region\n");
 		class_unregister(&watchdog_class);
-		return ERR_PTR(err);
+		return err;
 	}
 
-	return &watchdog_class;
+	return 0;
 }
 
 /*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1296106

FromDamien Riegel <damien.riegel@savoirfairelinux.com>
Date2015-12-21 18:40 +0100
Message-ID<qIcBc-11j-13@gated-at.bofh.it>
In reply to#1295687
On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
> The watchdog character device is currently created in watchdog_dev.c,
> and the watchdog device in watchdog_core.c. This results in
> cross-dependencies, since device creation needs to know the watchdog
> character device number as well as the watchdog class, both of which
> reside in watchdog_dev.c.
> 
> Create the watchdog device in watchdog_dev.c to simplify the code.
> 
> Inspired by earlier patch set from Damien Riegel.

Hi Guenter,

The main purpose of my patch was to inverse the device creation and the
cdev registration to avoid a racy situation, bu you have dropped that in
this version. Is there a reason for that?

Thanks,
Damien

> 
> Cc: Damien Riegel <damien.riegel@savoirfairelinux.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/watchdog/watchdog_core.c | 33 ++++--------------
>  drivers/watchdog/watchdog_core.h |  4 +--
>  drivers/watchdog/watchdog_dev.c  | 73 +++++++++++++++++++++++++++++++++-------
>  3 files changed, 69 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 551af042867c..f0293f7d2b80 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -42,7 +42,6 @@
>  #include "watchdog_core.h"	/* For watchdog_dev_register/... */
>  
>  static DEFINE_IDA(watchdog_ida);
> -static struct class *watchdog_class;
>  
>  /*
>   * Deferred Registration infrastructure.
> @@ -194,7 +193,7 @@ EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
>  
>  static int __watchdog_register_device(struct watchdog_device *wdd)
>  {
> -	int ret, id = -1, devno;
> +	int ret, id = -1;
>  
>  	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
>  		return -EINVAL;
> @@ -247,16 +246,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> -	devno = wdd->cdev.dev;
> -	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> -					wdd, "watchdog%d", wdd->id);
> -	if (IS_ERR(wdd->dev)) {
> -		watchdog_dev_unregister(wdd);
> -		ida_simple_remove(&watchdog_ida, id);
> -		ret = PTR_ERR(wdd->dev);
> -		return ret;
> -	}
> -
>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>  		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>  
> @@ -265,9 +254,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  			dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
>  				ret);
>  			watchdog_dev_unregister(wdd);
> -			device_destroy(watchdog_class, devno);
>  			ida_simple_remove(&watchdog_ida, wdd->id);
> -			wdd->dev = NULL;
>  			return ret;
>  		}
>  	}
> @@ -311,9 +298,6 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
>  
>  static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  {
> -	int ret;
> -	int devno;
> -
>  	if (wdd == NULL)
>  		return;
>  
> @@ -323,13 +307,8 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
>  		unregister_reboot_notifier(&wdd->reboot_nb);
>  
> -	devno = wdd->cdev.dev;
> -	ret = watchdog_dev_unregister(wdd);
> -	if (ret)
> -		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> -	device_destroy(watchdog_class, devno);
> +	watchdog_dev_unregister(wdd);
>  	ida_simple_remove(&watchdog_ida, wdd->id);
> -	wdd->dev = NULL;
>  }
>  
>  /**
> @@ -370,9 +349,11 @@ static int __init watchdog_deferred_registration(void)
>  
>  static int __init watchdog_init(void)
>  {
> -	watchdog_class = watchdog_dev_init();
> -	if (IS_ERR(watchdog_class))
> -		return PTR_ERR(watchdog_class);
> +	int err;
> +
> +	err = watchdog_dev_init();
> +	if (err < 0)
> +		return err;
>  
>  	watchdog_deferred_registration();
>  	return 0;
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 1c8d6b0e68c7..86ff962d1e15 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -32,6 +32,6 @@
>   *	Functions/procedures to be called by the core
>   */
>  extern int watchdog_dev_register(struct watchdog_device *);
> -extern int watchdog_dev_unregister(struct watchdog_device *);
> -extern struct class * __init watchdog_dev_init(void);
> +extern void watchdog_dev_unregister(struct watchdog_device *);
> +extern int __init watchdog_dev_init(void);
>  extern void __exit watchdog_dev_exit(void);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index d93118e97d11..c24392623e98 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -626,17 +626,18 @@ static struct miscdevice watchdog_miscdev = {
>  };
>  
>  /*
> - *	watchdog_dev_register: register a watchdog device
> + *	watchdog_cdev_register: register watchdog character device
>   *	@wdd: watchdog device
> + *	@devno: character device number
>   *
> - *	Register a watchdog device including handling the legacy
> + *	Register a watchdog character device including handling the legacy
>   *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
>   *	thus we set it up like that.
>   */
>  
> -int watchdog_dev_register(struct watchdog_device *wdd)
> +static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  {
> -	int err, devno;
> +	int err;
>  
>  	if (wdd->id == 0) {
>  		old_wdd = wdd;
> @@ -654,7 +655,6 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>  	}
>  
>  	/* Fill in the data structures */
> -	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
>  	cdev_init(&wdd->cdev, &watchdog_fops);
>  	wdd->cdev.owner = wdd->ops->owner;
>  
> @@ -672,13 +672,14 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>  }
>  
>  /*
> - *	watchdog_dev_unregister: unregister a watchdog device
> + *	watchdog_cdev_unregister: unregister watchdog character device
>   *	@watchdog: watchdog device
>   *
> - *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
> + *	Unregister watchdog character device and if needed the legacy
> + *	/dev/watchdog device.
>   */
>  
> -int watchdog_dev_unregister(struct watchdog_device *wdd)
> +static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>  {
>  	mutex_lock(&wdd->lock);
>  	set_bit(WDOG_UNREGISTERED, &wdd->status);
> @@ -689,7 +690,6 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>  		misc_deregister(&watchdog_miscdev);
>  		old_wdd = NULL;
>  	}
> -	return 0;
>  }
>  
>  static struct class watchdog_class = {
> @@ -701,29 +701,76 @@ static struct class watchdog_class = {
>  };
>  
>  /*
> + *	watchdog_dev_register: register a watchdog device
> + *	@wdd: watchdog device
> + *
> + *	Register a watchdog device including handling the legacy
> + *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
> + *	thus we set it up like that.
> + */
> +
> +int watchdog_dev_register(struct watchdog_device *wdd)
> +{
> +	struct device *dev;
> +	dev_t devno;
> +	int ret;
> +
> +	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
> +
> +	ret = watchdog_cdev_register(wdd, devno);
> +	if (ret)
> +		return ret;
> +
> +	dev = device_create(&watchdog_class, wdd->parent, devno, wdd,
> +			    "watchdog%d", wdd->id);
> +	if (IS_ERR(dev)) {
> +		watchdog_cdev_unregister(wdd);
> +		return PTR_ERR(dev);
> +	}
> +	wdd->dev = dev;
> +
> +	return ret;
> +}
> +
> +/*
> + *	watchdog_dev_unregister: unregister a watchdog device
> + *	@watchdog: watchdog device
> + *
> + *	Unregister watchdog device and if needed the legacy
> + *	/dev/watchdog device.
> + */
> +
> +void watchdog_dev_unregister(struct watchdog_device *wdd)
> +{
> +	watchdog_cdev_unregister(wdd);
> +	device_destroy(&watchdog_class, wdd->dev->devt);
> +	wdd->dev = NULL;
> +}
> +
> +/*
>   *	watchdog_dev_init: init dev part of watchdog core
>   *
>   *	Allocate a range of chardev nodes to use for watchdog devices
>   */
>  
> -struct class * __init watchdog_dev_init(void)
> +int __init watchdog_dev_init(void)
>  {
>  	int err;
>  
>  	err = class_register(&watchdog_class);
>  	if (err < 0) {
>  		pr_err("couldn't register class\n");
> -		return ERR_PTR(err);
> +		return err;
>  	}
>  
>  	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
>  	if (err < 0) {
>  		pr_err("watchdog: unable to allocate char dev region\n");
>  		class_unregister(&watchdog_class);
> -		return ERR_PTR(err);
> +		return err;
>  	}
>  
> -	return &watchdog_class;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296299

FromGuenter Roeck <linux@roeck-us.net>
Date2015-12-22 00:30 +0100
Message-ID<qIi3V-4vW-33@gated-at.bofh.it>
In reply to#1296106
On 12/21/2015 09:31 AM, Damien Riegel wrote:
> On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
>> The watchdog character device is currently created in watchdog_dev.c,
>> and the watchdog device in watchdog_core.c. This results in
>> cross-dependencies, since device creation needs to know the watchdog
>> character device number as well as the watchdog class, both of which
>> reside in watchdog_dev.c.
>>
>> Create the watchdog device in watchdog_dev.c to simplify the code.
>>
>> Inspired by earlier patch set from Damien Riegel.
>
> Hi Guenter,
>
> The main purpose of my patch was to inverse the device creation and the
> cdev registration to avoid a racy situation, bu you have dropped that in
> this version. Is there a reason for that?
>
Every other driver I looked at does it in the same order (cdev first, device
second). I don't really know if doing it differently has any undesired
side effect, so I wanted to play safe.

It would help a lot if someone listening to this exchange can confirm
that it is ok to create the device first, followed by the character device.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296845

FromDamien Riegel <damien.riegel@savoirfairelinux.com>
Date2015-12-22 16:50 +0100
Message-ID<qIxmi-5MK-9@gated-at.bofh.it>
In reply to#1296299
On Mon, Dec 21, 2015 at 03:28:39PM -0800, Guenter Roeck wrote:
> On 12/21/2015 09:31 AM, Damien Riegel wrote:
> >On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
> >>The watchdog character device is currently created in watchdog_dev.c,
> >>and the watchdog device in watchdog_core.c. This results in
> >>cross-dependencies, since device creation needs to know the watchdog
> >>character device number as well as the watchdog class, both of which
> >>reside in watchdog_dev.c.
> >>
> >>Create the watchdog device in watchdog_dev.c to simplify the code.
> >>
> >>Inspired by earlier patch set from Damien Riegel.
> >
> >Hi Guenter,
> >
> >The main purpose of my patch was to inverse the device creation and the
> >cdev registration to avoid a racy situation, bu you have dropped that in
> >this version. Is there a reason for that?
> >
> Every other driver I looked at does it in the same order (cdev first, device
> second). I don't really know if doing it differently has any undesired
> side effect, so I wanted to play safe.
> 
> It would help a lot if someone listening to this exchange can confirm
> that it is ok to create the device first, followed by the character device.

The issue is that some drivers use watchdog_device->dev in their
watchdog_ops functions. With a quick grep, I could spot 3 examples:

 - bcm2835_wdt_stop in bcm2835_wdt.c
 - gpio_wdt_hwping in gpio_wdt.c
 - a21_wdt_set_timeout in mena21_wdt.c

Maybe we should simply fix these drivers and keep watchdog_device->dev
for core internal usage?


Thanks,
Damien
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296875

FromGuenter Roeck <linux@roeck-us.net>
Date2015-12-22 17:20 +0100
Message-ID<qIxPk-6cI-1@gated-at.bofh.it>
In reply to#1296845
On 12/22/2015 07:33 AM, Damien Riegel wrote:
> On Mon, Dec 21, 2015 at 03:28:39PM -0800, Guenter Roeck wrote:
>> On 12/21/2015 09:31 AM, Damien Riegel wrote:
>>> On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
>>>> The watchdog character device is currently created in watchdog_dev.c,
>>>> and the watchdog device in watchdog_core.c. This results in
>>>> cross-dependencies, since device creation needs to know the watchdog
>>>> character device number as well as the watchdog class, both of which
>>>> reside in watchdog_dev.c.
>>>>
>>>> Create the watchdog device in watchdog_dev.c to simplify the code.
>>>>
>>>> Inspired by earlier patch set from Damien Riegel.
>>>
>>> Hi Guenter,
>>>
>>> The main purpose of my patch was to inverse the device creation and the
>>> cdev registration to avoid a racy situation, bu you have dropped that in
>>> this version. Is there a reason for that?
>>>
>> Every other driver I looked at does it in the same order (cdev first, device
>> second). I don't really know if doing it differently has any undesired
>> side effect, so I wanted to play safe.
>>
>> It would help a lot if someone listening to this exchange can confirm
>> that it is ok to create the device first, followed by the character device.
>
> The issue is that some drivers use watchdog_device->dev in their
> watchdog_ops functions. With a quick grep, I could spot 3 examples:
>
>   - bcm2835_wdt_stop in bcm2835_wdt.c
>   - gpio_wdt_hwping in gpio_wdt.c
>   - a21_wdt_set_timeout in mena21_wdt.c
>
> Maybe we should simply fix these drivers and keep watchdog_device->dev
> for core internal usage?
>

Yes, I have been thinking about that - essentially move the ->dev pointer
to the internal data structure and either use pr_ functions in those drivers,
or save a pointer to the platform device (if available) and use it.

I think I'll start working on a follow-up patch set to do just that.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web