[PATCH] regulator: core: Request GPIO before creating sysfs entries

[PATCH] regulator: core: Request GPIO before creating sysfs entries

[Krzysztof Adamski]

regulator_attr_is_visible (which is a .is_visible callback of
regulator_dev_group attribute_grpup) checks rdev->ena_pin to decide if
"status" file should be present in sysfs. This field is set at the end
of regulator_ena_gpio_request so it has to be called before
device_register() otherwise this test will always fail, causing "status"
file to not be visible.

Since regulator_attr_is_visible also tests for is_enabled() op, this
problem is only visible for regulators that does not define this
callback, like regulator-fixed.c.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
---
 drivers/regulator/core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4405be1..6ee9ba4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3913,6 +3913,16 @@ regulator_register(const struct regulator_desc *regulator_desc,
 			goto clean;
 	}
 
+	if ((config->ena_gpio || config->ena_gpio_initialized) &&
+	    gpio_is_valid(config->ena_gpio)) {
+		ret = regulator_ena_gpio_request(rdev, config);
+		if (ret != 0) {
+			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
+				 config->ena_gpio, ret);
+			goto wash;
+		}
+	}
+
 	/* register with sysfs */
 	rdev->dev.class = &regulator_class;
 	rdev->dev.parent = dev;
@@ -3926,16 +3936,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if ((config->ena_gpio || config->ena_gpio_initialized) &&
-	    gpio_is_valid(config->ena_gpio)) {
-		ret = regulator_ena_gpio_request(rdev, config);
-		if (ret != 0) {
-			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
-				 config->ena_gpio, ret);
-			goto wash;
-		}
-	}
-
 	/* set regulator constraints */
 	if (init_data)
 		constraints = &init_data->constraints;
-- 
2.1.4

RE: regulator: core: Request GPIO before creating sysfs entries

[Jon Hunter]

Hi Mark, Krzysztof,

