[PATCH] watchdog: core: factorize register error paths

[PATCH] watchdog: core: factorize register error paths

[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

Re: [PATCH] watchdog: core: factorize register error paths

[Guenter Roeck]

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
> 

Re: [PATCH] watchdog: core: factorize register error paths

[Damien Riegel]

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.

Re: [PATCH] watchdog: core: factorize register error paths

[Guenter Roeck]

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