Date:	Mon, 7 Dec 2015 11:15:49 -0500
From:	Damien Riegel <damien.riegel@...oirfairelinux.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	Wim Van Sebroeck <wim@...ana.be>, linux-watchdog@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT PATCH] watchdog: Move watchdog device creation to
 watchdog_dev.c

On Sun, Dec 06, 2015 at 11:51:41AM -0800, Guenter Roeck wrote:
> The watchdog character device s currently created in watchdog_dev.c,
> and the watchdog device in watchdog_core.c. This results in
> cross-dependencies, as the device creation needs to know the watchdog
> character device number.
> 
> On top of that, the watchdog character device is created before the
> watchdog device is created. This can result in race conditions if the
> watchdog device node is accessed before the watchdog device has been
> created.
> 
> To solve the problem, move watchdog device creation into watchdog_dev.c,
> and create the watchdog device prior to creating its device node.
> Also move device class creation into watchdog_dev.c, since this is now
> the only place where the watchdog class is needed.
> 
> Inspired by an earlier patch set from Damien Riegel.
> 
> Cc: Damien Riegel <damien.riegel@...oirfairelinux.com>
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> Hi Damien,
> 
> I think this approach would be a bit better. The watchdog device isn't
> really used in the watchdog core code, so it is better created in
> watchdog_dev.c. That also fits well with other pending changes, such as
> sysfs attribute support, and my attempts to move the ref/unref functions
> completely into the watchdog core. As a side effect, it also cleans up
> the error path in __watchdog_register_device().
> 
> What do you think ?

Hi Guenter,

Like the idea, but I don't really get the separation. For instance, you
move watchdog_class in watchdog_dev.c but you keep watchdog_ida in
watchdog_core.c whereas it is only used for device creation/deletion.

Also a few minor comments below.

> The code has been compile tested only so far. I'll try to test it later
> today, but I wanted to get it out for discussion.

I'll give it a try but I have only one board to test it.

> 
> Thanks,
> Guenter
> 
>  drivers/watchdog/watchdog_core.c | 37 ++----------------------
>  drivers/watchdog/watchdog_core.h |  2 +-
>  drivers/watchdog/watchdog_dev.c  | 61 ++++++++++++++++++++++++++++++++--------
>  3 files changed, 54 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 873f13972cf4..089e930fce19 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -41,7 +41,6 @@
>  #include "watchdog_core.h"	/* For watchdog_dev_register/... */
>  
>  static DEFINE_IDA(watchdog_ida);
> -static struct class *watchdog_class;
>  
>  /*
>   * Deferred Registration infrastructure.
> @@ -139,7 +138,7 @@ EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>  
>  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;
> @@ -192,16 +191,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> -	devno = wdd->cdev.dev;
> -	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> -					NULL, "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;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -232,19 +221,8 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
>  
>  static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  {
> -	int ret;
> -	int devno;
> -
> -	if (wdd == NULL)
> -		return;
> -
> -	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;
>  }
>  
>  /**
> @@ -287,17 +265,9 @@ static int __init watchdog_init(void)
>  {
>  	int err;
>  
> -	watchdog_class = class_create(THIS_MODULE, "watchdog");
> -	if (IS_ERR(watchdog_class)) {
> -		pr_err("couldn't create class\n");
> -		return PTR_ERR(watchdog_class);
> -	}
> -
>  	err = watchdog_dev_init();
> -	if (err < 0) {
> -		class_destroy(watchdog_class);
> +	if (err < 0)
>  		return err;
> -	}
>  
>  	watchdog_deferred_registration();
>  	return 0;
> @@ -306,7 +276,6 @@ static int __init watchdog_init(void)
>  static void __exit watchdog_exit(void)
>  {
>  	watchdog_dev_exit();
> -	class_destroy(watchdog_class);
>  	ida_destroy(&watchdog_ida);
>  }
>  
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 6c951418fca7..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 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 56a649e66eb2..07abe8c9e58c 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -49,6 +49,9 @@ static dev_t watchdog_devt;
>  /* the watchdog device behind /dev/watchdog */
>  static struct watchdog_device *old_wdd;
>  
> +/* the watchdog device class */
> +static struct class *watchdog_class;
> +
>  /*
>   *	watchdog_ping: ping the watchdog.
>   *	@wdd: the watchdog device to ping
> @@ -523,7 +526,7 @@ static struct miscdevice watchdog_miscdev = {
>   *	thus we set it up like that.
>   */
>  
> -int watchdog_dev_register(struct watchdog_device *wdd)
> +int _watchdog_dev_register(struct watchdog_device *wdd)

