[PATCH] BUG: pinmux: release only taken pins on error

[PATCH] BUG: pinmux: release only taken pins on error

[Richard Genoud]

commit e38d457de7be63e6ced1ea254aa51466deb1fef0
pinctrl: pinmux: Release all taken pins in pinmux_enable_setting

Introduced a bug in the release pin mechanism.
All the pins (taken or not) where released.
For instance, if a i2c function has already taken pins 5 and 6.
And the pins of function PHY are requested (pins 3 4 5 6 7).
The pins 3 and 4 will be taken, pin 5 is already taken, so the function
fails.
And we have pins 3 and 4 release, which is ok. But also pins 5 and 6 !.
And also pin 7 (which will have its mux_usecount to -1...)

This patch reset the original behaviour.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/pinmux.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 1a00658..917e830 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -409,6 +409,8 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting)
 			dev_err(pctldev->dev,
 				"could not request pin %d on device %s\n",
 				pins[i], pinctrl_dev_get_name(pctldev));
+			/* On error release *only* taken pins */
+			num_pins = i - 1; /* this pin just failed */
 			goto err_pin_request;
 		}
 	}
-- 
1.7.2.5

[PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Richard Genoud]

If pin_free is called on a pin already freed, mux_usecount is set to
UINT_MAX which is really a bad idea.
This will silently ignore a double call to pin_free

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/pinctrl/pinmux.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 917e830..9fd55f9 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -194,8 +194,9 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	}
 
 	if (!gpio_range) {
-		desc->mux_usecount--;
-		if (desc->mux_usecount)
+		if (1 == desc->mux_usecount)
+			desc->mux_usecount = 0;
+		else
 			return NULL;
 	}
 
-- 
1.7.2.5

Re: [PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Stephen Warren]

On 03/20/2013 05:31 AM, Richard Genoud wrote:
> If pin_free is called on a pin already freed, mux_usecount is set to
> UINT_MAX which is really a bad idea.
> This will silently ignore a double call to pin_free

Shouldn't we WARN_ON(this case)?

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

>  	if (!gpio_range) {
> -		desc->mux_usecount--;
> -		if (desc->mux_usecount)
> +		if (1 == desc->mux_usecount)
> +			desc->mux_usecount = 0;
> +		else
>  			return NULL;

What if desc-mux_usecount was 2; this patch prevents the use-count from
being decremented to 1 in this case. Shouldn't this be:

  	if (!gpio_range) {
+ 		if (WARN_ON(!desc->mux_usecount))
+ 			return NULL;
  		desc->mux_usecount--;

Re: [PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Richard Genoud]

2013/3/20 Stephen Warren <swarren@wwwdotorg.org>:
> On 03/20/2013 05:31 AM, Richard Genoud wrote:
>> If pin_free is called on a pin already freed, mux_usecount is set to
>> UINT_MAX which is really a bad idea.
>> This will silently ignore a double call to pin_free
>
> Shouldn't we WARN_ON(this case)?
yes indeed, it may be better to issue a big warning because AFAIK
that's not normal.

>
>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>
>>       if (!gpio_range) {
>> -             desc->mux_usecount--;
>> -             if (desc->mux_usecount)
>> +             if (1 == desc->mux_usecount)
>> +                     desc->mux_usecount = 0;
>> +             else
>>                       return NULL;
>
> What if desc-mux_usecount was 2; this patch prevents the use-count from
> being decremented to 1 in this case. Shouldn't this be:
>
>         if (!gpio_range) {
> +               if (WARN_ON(!desc->mux_usecount))
> +                       return NULL;
>                 desc->mux_usecount--;
>
Well, I'm not very familiar with this code, but can mux_usecount be
higher than 1 ?

Re: [PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Stephen Warren]

On 03/20/2013 10:59 AM, Richard Genoud wrote:
> 2013/3/20 Stephen Warren <swarren@wwwdotorg.org>:
>> On 03/20/2013 05:31 AM, Richard Genoud wrote:
>>> If pin_free is called on a pin already freed, mux_usecount is set to
>>> UINT_MAX which is really a bad idea.
>>> This will silently ignore a double call to pin_free
>>
>> Shouldn't we WARN_ON(this case)?
> yes indeed, it may be better to issue a big warning because AFAIK
> that's not normal.
> 
>>
>>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>>
>>>       if (!gpio_range) {
>>> -             desc->mux_usecount--;
>>> -             if (desc->mux_usecount)
>>> +             if (1 == desc->mux_usecount)
>>> +                     desc->mux_usecount = 0;
>>> +             else
>>>                       return NULL;
>>
>> What if desc-mux_usecount was 2; this patch prevents the use-count from
>> being decremented to 1 in this case. Shouldn't this be:
>>
>>         if (!gpio_range) {
>> +               if (WARN_ON(!desc->mux_usecount))
>> +                       return NULL;
>>                 desc->mux_usecount--;
>
> Well, I'm not very familiar with this code, but can mux_usecount be
> higher than 1 ?

Possibly not, but isn't that more something that the
resource-acquisition code (i.e. whatever does mux_usecount++) should
enforce; the cleanup code should probably support all cases in case the
enforcement rules change in the future. Either that, or convert the
field to a bool so it's clear what the range is.

[PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Richard Genoud]

If pin_free is called on a pin already freed, mux_usecount is set to
UINT_MAX which is really a bad idea.
This will issue a warning, so that we can correct the code responsible
for the double free.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
Ok Stephen, your idea (and code) seems better.
I signed-off it, but it's really yours...
Feel free to transform my Signed-off-by in a Tested-by

 drivers/pinctrl/pinmux.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 1a00658..bd83c8b 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -194,6 +194,11 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	}
 
 	if (!gpio_range) {
+		/*
+		 * A pin should not be freed more times than allocated.
+		 */
+		if (WARN_ON(!desc->mux_usecount))
+			return NULL;
 		desc->mux_usecount--;
 		if (desc->mux_usecount)
 			return NULL;
-- 
1.7.2.5

Re: [PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Stephen Warren]

On 03/21/2013 05:21 AM, Richard Genoud wrote:
> If pin_free is called on a pin already freed, mux_usecount is set to
> UINT_MAX which is really a bad idea.
> This will issue a warning, so that we can correct the code responsible
> for the double free.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
> Ok Stephen, your idea (and code) seems better.
> I signed-off it, but it's really yours...
> Feel free to transform my Signed-off-by in a Tested-by

In this situation, I think you'd often say:
Suggested-by: Stephen Warren <swarren@nvidia.com>

FWIW,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

Re: [PATCH] BUG: pinmux: forbid mux_usecount to be set at UINT_MAX

[Linus Walleij]

On Thu, Mar 21, 2013 at 12:21 PM, Richard Genoud
<richard.genoud@gmail.com> wrote:

> If pin_free is called on a pin already freed, mux_usecount is set to
> UINT_MAX which is really a bad idea.
> This will issue a warning, so that we can correct the code responsible
> for the double free.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

This version of the patch applied with Stephen's Reviewed-by
and BUG prefix stripped, it's applied to the fixes branch for
the v3.9 release candidates.

Yours,
Linus Walleij

[PATCH] BUG: [RFC] pinctrl: pins are freed 2 times in pinctrl_bind_pins

[Richard Genoud]

If the function pinctrl_select_state() fails because one pin is already
taken elsewhere, pinmux_enable_setting makes all the necessary pin_free
calls (and not more than necessary).
The problem here is that devm_pinctrl_put() will be called on the pin
group, and each pin in this group has already been freed.

Example:
If a i2c function has already sucessfully taken pins 5 and 6.
And now, pinctrl_bind_pins() is called for function PHY (pins 3 4 5 6 7).
pinmux_enable_setting() will fail AND call pin_free on necessary pins.
But if devm_pinctrl_put() is called, it will call again pin_free on pins
3 4 5 6 7.
So, the pins 5 and 6 will be released (and pins 3 4 7 double freed).
Which means that even if the i2c function has claim the pins, they will
be available for other functions.

This patch simply doesn't call devm_pinctrl_put when
pinctrl_select_state fails, but I'm not sure it's the right thing to do.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/base/pinctrl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/pinctrl.c b/drivers/base/pinctrl.c
index 67a274e..537406d 100644
--- a/drivers/base/pinctrl.c
+++ b/drivers/base/pinctrl.c
@@ -45,7 +45,7 @@ int pinctrl_bind_pins(struct device *dev)
 	ret = pinctrl_select_state(dev->pins->p, dev->pins->default_state);
 	if (ret) {
 		dev_dbg(dev, "failed to activate default pinctrl state\n");
-		goto cleanup_get;
+		goto cleanup_alloc; /* pins already un-muxed */
 	}
 
 	return 0;
-- 
1.7.2.5

Re: [PATCH] BUG: [RFC] pinctrl: pins are freed 2 times in pinctrl_bind_pins

[Stephen Warren]

On 03/20/2013 05:31 AM, Richard Genoud wrote:
> If the function pinctrl_select_state() fails because one pin is already
> taken elsewhere, pinmux_enable_setting makes all the necessary pin_free
> calls (and not more than necessary).
> The problem here is that devm_pinctrl_put() will be called on the pin
> group, and each pin in this group has already been freed.
> 
> Example:
> If a i2c function has already sucessfully taken pins 5 and 6.
> And now, pinctrl_bind_pins() is called for function PHY (pins 3 4 5 6 7).
> pinmux_enable_setting() will fail AND call pin_free on necessary pins.
> But if devm_pinctrl_put() is called, it will call again pin_free on pins
> 3 4 5 6 7.
> So, the pins 5 and 6 will be released (and pins 3 4 7 double freed).
> Which means that even if the i2c function has claim the pins, they will
> be available for other functions.
> 
> This patch simply doesn't call devm_pinctrl_put when
> pinctrl_select_state fails, but I'm not sure it's the right thing to do.

The correct fix here is not to skip the call to devm_pinctrl_put(),
since that undoes a lot of other things besides the current state selection.

Instead, pinctrl_select_state_locked() needs to be fixed so that:

a)

Change "p->state = state;" to "p->state = NULL;" or similar, to indicate
that no state is selected. (Please validate if a NULL value in that
variable will cause problems elsewhere)

b)

Add back the assignment "p->state = state;" at the end of the function,
if no error occurred.

c)

Fix the list_for_each_entry() call that applies all the settings for the
new state so that if it fails, it undoes everything that it's applied so
far. That's the hard part, unless there's a
list_for_each_entry_before_the_current_one_that_list_for_each_entry_iterated_over_already()
macro!

Re: [PATCH] BUG: [RFC] pinctrl: pins are freed 2 times in pinctrl_bind_pins

[Richard Genoud]

2013/3/20 Stephen Warren <swarren@wwwdotorg.org>:
> On 03/20/2013 05:31 AM, Richard Genoud wrote:
>> If the function pinctrl_select_state() fails because one pin is already
>> taken elsewhere, pinmux_enable_setting makes all the necessary pin_free
>> calls (and not more than necessary).
>> The problem here is that devm_pinctrl_put() will be called on the pin
>> group, and each pin in this group has already been freed.
>>
>> Example:
>> If a i2c function has already sucessfully taken pins 5 and 6.
>> And now, pinctrl_bind_pins() is called for function PHY (pins 3 4 5 6 7).
>> pinmux_enable_setting() will fail AND call pin_free on necessary pins.
>> But if devm_pinctrl_put() is called, it will call again pin_free on pins
>> 3 4 5 6 7.
>> So, the pins 5 and 6 will be released (and pins 3 4 7 double freed).
>> Which means that even if the i2c function has claim the pins, they will
>> be available for other functions.
>>
>> This patch simply doesn't call devm_pinctrl_put when
>> pinctrl_select_state fails, but I'm not sure it's the right thing to do.
>
> The correct fix here is not to skip the call to devm_pinctrl_put(),
> since that undoes a lot of other things besides the current state selection.
>
> Instead, pinctrl_select_state_locked() needs to be fixed so that:
>
> a)
>
> Change "p->state = state;" to "p->state = NULL;" or similar, to indicate
> that no state is selected. (Please validate if a NULL value in that
> variable will cause problems elsewhere)
>
> b)
>
> Add back the assignment "p->state = state;" at the end of the function,
> if no error occurred.
>
> c)
>
> Fix the list_for_each_entry() call that applies all the settings for the
> new state so that if it fails, it undoes everything that it's applied so
> far. That's the hard part, unless there's a
> list_for_each_entry_before_the_current_one_that_list_for_each_entry_iterated_over_already()
> macro!
ok, I'll look into that.
Thanks for your advices !

PS: with its 90chars long, your macro won't please checkpatch.pl ! :)

Regards,
Richard

Re: [PATCH] BUG: pinmux: release only taken pins on error

[Axel Lin]

2013/3/20 Richard Genoud <richard.genoud@gmail.com>:
> commit e38d457de7be63e6ced1ea254aa51466deb1fef0
> pinctrl: pinmux: Release all taken pins in pinmux_enable_setting
>
> Introduced a bug in the release pin mechanism.
> All the pins (taken or not) where released.
> For instance, if a i2c function has already taken pins 5 and 6.
> And the pins of function PHY are requested (pins 3 4 5 6 7).
> The pins 3 and 4 will be taken, pin 5 is already taken, so the function
> fails.
> And we have pins 3 and 4 release, which is ok. But also pins 5 and 6 !.
> And also pin 7 (which will have its mux_usecount to -1...)
>
> This patch reset the original behaviour.
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/pinctrl/pinmux.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 1a00658..917e830 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -409,6 +409,8 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting)
>                         dev_err(pctldev->dev,
>                                 "could not request pin %d on device %s\n",
>                                 pins[i], pinctrl_dev_get_name(pctldev));
> +                       /* On error release *only* taken pins */
> +                       num_pins = i - 1; /* this pin just failed */
This seems pointless, in the goto err_pin_request path, the code does
not use num_pins variable at all.

