[PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[Linus Walleij]

From: Lee Jones <lee.jones@linaro.org>

Current failure path neglects to mutex_destroy() before returning
an error due to an invalid parameter or an error received from
gpiochip_add(). This patch aims to remedy that behaviour.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/pinctrl-abx500.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
index 426b47c..81ef515 100644
--- a/drivers/pinctrl/pinctrl-abx500.c
+++ b/drivers/pinctrl/pinctrl-abx500.c
@@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
 	default:
 		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
 				(int) platid->driver_data);
+		mutex_destroy(&pct->lock);
 		return -EINVAL;
 	}
 
 	if (!pct->soc) {
 		dev_err(&pdev->dev, "Invalid SOC data\n");
+		mutex_destroy(&pct->lock);
 		return -EINVAL;
 	}
 
@@ -1176,6 +1178,7 @@ static int abx500_gpio_probe(struct platform_device *pdev)
 	ret = gpiochip_add(&pct->chip);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
+		mutex_destroy(&pct->lock);
 		goto out_rem_irq;
 	}
 	dev_info(&pdev->dev, "added gpiochip\n");
-- 
1.7.11.3

Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[Stephen Warren]

On 01/30/2013 12:40 PM, Linus Walleij wrote:
> From: Lee Jones <lee.jones@linaro.org>
> 
> Current failure path neglects to mutex_destroy() before returning
> an error due to an invalid parameter or an error received from
> gpiochip_add(). This patch aims to remedy that behaviour.

> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c

> @@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
>  	default:
>  		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
>  				(int) platid->driver_data);
> +		mutex_destroy(&pct->lock);
>  		return -EINVAL;

Especially given there's already a label out_free which performs this
mutex_destroy(), those last two lines would be better as:

ret = -EINVAL;
goto out_free;

>  	}
>  
>  	if (!pct->soc) {
>  		dev_err(&pdev->dev, "Invalid SOC data\n");
> +		mutex_destroy(&pct->lock);
>  		return -EINVAL;

Same there.

>  	}
>  
> @@ -1176,6 +1178,7 @@ static int abx500_gpio_probe(struct platform_device *pdev)
>  	ret = gpiochip_add(&pct->chip);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
> +		mutex_destroy(&pct->lock);
>  		goto out_rem_irq;

And here, just change the goto target to out_free rather than adding the
mutex_destroy() call.

Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[Lee Jones]

On Wed, 30 Jan 2013, Stephen Warren wrote:

> On 01/30/2013 12:40 PM, Linus Walleij wrote:
> > From: Lee Jones <lee.jones@linaro.org>
> > 
> > Current failure path neglects to mutex_destroy() before returning
> > an error due to an invalid parameter or an error received from
> > gpiochip_add(). This patch aims to remedy that behaviour.
> 
> > diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
> 
> > @@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
> >  	default:
> >  		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
> >  				(int) platid->driver_data);
> > +		mutex_destroy(&pct->lock);
> >  		return -EINVAL;
> 
> Especially given there's already a label out_free which performs this
> mutex_destroy(), those last two lines would be better as:
> 
> ret = -EINVAL;
> goto out_free;

Yes, that's one way of doing it. I figured it was 6 of one and half a
dozen of the other to be honest.

Either I:

 +           mutex_destroy(&pct->lock);

Or:

 +           ret = -EINVAL; 
 +           goto out_free;
 -           return -EINVAL;

I figured the smallest diff would be best. To be honest, I'm not
bothered either way. If it offends you, I can do it the other way, no
problem. Just let me know quick, so I can get the fixed up patch to
Linus.

NB: There is no 'out_free:' at this point, it has already been
removed.

I'll leave it up to you.

> >  	}
> >  
> >  	if (!pct->soc) {
> >  		dev_err(&pdev->dev, "Invalid SOC data\n");
> > +		mutex_destroy(&pct->lock);
> >  		return -EINVAL;
> 
> Same there.
> 
> >  	}
> >  
> > @@ -1176,6 +1178,7 @@ static int abx500_gpio_probe(struct platform_device *pdev)
> >  	ret = gpiochip_add(&pct->chip);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "unable to add gpiochip: %d\n", ret);
> > +		mutex_destroy(&pct->lock);
> >  		goto out_rem_irq;
> 
> And here, just change the goto target to out_free rather than adding the
> mutex_destroy() call.


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[Stephen Warren]

