* [PATCH] watchdog: core: factorize register error paths @ 2015-11-24 20:15 Damien Riegel 2015-11-24 20:45 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Damien Riegel @ 2015-11-24 20:15 UTC (permalink / raw) To: linux-watchdog; +Cc: Wim Van Sebroeck, Guenter Roeck, kernel, Damien Riegel The commit adding the reboot notifier in the core introduced a new error path in __watchdog_register_device, making error paths quite redondant. This commit factorizes all error paths that do some cleanup. Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> --- drivers/watchdog/watchdog_core.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 551af04..aff17b1 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd) wdd->id = id; ret = watchdog_dev_register(wdd); - if (ret) { - ida_simple_remove(&watchdog_ida, id); - return ret; - } + if (ret) + goto remove_ida; } 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; + goto unregister_device; } if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { @@ -264,11 +260,7 @@ 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); - ida_simple_remove(&watchdog_ida, wdd->id); - wdd->dev = NULL; - return ret; + goto unregister_device; } } @@ -282,6 +274,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) } return 0; + +unregister_device: + watchdog_dev_unregister(wdd); + if (!IS_ERR(wdd->dev)) { + device_destroy(watchdog_class, devno); + wdd->dev = NULL; + } +remove_ida: + ida_simple_remove(&watchdog_ida, id); + + return ret; } /** -- 2.5.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: core: factorize register error paths 2015-11-24 20:15 [PATCH] watchdog: core: factorize register error paths Damien Riegel @ 2015-11-24 20:45 ` Guenter Roeck 2015-11-24 22:40 ` Damien Riegel 0 siblings, 1 reply; 4+ messages in thread From: Guenter Roeck @ 2015-11-24 20:45 UTC (permalink / raw) To: Damien Riegel; +Cc: linux-watchdog, Wim Van Sebroeck, kernel Hi Damien, On Tue, Nov 24, 2015 at 03:15:35PM -0500, Damien Riegel wrote: > The commit adding the reboot notifier in the core introduced a new error > path in __watchdog_register_device, making error paths quite redondant. > s/redondant/redundant/ > This commit factorizes all error paths that do some cleanup. > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > --- > drivers/watchdog/watchdog_core.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index 551af04..aff17b1 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > wdd->id = id; > > ret = watchdog_dev_register(wdd); > - if (ret) { > - ida_simple_remove(&watchdog_ida, id); > - return ret; > - } > + if (ret) > + goto remove_ida; > } > > 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; > + goto unregister_device; > } > > if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { > @@ -264,11 +260,7 @@ 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); > - ida_simple_remove(&watchdog_ida, wdd->id); > - wdd->dev = NULL; > - return ret; > + goto unregister_device; > } > } > > @@ -282,6 +274,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > } > > return 0; > + > +unregister_device: > + watchdog_dev_unregister(wdd); > + if (!IS_ERR(wdd->dev)) { > + device_destroy(watchdog_class, devno); > + wdd->dev = NULL; Oddly enough this leaves wdd->dev in place if it is an ERR_PTR. While that was the case before, it might be better and more consistent to always set it to NULL. I also wonder if it is really necessary to call watchdog_dev_unregister() first. Something like destroy_device: device_destroy(watchdog_class, devno); unregister_watchdog: wdd->dev = NULL; watchdog_dev_unregister(wdd); would be cleaner. Wonder why it isn't done in that order to start with. It should be possible; after all, the device is created only after watchdog_dev_register() succeeded, so one should think that it makes sense to remove it first. Any idea ? Thanks, Guenter > + } > +remove_ida: > + ida_simple_remove(&watchdog_ida, id); > + > + return ret; > } > > /** > -- > 2.5.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: core: factorize register error paths 2015-11-24 20:45 ` Guenter Roeck @ 2015-11-24 22:40 ` Damien Riegel 2015-11-24 23:23 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Damien Riegel @ 2015-11-24 22:40 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-watchdog, Wim Van Sebroeck, kernel On Tue, Nov 24, 2015 at 12:45:48PM -0800, Guenter Roeck wrote: > Hi Damien, > > On Tue, Nov 24, 2015 at 03:15:35PM -0500, Damien Riegel wrote: > > The commit adding the reboot notifier in the core introduced a new error > > path in __watchdog_register_device, making error paths quite redondant. > > > s/redondant/redundant/ > > > This commit factorizes all error paths that do some cleanup. > > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > > --- > > drivers/watchdog/watchdog_core.c | 27 +++++++++++++++------------ > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > > index 551af04..aff17b1 100644 > > --- a/drivers/watchdog/watchdog_core.c > > +++ b/drivers/watchdog/watchdog_core.c > > @@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > > wdd->id = id; > > > > ret = watchdog_dev_register(wdd); > > - if (ret) { > > - ida_simple_remove(&watchdog_ida, id); > > - return ret; > > - } > > + if (ret) > > + goto remove_ida; > > } > > > > 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; > > + goto unregister_device; > > } > > > > if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { > > @@ -264,11 +260,7 @@ 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); > > - ida_simple_remove(&watchdog_ida, wdd->id); > > - wdd->dev = NULL; > > - return ret; > > + goto unregister_device; > > } > > } > > > > @@ -282,6 +274,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > > } > > > > return 0; > > + > > +unregister_device: > > + watchdog_dev_unregister(wdd); > > + if (!IS_ERR(wdd->dev)) { > > + device_destroy(watchdog_class, devno); > > + wdd->dev = NULL; > > Oddly enough this leaves wdd->dev in place if it is an ERR_PTR. > While that was the case before, it might be better and more > consistent to always set it to NULL. > > I also wonder if it is really necessary to call watchdog_dev_unregister() > first. Something like > > destroy_device: > device_destroy(watchdog_class, devno); > unregister_watchdog: > wdd->dev = NULL; > watchdog_dev_unregister(wdd); > > would be cleaner. Wonder why it isn't done in that order to start with. > It should be possible; after all, the device is created only after > watchdog_dev_register() succeeded, so one should think that it makes > sense to remove it first. Any idea ? No idea why the device_destroy was done after watchdog_dev_unregister in __watchdog_unregister_device, I think there is no reasons for that, and it seems safe to reorder them. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] watchdog: core: factorize register error paths 2015-11-24 22:40 ` Damien Riegel @ 2015-11-24 23:23 ` Guenter Roeck 0 siblings, 0 replies; 4+ messages in thread From: Guenter Roeck @ 2015-11-24 23:23 UTC (permalink / raw) To: Damien Riegel, linux-watchdog, Wim Van Sebroeck, kernel On Tue, Nov 24, 2015 at 05:40:14PM -0500, Damien Riegel wrote: > On Tue, Nov 24, 2015 at 12:45:48PM -0800, Guenter Roeck wrote: > > Hi Damien, > > > > On Tue, Nov 24, 2015 at 03:15:35PM -0500, Damien Riegel wrote: > > > The commit adding the reboot notifier in the core introduced a new error > > > path in __watchdog_register_device, making error paths quite redondant. > > > > > s/redondant/redundant/ > > > > > This commit factorizes all error paths that do some cleanup. > > > > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> > > > --- > > > drivers/watchdog/watchdog_core.c | 27 +++++++++++++++------------ > > > 1 file changed, 15 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > > > index 551af04..aff17b1 100644 > > > --- a/drivers/watchdog/watchdog_core.c > > > +++ b/drivers/watchdog/watchdog_core.c > > > @@ -241,20 +241,16 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > > > wdd->id = id; > > > > > > ret = watchdog_dev_register(wdd); > > > - if (ret) { > > > - ida_simple_remove(&watchdog_ida, id); > > > - return ret; > > > - } > > > + if (ret) > > > + goto remove_ida; > > > } > > > > > > 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; > > > + goto unregister_device; > > > } > > > > > > if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) { > > > @@ -264,11 +260,7 @@ 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); > > > - ida_simple_remove(&watchdog_ida, wdd->id); > > > - wdd->dev = NULL; > > > - return ret; > > > + goto unregister_device; > > > } > > > } > > > > > > @@ -282,6 +274,17 @@ static int __watchdog_register_device(struct watchdog_device *wdd) > > > } > > > > > > return 0; > > > + > > > +unregister_device: > > > + watchdog_dev_unregister(wdd); > > > + if (!IS_ERR(wdd->dev)) { > > > + device_destroy(watchdog_class, devno); > > > + wdd->dev = NULL; > > > > Oddly enough this leaves wdd->dev in place if it is an ERR_PTR. > > While that was the case before, it might be better and more > > consistent to always set it to NULL. > > > > I also wonder if it is really necessary to call watchdog_dev_unregister() > > first. Something like > > > > destroy_device: > > device_destroy(watchdog_class, devno); > > unregister_watchdog: > > wdd->dev = NULL; > > watchdog_dev_unregister(wdd); > > > > would be cleaner. Wonder why it isn't done in that order to start with. > > It should be possible; after all, the device is created only after > > watchdog_dev_register() succeeded, so one should think that it makes > > sense to remove it first. Any idea ? > > No idea why the device_destroy was done after watchdog_dev_unregister in > __watchdog_unregister_device, I think there is no reasons for that, and > it seems safe to reorder them. > Ok, let's do it then. Thanks, Guenter ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-24 23:23 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-24 20:15 [PATCH] watchdog: core: factorize register error paths Damien Riegel 2015-11-24 20:45 ` Guenter Roeck 2015-11-24 22:40 ` Damien Riegel 2015-11-24 23:23 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.