It appears that commit daad134d6649 ("regulator: core: Request GPIO
before creating sysfs entries") breaks boot on tegra124-nyan-big in
-next today.

Looking at the change, it does not appear that the exit path has been
updated correctly and so if a regulator is deferred then there is a
crash in the exit path. I am not sure that there is a simple way to
workaround this because of fix from commit 53032dafc6b9 ("regulator
core: fix double-free in regulator_register() error path") unless we
move regulator_ena_gpio_free() into regulator_dev_release().

Cheers
Jon

Re: regulator: core: Request GPIO before creating sysfs entries

[Krzysztof Adamski]

On Tue, Feb 23, 2016 at 01:22:38PM +0000, Jon Hunter wrote:
>Hi Mark, Krzysztof,
>
>It appears that commit daad134d6649 ("regulator: core: Request GPIO
>before creating sysfs entries") breaks boot on tegra124-nyan-big in
>-next today.
>
>Looking at the change, it does not appear that the exit path has been
>updated correctly and so if a regulator is deferred then there is a
>crash in the exit path. I am not sure that there is a simple way to
>workaround this because of fix from commit 53032dafc6b9 ("regulator
>core: fix double-free in regulator_register() error path") unless we
>move regulator_ena_gpio_free() into regulator_dev_release().

Hi Jon,

You are totally right about the problem but I can't see why changing 
"goto wash" to "goto clean" in error path of regulator_ena_gpio_request 
is not enough. What am I missing?

Best regards,
Krzysztof Adamski

Re: regulator: core: Request GPIO before creating sysfs entries

[Jon Hunter]

Hi Krzysztof,

On 23/02/16 13:56, Krzysztof Adamski wrote:
> On Tue, Feb 23, 2016 at 01:22:38PM +0000, Jon Hunter wrote:
>> Hi Mark, Krzysztof,
>>
>> It appears that commit daad134d6649 ("regulator: core: Request GPIO
>> before creating sysfs entries") breaks boot on tegra124-nyan-big in
>> -next today.
>>
>> Looking at the change, it does not appear that the exit path has been
>> updated correctly and so if a regulator is deferred then there is a
>> crash in the exit path. I am not sure that there is a simple way to
>> workaround this because of fix from commit 53032dafc6b9 ("regulator
>> core: fix double-free in regulator_register() error path") unless we
>> move regulator_ena_gpio_free() into regulator_dev_release().
> 
> Hi Jon,
> 
> You are totally right about the problem but I can't see why changing
> "goto wash" to "goto clean" in error path of regulator_ena_gpio_request
> is not enough. What am I missing?

So device_unregister() will call regulator_dev_release() which will free
rdev (see commit 53032dafc6b9). However, rdev is needed by
regulator_ena_gpio_free() and therefore, we need to call
regulator_ena_gpio_free() before we call device_unregister(). So
swapping the order will cause other problems.

One possibility would be to move regulator_ena_gpio_free() into
regulator_dev_release(). If did we could remove the
regulator_ena_gpio_free() from regulator_unregister(). However, I have
not looked at that in great detail. May be Mark can comment if that
would be ok.

Cheers
Jon

Re: regulator: core: Request GPIO before creating sysfs entries

[Krzysztof Adamski]

On Tue, Feb 23, 2016 at 02:15:21PM +0000, Jon Hunter wrote:
>Hi Krzysztof,
>
>On 23/02/16 13:56, Krzysztof Adamski wrote:
>> On Tue, Feb 23, 2016 at 01:22:38PM +0000, Jon Hunter wrote:
>>> Hi Mark, Krzysztof,
>>>
>>> It appears that commit daad134d6649 ("regulator: core: Request GPIO
>>> before creating sysfs entries") breaks boot on tegra124-nyan-big in
>>> -next today.
>>>
>>> Looking at the change, it does not appear that the exit path has been
>>> updated correctly and so if a regulator is deferred then there is a
>>> crash in the exit path. I am not sure that there is a simple way to
>>> workaround this because of fix from commit 53032dafc6b9 ("regulator
>>> core: fix double-free in regulator_register() error path") unless we
>>> move regulator_ena_gpio_free() into regulator_dev_release().
>>
>> Hi Jon,
>>
>> You are totally right about the problem but I can't see why changing
>> "goto wash" to "goto clean" in error path of regulator_ena_gpio_request
>> is not enough. What am I missing?
>
>So device_unregister() will call regulator_dev_release() which will free
>rdev (see commit 53032dafc6b9). However, rdev is needed by
>regulator_ena_gpio_free() and therefore, we need to call
>regulator_ena_gpio_free() before we call device_unregister(). So
>swapping the order will cause other problems.
>
>One possibility would be to move regulator_ena_gpio_free() into
>regulator_dev_release(). If did we could remove the
>regulator_ena_gpio_free() from regulator_unregister(). However, I have
>not looked at that in great detail. May be Mark can comment if that
>would be ok.

But in case of regulator_ena_gpio_free() failure, we shouldn't call 
device_unregister() at all, since it was not yet registered and that is 
exactly what "clean" does.

Best regards,
Krzysztof Adamski

[PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Krzysztof Adamski]

This problem was introduced by:
commit daad134d6649 ("regulator: core: Request GPIO before creating
sysfs entries")

The error path was not updated correctly and in case
regulator_ena_gpio_free failed, device_unregister was called even though
it was not registered yet.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/regulator/core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ee9ba4..d1e7859 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
-			goto wash;
+			goto clean;
 		}
 	}
 
@@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	ret = set_machine_constraints(rdev, constraints);
 	if (ret < 0)
-		goto scrub;
+		goto wash;
 
 	if (init_data && init_data->supply_regulator)
 		rdev->supply_name = init_data->supply_regulator;
@@ -3972,10 +3972,8 @@ out:
 unset_supplies:
 	unset_regulator_supplies(rdev);
 
-scrub:
-	regulator_ena_gpio_free(rdev);
-
 wash:
+	regulator_ena_gpio_free(rdev);
 	device_unregister(&rdev->dev);
 	/* device core frees rdev */
 	rdev = ERR_PTR(ret);
-- 
2.1.4

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Jon Hunter]

On 23/02/16 14:47, Krzysztof Adamski wrote:
> This problem was introduced by:
> commit daad134d6649 ("regulator: core: Request GPIO before creating
> sysfs entries")
> 
> The error path was not updated correctly and in case
> regulator_ena_gpio_free failed, device_unregister was called even though
> it was not registered yet.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>

Nit ... I think that order of the above should be reversed.

> ---
>  drivers/regulator/core.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ee9ba4..d1e7859 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  		if (ret != 0) {
>  			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>  				 config->ena_gpio, ret);
> -			goto wash;
> +			goto clean;
>  		}
>  	}
>  
> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  
>  	ret = set_machine_constraints(rdev, constraints);
>  	if (ret < 0)
> -		goto scrub;
> +		goto wash;
>  
>  	if (init_data && init_data->supply_regulator)
>  		rdev->supply_name = init_data->supply_regulator;
> @@ -3972,10 +3972,8 @@ out:
>  unset_supplies:
>  	unset_regulator_supplies(rdev);
>  
> -scrub:
> -	regulator_ena_gpio_free(rdev);
> -
>  wash:
> +	regulator_ena_gpio_free(rdev);
>  	device_unregister(&rdev->dev);
>  	/* device core frees rdev */
>  	rdev = ERR_PTR(ret);

What about the case where device_register() fails? I think you still
call clean and so you will leak the gpio?

Jon

Re: regulator: core: Request GPIO before creating sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Tue, Feb 23, 2016 at 01:22:38PM +0000, Jon Hunter wrote:

> It appears that commit daad134d6649 ("regulator: core: Request GPIO
> before creating sysfs entries") breaks boot on tegra124-nyan-big in
> -next today.

Please when reporting problems provide some concrete description of the
problem you are trying to report, for example a boot log.  This makes it
much easier to understand the problem you are trying to report.  I see
there is more information further down the thread but it's good to get
this stuff in the initial report.

> Looking at the change, it does not appear that the exit path has been
> updated correctly and so if a regulator is deferred then there is a
> crash in the exit path. I am not sure that there is a simple way to

Again, please be specific about the analysis you have done - in this
case what specifically is the crash?

> workaround this because of fix from commit 53032dafc6b9 ("regulator
> core: fix double-free in regulator_register() error path") unless we
> move regulator_ena_gpio_free() into regulator_dev_release().

That seems like a sensible idea, do you forsee a problem doing that?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Krzysztof Adamski]

On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote:
>
>On 23/02/16 14:47, Krzysztof Adamski wrote:
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
>> Reported-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
>Nit ... I think that order of the above should be reversed.
>

Couldn't find any reference stating proper order of those tags and 
briefly looking at other commit messages shows this order as quite 
common.

>> ---
>>  drivers/regulator/core.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 6ee9ba4..d1e7859 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>  		if (ret != 0) {
>>  			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>>  				 config->ena_gpio, ret);
>> -			goto wash;
>> +			goto clean;
>>  		}
>>  	}
>>
>> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>
>>  	ret = set_machine_constraints(rdev, constraints);
>>  	if (ret < 0)
>> -		goto scrub;
>> +		goto wash;
>>
>>  	if (init_data && init_data->supply_regulator)
>>  		rdev->supply_name = init_data->supply_regulator;
>> @@ -3972,10 +3972,8 @@ out:
>>  unset_supplies:
>>  	unset_regulator_supplies(rdev);
>>
>> -scrub:
>> -	regulator_ena_gpio_free(rdev);
>> -
>>  wash:
>> +	regulator_ena_gpio_free(rdev);
>>  	device_unregister(&rdev->dev);
>>  	/* device core frees rdev */
>>  	rdev = ERR_PTR(ret);
>
>What about the case where device_register() fails? I think you still
>call clean and so you will leak the gpio?
>
>Jon
>
True. I couldn't find anything more clever than calling 
regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting 
it inside of regulator_dev_release() won't entirely fix the problem 
either as this won't be called in this particular case 
(device_register() fail). I personally still prefer calling 
regulator_ena_gpio_free() inside of regulator_register insted of 
deffering it to regulator_dev_release() as it seems to be clearer to me.

Best regards,
Krzysztof Adamski

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Krzysztof Adamski]

On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote:
>
>On 23/02/16 14:47, Krzysztof Adamski wrote:
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
>> Reported-by: Jon Hunter <jonathanh@nvidia.com>
>
>Nit ... I think that order of the above should be reversed.
>

Couldn't find any reference stating proper order of those tags and 
briefly looking at other commit messages shows this order as quite 
common.

>> ---
>>  drivers/regulator/core.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index 6ee9ba4..d1e7859 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>  		if (ret != 0) {
>>  			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>>  				 config->ena_gpio, ret);
>> -			goto wash;
>> +			goto clean;
>>  		}
>>  	}
>>
>> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>>
>>  	ret = set_machine_constraints(rdev, constraints);
>>  	if (ret < 0)
>> -		goto scrub;
>> +		goto wash;
>>
>>  	if (init_data && init_data->supply_regulator)
>>  		rdev->supply_name = init_data->supply_regulator;
>> @@ -3972,10 +3972,8 @@ out:
>>  unset_supplies:
>>  	unset_regulator_supplies(rdev);
>>
>> -scrub:
>> -	regulator_ena_gpio_free(rdev);
>> -
>>  wash:
>> +	regulator_ena_gpio_free(rdev);
>>  	device_unregister(&rdev->dev);
>>  	/* device core frees rdev */
>>  	rdev = ERR_PTR(ret);
>
>What about the case where device_register() fails? I think you still
>call clean and so you will leak the gpio?
>
>Jon
>
True. I couldn't find anything more clever than calling 
regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting 
it inside of regulator_dev_release() won't entirely fix the problem 
either as this won't be called in this particular case 
(device_register() fail). I personally still prefer calling 
regulator_ena_gpio_free() inside of regulator_register insted of 
deffering it to regulator_dev_release() as it seems to be clearer to me.

Best regards,
Krzysztof Adamski

[PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Krzysztof Adamski]

This problem was introduced by:
commit daad134d6649 ("regulator: core: Request GPIO before creating
sysfs entries")

The error path was not updated correctly and in case
regulator_ena_gpio_free failed, device_unregister was called even though
it was not registered yet.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
Reported-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/regulator/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ee9ba4..055f8c1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
-			goto wash;
+			goto clean;
 		}
 	}
 