On 01/31/2013 01:01 AM, Lee Jones wrote:
> On Wed, 30 Jan 2013, Stephen Warren wrote:
> 
>> On 01/30/2013 12:40 PM, Linus Walleij wrote:
>>> From: Lee Jones <lee.jones@linaro.org>
>>>
>>> Current failure path neglects to mutex_destroy() before returning
>>> an error due to an invalid parameter or an error received from
>>> gpiochip_add(). This patch aims to remedy that behaviour.
>>
>>> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
>>
>>> @@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
>>>  	default:
>>>  		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
>>>  				(int) platid->driver_data);
>>> +		mutex_destroy(&pct->lock);
>>>  		return -EINVAL;
>>
>> Especially given there's already a label out_free which performs this
>> mutex_destroy(), those last two lines would be better as:
>>
>> ret = -EINVAL;
>> goto out_free;
> 
> Yes, that's one way of doing it. I figured it was 6 of one and half a
> dozen of the other to be honest.
> 
> Either I:
> 
>  +           mutex_destroy(&pct->lock);
> 
> Or:
> 
>  +           ret = -EINVAL; 
>  +           goto out_free;
>  -           return -EINVAL;
> 
> I figured the smallest diff would be best. To be honest, I'm not
> bothered either way. If it offends you, I can do it the other way, no
> problem. Just let me know quick, so I can get the fixed up patch to
> Linus.
> 
> NB: There is no 'out_free:' at this point, it has already been
> removed.

Where has it been removed? Both the latest linux-next and LinusW's
pinctrl tree on git.kernel.org still contain it...

The style in that code is clearly "goto foo" for error-handling, and
makes for smaller simpler code, so I don't see why the label would be
removed.

Still, this review is just a suggestion; this driver isn't anything I
have any ownership of, so I guess feel free to go either way.

Re: [PATCH 4/4] pinctrl/abx500: destroy mutex if returning early due to error

[Lee Jones]

On Thu, 31 Jan 2013, Stephen Warren wrote:

> On 01/31/2013 01:01 AM, Lee Jones wrote:
> > On Wed, 30 Jan 2013, Stephen Warren wrote:
> > 
> >> On 01/30/2013 12:40 PM, Linus Walleij wrote:
> >>> From: Lee Jones <lee.jones@linaro.org>
> >>>
> >>> Current failure path neglects to mutex_destroy() before returning
> >>> an error due to an invalid parameter or an error received from
> >>> gpiochip_add(). This patch aims to remedy that behaviour.
> >>
> >>> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c
> >>
> >>> @@ -1155,11 +1155,13 @@ static int abx500_gpio_probe(struct platform_device *pdev)
> >>>  	default:
> >>>  		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
> >>>  				(int) platid->driver_data);
> >>> +		mutex_destroy(&pct->lock);
> >>>  		return -EINVAL;
> >>
> >> Especially given there's already a label out_free which performs this
> >> mutex_destroy(), those last two lines would be better as:
> >>
> >> ret = -EINVAL;
> >> goto out_free;
> > 
> > Yes, that's one way of doing it. I figured it was 6 of one and half a
> > dozen of the other to be honest.
> > 
> > Either I:
> > 
> >  +           mutex_destroy(&pct->lock);
> > 
> > Or:
> > 
> >  +           ret = -EINVAL; 
> >  +           goto out_free;
> >  -           return -EINVAL;
> > 
> > I figured the smallest diff would be best. To be honest, I'm not
> > bothered either way. If it offends you, I can do it the other way, no
> > problem. Just let me know quick, so I can get the fixed up patch to
> > Linus.
> > 
> > NB: There is no 'out_free:' at this point, it has already been
> > removed.
> 
> Where has it been removed? Both the latest linux-next and LinusW's
> pinctrl tree on git.kernel.org still contain it...

It's an ordering thing. I submitted a 13 patch patch-set to Linus
which completely reworks the driver. One of the first things I did was
to remove all IRQ handling from the driver and pass responsibility over
to the AB8500 core driver. However, there were some issues with some
of the patches, so Linus decided it would be better to get the simple
stuff out of the way whilst I fixup the more complex IRQ stuff. This
was one of the simple patches which was located at the 'end' of the
patch-set. I don't think you've seen the other stuff yet.

> The style in that code is clearly "goto foo" for error-handling, and
> makes for smaller simpler code, so I don't see why the label would be
> removed.
> 
> Still, this review is just a suggestion; this driver isn't anything I
> have any ownership of, so I guess feel free to go either way.

I too am easy and would be happy to go either way. The implementation
now only has one goto for post gpiochip_add() stuff. As I say, the
others have now been removed as they pertained to IRQ stuff.

Sorry for any confusion.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog