Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756079AbbLGQWI (ORCPT ); Mon, 7 Dec 2015 11:22:08 -0500 Received: from mail.savoirfairelinux.com ([208.88.110.44]:47252 "EHLO mail.savoirfairelinux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932076AbbLGQWF (ORCPT ); Mon, 7 Dec 2015 11:22:05 -0500 Date: Mon, 7 Dec 2015 11:15:49 -0500 From: Damien Riegel To: Guenter Roeck Cc: Wim Van Sebroeck , linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC/RFT PATCH] watchdog: Move watchdog device creation to watchdog_dev.c Message-ID: <20151207161549.GA6030@localhost> Mail-Followup-To: Damien Riegel , Guenter Roeck , Wim Van Sebroeck , linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org References: <1449431501-15843-1-git-send-email-linux@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449431501-15843-1-git-send-email-linux@roeck-us.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9657 Lines: 301 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 > Signed-off-by: Guenter Roeck > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/