@@ -3931,7 +3931,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
-		goto clean;
+		goto wash;
 	}
 
 	dev_set_drvdata(&rdev->dev, rdev);
@@ -3974,13 +3974,13 @@ unset_supplies:
 
 scrub:
 	regulator_ena_gpio_free(rdev);
-
-wash:
 	device_unregister(&rdev->dev);
 	/* device core frees rdev */
 	rdev = ERR_PTR(ret);
 	goto out;
 
+wash:
+	regulator_ena_gpio_free(rdev);
 clean:
 	kfree(rdev);
 	rdev = ERR_PTR(ret);
-- 
2.1.4

[PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Krzysztof Adamski]

This problem was introduced by:
commit daad134d6649 ("regulator: core: Request GPIO before creating
sysfs entries")

The error path was not updated correctly and in case
regulator_ena_gpio_free failed, device_unregister was called even though
it was not registered yet.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/regulator/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ee9ba4..055f8c1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
-			goto wash;
+			goto clean;
 		}
 	}
 
@@ -3931,7 +3931,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
-		goto clean;
+		goto wash;
 	}
 
 	dev_set_drvdata(&rdev->dev, rdev);
@@ -3974,13 +3974,13 @@ unset_supplies:
 
 scrub:
 	regulator_ena_gpio_free(rdev);