Doc not updated and still refers to watchdog_dev_register.

>  {
>  	int err, devno;
>  
> @@ -532,11 +535,12 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>  		watchdog_miscdev.parent = wdd->parent;
>  		err = misc_register(&watchdog_miscdev);
>  		if (err != 0) {
> -			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> -				wdd->info->identity, WATCHDOG_MINOR, err);
> +			dev_err(wdd->dev,
> +				"Cannot register miscdev on minor=%d (err=%d).\n",
> +				WATCHDOG_MINOR, err);
>  			if (err == -EBUSY)
> -				pr_err("%s: a legacy watchdog module is probably present.\n",
> -					wdd->info->identity);
> +				dev_err(wdd->dev,
> +					"A legacy watchdog module is probably present.\n");
>  			old_wdd = NULL;
>  			return err;
>  		}
> @@ -550,8 +554,8 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>  	/* Add the device */
>  	err  = cdev_add(&wdd->cdev, devno, 1);
>  	if (err) {
> -		pr_err("watchdog%d unable to add device %d:%d\n",
> -			wdd->id,  MAJOR(watchdog_devt), wdd->id);
> +		dev_err(wdd->dev, "Unable to add device %d:%d\n",
> +			MAJOR(watchdog_devt), wdd->id);
>  		if (wdd->id == 0) {
>  			misc_deregister(&watchdog_miscdev);
>  			old_wdd = NULL;
> @@ -567,7 +571,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>   *	Unregister the watchdog and if needed the legacy /dev/watchdog device.
>   */
>  
> -int watchdog_dev_unregister(struct watchdog_device *wdd)
> +void _watchdog_dev_unregister(struct watchdog_device *wdd)

Doc not updated.

>  {
>  	mutex_lock(&wdd->lock);
>  	set_bit(WDOG_UNREGISTERED, &wdd->status);
> @@ -578,7 +582,31 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>  		misc_deregister(&watchdog_miscdev);
>  		old_wdd = NULL;
>  	}
> -	return 0;
> +}
> +
> +int watchdog_dev_register(struct watchdog_device *wdd)
> +{
> +	dev_t devno;
> +	int ret;
> +
> +	devno = MKDEV(MAJOR(watchdog_devt), wdd->id);

This is now done twice, here and in _watchdog_dev_register. Maybe
_watchdog_dev_register should be updated to read:

  devno = wdd->dev.devt

The end result is the same but the link between the two devices is more
explicit (like it is now).

> +	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> +				 wdd, "watchdog%d", wdd->id);
> +	if (IS_ERR(wdd->dev))
> +		return PTR_ERR(wdd->dev);
> +
> +	ret = _watchdog_dev_register(wdd);
> +	if (ret)
> +		device_destroy(watchdog_class, devno);
> +
> +	return ret;
> +}
> +
> +void watchdog_dev_unregister(struct watchdog_device *wdd)
> +{
> +	_watchdog_dev_unregister(wdd);
> +	device_destroy(watchdog_class, wdd->dev->devt);
> +	wdd->dev = NULL;
>  }
>  
>  /*
> @@ -589,9 +617,19 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
>  
>  int __init watchdog_dev_init(void)
>  {
> -	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> -	if (err < 0)
> +	int err;
> +
> +	watchdog_class = class_create(THIS_MODULE, "watchdog");
> +	if (IS_ERR(watchdog_class)) {
> +		pr_err("couldn't create watchdog class\n");
> +		return PTR_ERR(watchdog_class);
> +	}
> +
> +	err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> +	if (err < 0) {
>  		pr_err("watchdog: unable to allocate char dev region\n");
> +		class_destroy(watchdog_class);
> +	}

Newline here ? checkpatch does not complain and this file style is not
consistent on new line before return. Don't know which style is
preferred.

>  	return err;
>  }
>  
> @@ -604,4 +642,5 @@ int __init watchdog_dev_init(void)
>  void __exit watchdog_dev_exit(void)
>  {
>  	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> +	class_destroy(watchdog_class);
>  }
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists