On Sun, Nov 29, 2015 at 06:31:18PM -0800, Guenter Roeck wrote:
> Hi Damien,
>
> On 11/26/2015 12:35 PM, Damien Riegel wrote:
> >Currently, the entry in /dev is created before the device associated
> >with the watchdog is created. Therefore, a user can do operations on the
> >watchdog before it is fully ready.
> >
> >This patch changes order of operations to create the device first. As
> >the core might try to create the device with different ids, it is
> >simpler to group the device creation and the char device registration in
> >the same function. This commit also adds a counterpart function to group
> >unregistration and device destruction.
> >
> >Signed-off-by: Damien Riegel <[email protected]>
> >---
> >Changes in v3:
> > - changed order of operations to first create the device and then
> > register the char dev. On cleanup, unregister then destroy.
> >
> >Changes in v2:
> > - move call to device_destroy before watchdog_dev_unregister
> >
> > drivers/watchdog/watchdog_core.c | 60
> > +++++++++++++++++++++++-----------------
> > drivers/watchdog/watchdog_core.h | 1 +
> > drivers/watchdog/watchdog_dev.c | 7 ++++-
> > 3 files changed, 42 insertions(+), 26 deletions(-)
> >
> >diff --git a/drivers/watchdog/watchdog_core.c
> >b/drivers/watchdog/watchdog_core.c
> >index 551af04..13a7a58 100644
> >--- a/drivers/watchdog/watchdog_core.c
> >+++ b/drivers/watchdog/watchdog_core.c
> >@@ -192,9 +192,39 @@ void watchdog_set_restart_priority(struct
> >watchdog_device *wdd, int priority)
> > }
> > EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
> >
> >+static int __watchdog_create_register(struct watchdog_device *wdd)
>
> Do we need the '__' here ? Seems unnecessary.
> After all, there is no public watchdog_create_register().
>
It seems that some subsystems also use '__' for functions that assume
that arguments are valid, or/and don't lock. But it is just a matter of
style, I don't mind dropping them.
> >+{
> >+ dev_t devno;
> >+ int ret;
> >+
> >+ devno = watchdog_dev_get_devno(wdd);
>
> Maybe that explains the initialization order.
>
> In a way this is getting odd: The only reason for watchdog_dev_init()
> to exist is to initialize watchdog_devt, which is then returned by
> this function. Looking into the use of watchdog_devt in watchdog_dev.c,
> it is not actually needed there anymore: Its sole purpose is now to
> report it with watchdog_dev_get_devno(). The only other remaining use,
>
> pr_err("watchdog%d unable to add device %d:%d\n",
> wdd->id, MAJOR(watchdog_devt), wdd->id);
>
> could as well be replaced with
>
> pr_err("watchdog%d unable to add device %d:%d\n",
> wdd->id, MAJOR(devno), wdd->id);
>
> or even
> pr_err("watchdog%d unable to add device %d:%d\n",
> wdd->id, MAJOR(devno), MINOR(devno));
>
> Given that, maybe it would make sense to move watchdog_devt and its
> initialization to watchdog_core.c, and drop watchdog_dev_get_devno(),
> watchdog_dev_init(), and watchdog_dev_exit().
>
> What do you think ? More important, Wim, what do you think ?
I think it would make sense as watchdog_class is already handled in
watchdog_core.c. That way, all the core init operations would be in the
same file, and watchdog_dev.c would only contain the necessary bits to
register/unregister a watchdog device.
>
> [ that should be separate patch, though ]
>
> >+ wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> >+ wdd, "watchdog%d", wdd->id);
>
> Please align continuation lines with '('.
>
> >+ if (IS_ERR(wdd->dev))
> >+ return PTR_ERR(wdd->dev);
> >+
> >+ ret = watchdog_dev_register(wdd);
> >+ if (ret)
> >+ device_destroy(watchdog_class, devno);
> >+
> >+ return ret;
> >+}
> >+
> >+static void __watchdog_unregister_destroy(struct watchdog_device *wdd)
>
> Same as above - not sure if the '__' adds any value.
>
> >+{
> >+ dev_t devno = wdd->cdev.dev;
> >+ int ret;
> >+
> >+ ret = watchdog_dev_unregister(wdd);
> >+ if (ret)
> >+ pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> >+ device_destroy(watchdog_class, devno);
> >+ wdd->dev = NULL;
> >+}
> >+
> > 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;
> >@@ -228,7 +258,7 @@ static int __watchdog_register_device(struct
> >watchdog_device *wdd)
> > return id;
> > wdd->id = id;
> >
> >- ret = watchdog_dev_register(wdd);
> >+ ret = __watchdog_create_register(wdd);
> > if (ret) {
> > ida_simple_remove(&watchdog_ida, id);
> > if (!(id == 0 && ret == -EBUSY))
> >@@ -240,23 +270,13 @@ static int __watchdog_register_device(struct
> >watchdog_device *wdd)
> > return id;
> > wdd->id = id;
> >
> >- ret = watchdog_dev_register(wdd);
> >+ ret = __watchdog_create_register(wdd);
> > if (ret) {
> > ida_simple_remove(&watchdog_ida, id);
> > return ret;
> > }
> > }
> >
> >- 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;
> >
> >@@ -264,10 +284,8 @@ static int __watchdog_register_device(struct
> >watchdog_device *wdd)
> > if (ret) {
> > dev_err(wdd->dev, "Cannot register reboot notifier
> > (%d)\n",
> > ret);
> >- watchdog_dev_unregister(wdd);
> >- device_destroy(watchdog_class, devno);
> >+ __watchdog_unregister_destroy(wdd);
> > ida_simple_remove(&watchdog_ida, wdd->id);
> >- wdd->dev = NULL;
> > return ret;
> > }
> > }
> >@@ -311,9 +329,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 +338,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_unregister_destroy(wdd);
> > ida_simple_remove(&watchdog_ida, wdd->id);
> >- wdd->dev = NULL;
> > }
> >
> > /**
> >diff --git a/drivers/watchdog/watchdog_core.h
> >b/drivers/watchdog/watchdog_core.h
> >index 1c8d6b0..8c98164 100644
> >--- a/drivers/watchdog/watchdog_core.h
> >+++ b/drivers/watchdog/watchdog_core.h
> >@@ -35,3 +35,4 @@ 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 __exit watchdog_dev_exit(void);
> >+dev_t watchdog_dev_get_devno(struct watchdog_device *);
> >diff --git a/drivers/watchdog/watchdog_dev.c
> >b/drivers/watchdog/watchdog_dev.c
> >index 21224bd..140a995 100644
> >--- a/drivers/watchdog/watchdog_dev.c
> >+++ b/drivers/watchdog/watchdog_dev.c
> >@@ -632,6 +632,11 @@ static struct miscdevice watchdog_miscdev = {
> > * thus we set it up like that.
> > */
> >
> >+dev_t watchdog_dev_get_devno(struct watchdog_device *wdd)
> >+{
> >+ return MKDEV(MAJOR(watchdog_devt), wdd->id);
> >+}
> >+
> > int watchdog_dev_register(struct watchdog_device *wdd)
> > {
> > int err, devno;
> >@@ -652,7 +657,7 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> > }
> >
> > /* Fill in the data structures */
> >- devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
> >+ devno = wdd->dev->devt;
> > cdev_init(&wdd->cdev, &watchdog_fops);
> > wdd->cdev.owner = wdd->ops->owner;
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