-
-wash:
 	device_unregister(&rdev->dev);
 	/* device core frees rdev */
 	rdev = ERR_PTR(ret);
 	goto out;
 
+wash:
+	regulator_ena_gpio_free(rdev);
 clean:
 	kfree(rdev);
 	rdev = ERR_PTR(ret);
-- 
2.1.4

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Jon Hunter]

On 24/02/16 08:26, Krzysztof Adamski wrote:
> On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote:
>>
>> On 23/02/16 14:47, Krzysztof Adamski wrote:
>>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
>>> Reported-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> Nit ... I think that order of the above should be reversed.
>>
> 
> Couldn't find any reference stating proper order of those tags and
> briefly looking at other commit messages shows this order as quite common.
> 
>>> ---
>>>  drivers/regulator/core.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>>> index 6ee9ba4..d1e7859 100644
>>> --- a/drivers/regulator/core.c
>>> +++ b/drivers/regulator/core.c
>>> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc
>>> *regulator_desc,
>>>          if (ret != 0) {
>>>              rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>>>                   config->ena_gpio, ret);
>>> -            goto wash;
>>> +            goto clean;
>>>          }
>>>      }
>>>
>>> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc
>>> *regulator_desc,
>>>
>>>      ret = set_machine_constraints(rdev, constraints);
>>>      if (ret < 0)
>>> -        goto scrub;
>>> +        goto wash;
>>>
>>>      if (init_data && init_data->supply_regulator)
>>>          rdev->supply_name = init_data->supply_regulator;
>>> @@ -3972,10 +3972,8 @@ out:
>>>  unset_supplies:
>>>      unset_regulator_supplies(rdev);
>>>
>>> -scrub:
>>> -    regulator_ena_gpio_free(rdev);
>>> -
>>>  wash:
>>> +    regulator_ena_gpio_free(rdev);
>>>      device_unregister(&rdev->dev);
>>>      /* device core frees rdev */
>>>      rdev = ERR_PTR(ret);
>>
>> What about the case where device_register() fails? I think you still
>> call clean and so you will leak the gpio?
>>
>> Jon
>>
> True. I couldn't find anything more clever than calling
> regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting
> it inside of regulator_dev_release() won't entirely fix the problem
> either as this won't be called in this particular case
> (device_register() fail). I personally still prefer calling
> regulator_ena_gpio_free() inside of regulator_register insted of
> deffering it to regulator_dev_release() as it seems to be clearer to me.

