| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 21 Dec 2015 12:31:24 -0500
From: Damien Riegel <damien.riegel@...oirfairelinux.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: linux-watchdog@...r.kernel.org, Wim Van Sebroeck <wim@...ana.be>,
Pratyush Anand <panand@...hat.com>,
Hans de Goede <hdegoede@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c
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@...oirfairelinux.com>
> Signed-off-by: Guenter Roeck <linux@...ck-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@...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