Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/5] watchdog: Create watchdog device in watchdog_dev.c
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
> Signed-off-by: Guenter Roeck
> ---
> 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