Yes if you were to put regulator_ena_gpio_free() inside the
regulator_dev_release(), you would still need to call
regulator_ena_gpio_free() if the device_register fails. I see the way
you have it now there are two places you call regulator_ena_gpio_free()
in the error path. It is not a big deal, really either way.

Jon

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Jon Hunter]

On 24/02/16 08:27, Krzysztof Adamski wrote:
> This problem was introduced by:
> commit daad134d6649 ("regulator: core: Request GPIO before creating
> sysfs entries")

I think you need to be more specific about what the "problem" is. The
subject does not really explain it either. I would suggest you change
the subject to "fix crash in error path of regulator_register()" as
it is not really regulator_ena_gpio_free() that is a problem.

I would like to point out that this problem breaks the boot on all 
Tegra 32-bit devices because it is quite common for the regulator
registration to be deferred on boot and this will trigger a crash.
So it is very common and I am not sure what other platforms are
impacted by this. Therefore, it would be good to get this fixed soon.

Mark mentioned included a boot log of the crash. Here is one from
Tegra124-TK1, but unfortunately, it is not too useful, although the
crash does happen right after the regulator registration deferral.
Nonetheless I have bisected the kernel and found the commit that was
responsible and verified that reverting it fixes the crash.


[    0.505924] +USB0_VBUS_SW: Failed to request enable GPIO108: -517
[    0.510196] Unable to handle kernel NULL pointer dereference at virtual address 00000044
[    0.518360] pgd = c0004000
[    0.521108] [00000044] *pgd=00000000
[    0.524752] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[    0.530121] Modules linked in:
[    0.533247] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc5-next-20160224-75670-g96aa086 #1
[    0.541995] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
[    0.548322] task: ee85b380 ti: ee85c000 task.ti: ee85c000
[    0.553786] PC is at kernfs_find_ns+0x8/0xf4
[    0.558117] LR is at kernfs_find_and_get_ns+0x30/0x48
[    0.563232] pc : [<c024afe8>]    lr : [<c024b1a4>]    psr: 60000013
[    0.563232] sp : ee85dd4c  ip : 00000001  fp : c0c1857c
[    0.574842] r10: eefeca70  r9 : c0b3b83c  r8 : ee9cdc60
[    0.580129] r7 : c0857730  r6 : 00000000  r5 : 00000000  r4 : c0c10718
[    0.586715] r3 : ee85b380  r2 : 00000000  r1 : c0857730  r0 : 00000000
[    0.593304] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    0.600496] Control: 10c5387d  Table: 8000406a  DAC: 00000051
[    0.606302] Process swapper/0 (pid: 1, stack limit = 0xee85c210)
[    0.612369] Stack: (0xee85dd4c to 0xee85e000)
[    0.616792] dd40:                            c0c10718 00000000 00000000 c0857730 c024b1a4
[    0.625027] dd60: 00000000 c0c2654c ee9cdc68 ee9cdc60 ee9926d0 c024df1c 00000000 c0c26510
[    0.633260] dd80: ee9cdc68 c0439938 ee9cdc60 ee9cdc60 00000000 c042f9d0 00000000 c01a927c
[    0.641493] dda0: 00000008 ee9926d0 ee9c9490 ee9cdc60 fffffdfb c0c18730 ee9926d0 c042fb80
[    0.649727] ddc0: ee99dc10 c03abe90 00000002 ee9a0210 ee9ccb50 ee800000 c03add80 ee9c9bc0
[    0.657960] dde0: ee9cdc60 00000001 eefeca70 c01eb2f8 ee85de34 ee85de34 ee9c9410 ee9a0210
[    0.666193] de00: ee99dc10 ee9a0200 c0b3b83c eefeca70 00000000 c03add98 ee9ccb50 ee9c94d0
[    0.674426] de20: ee99dc10 00000000 ee9a0210 c03aebb4 00000000 ee9a0210 ee9ccb50 ee99dc10
[    0.682660] de40: eefeca70 00000000 00000001 0000006c 00000000 00000008 ee9a0210 fffffffe
[    0.690893] de60: ee9a0210 fffffdfb c0c188b8 c0c188b8 00000000 c0434080 c0434030 ee9a0210
[    0.699126] de80: c0caa264 c0caa26c 00000000 c0432aac 00000000 ee9a0210 c0c188b8 ee9a0244
[    0.707360] dea0: 00000000 c0b1bc2c 00000000 c0432c14 00000000 c0c188b8 c0432b68 c04310d0
[    0.715593] dec0: ee81a55c ee997b34 c0c188b8 ee9c6700 c0c25f68 c04320a0 c09cc320 c0c188b8
[    0.723826] dee0: c0c065c8 c0c188b8 c0c065c8 ee9bbdc0 c0c60000 c0433410 c0433d14 c0c065c8
[    0.732059] df00: c0c065c8 c0101768 c0802898 00000014 00000000 00000000 c0c0c400 c0c0c488
[    0.740293] df20: 00000000 c0c0a998 efffcc66 c0820554 00000101 c01378d4 00000000 c099ce50
[    0.748526] df40: c0a70794 00000000 00000004 00000004 c0c0a960 efffcc00 c0b4f748 00000004
[    0.756760] df60: c0b3b828 c0c60000 00000101 c0b3b83c c0b00598 c0b00dd8 00000004 00000004
[    0.764993] df80: 00000000 c0b00598 00000000 c07d4ec8 00000000 00000000 00000000 00000000
[    0.773226] dfa0: 00000000 c07d4ed0 00000000 c0107938 00000000 00000000 00000000 00000000
[    0.781459] dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.789693] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 dfffffff ffffbfff
[    0.797933] [<c024afe8>] (kernfs_find_ns) from [<ee9cdc68>] (0xee9cdc68)
[    0.804687] Code: e5832028 e8bd8038 e92d40f0 e1a06002 (e1d024b4) 
[    0.810868] ---[ end trace 7d0e5449506e9287 ]---

 
> The error path was not updated correctly and in case
> regulator_ena_gpio_free failed, device_unregister was called even though
> it was not registered yet.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/regulator/core.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 6ee9ba4..055f8c1 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  		if (ret != 0) {
>  			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
>  				 config->ena_gpio, ret);
> -			goto wash;
> +			goto clean;
>  		}
>  	}
>  
> @@ -3931,7 +3931,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  	ret = device_register(&rdev->dev);
>  	if (ret != 0) {
>  		put_device(&rdev->dev);
> -		goto clean;
> +		goto wash;
>  	}
>  
>  	dev_set_drvdata(&rdev->dev, rdev);
> @@ -3974,13 +3974,13 @@ unset_supplies:
>  
>  scrub:
>  	regulator_ena_gpio_free(rdev);
> -
> -wash:
>  	device_unregister(&rdev->dev);
>  	/* device core frees rdev */
>  	rdev = ERR_PTR(ret);
>  	goto out;
>  
> +wash:
> +	regulator_ena_gpio_free(rdev);
>  clean:
>  	kfree(rdev);
>  	rdev = ERR_PTR(ret);
> 