>                         goto err_pin_request;

see the code in err_pin_request,
it actually does "while (--i>-0", so it won't call pin_free for the
just failed pin.

err_pin_request:
        /* On error release all taken pins */
        while (--i >= 0)
                pin_free(pctldev, pins[i], NULL);

        return ret;

Re: [PATCH] BUG: pinmux: release only taken pins on error

[Richard Genoud]

2013/3/20 Axel Lin <axel.lin@ingics.com>:
> 2013/3/20 Richard Genoud <richard.genoud@gmail.com>:
>> commit e38d457de7be63e6ced1ea254aa51466deb1fef0
>> pinctrl: pinmux: Release all taken pins in pinmux_enable_setting
>>
>> Introduced a bug in the release pin mechanism.
>> All the pins (taken or not) where released.
>> For instance, if a i2c function has already taken pins 5 and 6.
>> And the pins of function PHY are requested (pins 3 4 5 6 7).
>> The pins 3 and 4 will be taken, pin 5 is already taken, so the function
>> fails.
>> And we have pins 3 and 4 release, which is ok. But also pins 5 and 6 !.
>> And also pin 7 (which will have its mux_usecount to -1...)
>>
>> This patch reset the original behaviour.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/pinctrl/pinmux.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>> index 1a00658..917e830 100644
>> --- a/drivers/pinctrl/pinmux.c
>> +++ b/drivers/pinctrl/pinmux.c
>> @@ -409,6 +409,8 @@ int pinmux_enable_setting(struct pinctrl_setting const *setting)
>>                         dev_err(pctldev->dev,
>>                                 "could not request pin %d on device %s\n",
>>                                 pins[i], pinctrl_dev_get_name(pctldev));
>> +                       /* On error release *only* taken pins */
>> +                       num_pins = i - 1; /* this pin just failed */
> This seems pointless, in the goto err_pin_request path, the code does
> not use num_pins variable at all.
>
>>                         goto err_pin_request;
>
> see the code in err_pin_request,
> it actually does "while (--i>-0", so it won't call pin_free for the
> just failed pin.
>
> err_pin_request:
>         /* On error release all taken pins */
>         while (--i >= 0)
>                 pin_free(pctldev, pins[i], NULL);
>
>         return ret;

arrrrgg !
it's only Wednesday, and I'm already doing such mistakes !
sorry for that...
This patch can be tossed away with no regret.

The other 2 patches are better I guess