Otherwise ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Jon Hunter]

On 24/02/16 08:27, Krzysztof Adamski wrote:
> This problem was introduced by:
> commit daad134d6649 ("regulator: core: Request GPIO before creating
> sysfs entries")
> 
> The error path was not updated correctly and in case
> regulator_ena_gpio_free failed, device_unregister was called even though
> it was not registered yet.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
> Reported-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Make sure you tag the patch as V2, etc, so that Mark knows which version
to pick up.

Jon

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Jon Hunter]

On 24/02/16 08:27, Krzysztof Adamski wrote:
> This problem was introduced by:
> commit daad134d6649 ("regulator: core: Request GPIO before creating
> sysfs entries")
> 
> The error path was not updated correctly and in case
> regulator_ena_gpio_free failed, device_unregister was called even though
> it was not registered yet.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>

Make sure you tag the patch as V2, etc, so that Mark knows which version
to pick up.

Jon

[PATCH v3] regulator: core: fix crash in error path of regulator_register

[Krzysztof Adamski]

This problem was introduced by:
commit daad134d6649 ("regulator: core: Request GPIO before creating
sysfs entries")

The error path was not updated correctly after moving GPIO registration
code and in case regulator_ena_gpio_free failed, device_unregister() was
called even though device_register() was not yet called.

This problem breaks the boot at least on all Tegra 32-bit devices. It
will also crash each device that specifices GPIO that is unavaiable at
regulator_register call. Here's error log I've got when forced GPIO to
be invalid:

[    1.116612] usb-otg-vbus-reg: Failed to request enable GPIO10: -22
[    1.122794] Unable to handle kernel NULL pointer dereference at
virtual address 00000044
[    1.130894] pgd = c0004000
[    1.133598] [00000044] *pgd=00000000
[    1.137205] Internal error: Oops: 5 [#1] SMP ARM

and here's backtrace from KDB:

Exception stack(0xef11fbd0 to 0xef11fc18)
fbc0:                                     00000000 c0738a14 00000000 00000000
fbe0: c0b2a0b0 00000000 00000000 c0738a14 c0b5fdf8 00000001 ef7f6074 ef11fc4c
fc00: ef11fc50 ef11fc20 c02a8344 c02a7f1c 60000013 ffffffff
[<c010cee0>] (__dabt_svc) from [<c02a7f1c>] (kernfs_find_ns+0x18/0xf8)
[<c02a7f1c>] (kernfs_find_ns) from [<c02a8344>] (kernfs_find_and_get_ns+0x40/0x58)
[<c02a8344>] (kernfs_find_and_get_ns) from [<c02ac4a4>] (sysfs_unmerge_group+0x28/0x68)
[<c02ac4a4>] (sysfs_unmerge_group) from [<c044389c>] (dpm_sysfs_remove+0x30/0x5c)
[<c044389c>] (dpm_sysfs_remove) from [<c0436ba8>] (device_del+0x48/0x1f4)
[<c0436ba8>] (device_del) from [<c0436d84>] (device_unregister+0x30/0x6c)
[<c0436d84>] (device_unregister) from [<c0403910>] (regulator_register+0x6d0/0xdac)
[<c0403910>] (regulator_register) from [<c04052d4>] (devm_regulator_register+0x50/0x84)
[<c04052d4>] (devm_regulator_register) from [<c0406298>] (reg_fixed_voltage_probe+0x25c/0x3c0)
[<c0406298>] (reg_fixed_voltage_probe) from [<c043d21c>] (platform_drv_probe+0x60/0xb0)
[<c043d21c>] (platform_drv_probe) from [<c043b078>] (driver_probe_device+0x24c/0x440)
[<c043b078>] (driver_probe_device) from [<c043b5e8>] (__device_attach_driver+0xc0/0x120)
[<c043b5e8>] (__device_attach_driver) from [<c043901c>] (bus_for_each_drv+0x6c/0x98)
[<c043901c>] (bus_for_each_drv) from [<c043ad20>] (__device_attach+0xac/0x138)
[<c043ad20>] (__device_attach) from [<c043b664>] (device_initial_probe+0x1c/0x20)
[<c043b664>] (device_initial_probe) from [<c043a074>] (bus_probe_device+0x94/0x9c)
[<c043a074>] (bus_probe_device) from [<c043a610>] (deferred_probe_work_func+0x80/0xcc)
[<c043a610>] (deferred_probe_work_func) from [<c01381d0>] (process_one_work+0x158/0x454)
[<c01381d0>] (process_one_work) from [<c013854c>] (worker_thread+0x38/0x510)
[<c013854c>] (worker_thread) from [<c013e154>] (kthread+0xe8/0x104)
[<c013e154>] (kthread) from [<c0108638>] (ret_from_fork+0x14/0x3c)

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@tieto.com>
Reported-by: Jon Hunter <jonathanh@nvidia.com>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
---

Changes compared to v2:
- Extended commit message

Changes compared to v1:
- Fixed error path of device_register too, as noticed by Jon


 drivers/regulator/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6ee9ba4..055f8c1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
-			goto wash;
+			goto clean;
 		}
 	}
 
@@ -3931,7 +3931,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
 		put_device(&rdev->dev);
-		goto clean;
+		goto wash;
 	}
 
 	dev_set_drvdata(&rdev->dev, rdev);
@@ -3974,13 +3974,13 @@ unset_supplies:
 
 scrub:
 	regulator_ena_gpio_free(rdev);
-
-wash:
 	device_unregister(&rdev->dev);
 	/* device core frees rdev */
 	rdev = ERR_PTR(ret);
 	goto out;
 
+wash:
+	regulator_ena_gpio_free(rdev);
 clean:
 	kfree(rdev);
 	rdev = ERR_PTR(ret);
-- 
2.1.4

Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Wed, Feb 24, 2016 at 09:20:19AM +0000, Jon Hunter wrote:

> Make sure you tag the patch as V2, etc, so that Mark knows which version
> to pick up.

Versioning is nice but not totally essential.  What really helps is not
burying new patches in the middle of threads, especially if it's a
multi-patch series (not the case here but people do do that).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]