[PATCH] PM / OPP: Initialize regulator pointer to an error value

[PATCH] PM / OPP: Initialize regulator pointer to an error value

[Viresh Kumar]

We are currently required to do two checks for regulator pointer:
IS_ERR() and IS_NULL().

And multiple instances are reported, about both of these not being used
consistently and so resulting in crashes.

Fix that by initializing regulator pointer with an error value and
checking it only against an error.

This makes code consistent and efficient.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp/core.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index d7cd4e265766..146b6197d598 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -257,7 +257,7 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
 	}
 
 	reg = dev_opp->regulator;
-	if (IS_ERR_OR_NULL(reg)) {
+	if (IS_ERR(reg)) {
 		/* Regulator may not be required for device */
 		if (reg)
 			dev_err(dev, "%s: Invalid regulator (%ld)\n", __func__,
@@ -798,6 +798,9 @@ static struct device_opp *_add_device_opp(struct device *dev)
 		of_node_put(np);
 	}
 
+	/* Set regulator to a non-NULL error value */
+	dev_opp->regulator = ERR_PTR(-EFAULT);
+
 	/* Find clk for the device */
 	dev_opp->clk = clk_get(dev, NULL);
 	if (IS_ERR(dev_opp->clk)) {
@@ -845,7 +848,7 @@ static void _remove_device_opp(struct device_opp *dev_opp)
 	if (dev_opp->prop_name)
 		return;
 
-	if (!IS_ERR_OR_NULL(dev_opp->regulator))
+	if (!IS_ERR(dev_opp->regulator))
 		return;
 
 	/* Release clk */
@@ -975,7 +978,7 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 {
 	struct regulator *reg = dev_opp->regulator;
 
-	if (!IS_ERR_OR_NULL(reg) &&
+	if (!IS_ERR(reg) &&
 	    !regulator_is_supported_voltage(reg, opp->u_volt_min,
 					    opp->u_volt_max)) {
 		pr_warn("%s: OPP minuV: %lu maxuV: %lu, not supported by regulator\n",
@@ -1435,7 +1438,7 @@ int dev_pm_opp_set_regulator(struct device *dev, const char *name)
 	}
 
 	/* Already have a regulator set */
-	if (WARN_ON(!IS_ERR_OR_NULL(dev_opp->regulator))) {
+	if (WARN_ON(!IS_ERR(dev_opp->regulator))) {
 		ret = -EBUSY;
 		goto err;
 	}
@@ -1486,7 +1489,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
 		goto unlock;
 	}
 
-	if (IS_ERR_OR_NULL(dev_opp->regulator)) {
+	if (IS_ERR(dev_opp->regulator)) {
 		dev_err(dev, "%s: Doesn't have regulator set\n", __func__);
 		goto unlock;
 	}
@@ -1495,7 +1498,7 @@ void dev_pm_opp_put_regulator(struct device *dev)
 	WARN_ON(!list_empty(&dev_opp->opp_list));
 
 	regulator_put(dev_opp->regulator);
-	dev_opp->regulator = ERR_PTR(-EINVAL);
+	dev_opp->regulator = ERR_PTR(-EFAULT);
 
 	/* Try freeing device_opp if this was the last blocking resource */
 	_remove_device_opp(dev_opp);
-- 
2.7.1.410.g6faf27b

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Jon Hunter]

On 15/02/16 16:26, Viresh Kumar wrote:
> We are currently required to do two checks for regulator pointer:
> IS_ERR() and IS_NULL().
> 
> And multiple instances are reported, about both of these not being used
> consistently and so resulting in crashes.
> 
> Fix that by initializing regulator pointer with an error value and
> checking it only against an error.
> 
> This makes code consistent and efficient.
> 
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

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

Thanks
Jon

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Viresh Kumar]

On 15-02-16, 16:42, Jon Hunter wrote:
> 
> On 15/02/16 16:26, Viresh Kumar wrote:
> > We are currently required to do two checks for regulator pointer:
> > IS_ERR() and IS_NULL().
> > 
> > And multiple instances are reported, about both of these not being used
> > consistently and so resulting in crashes.
> > 
> > Fix that by initializing regulator pointer with an error value and
> > checking it only against an error.
> > 
> > This makes code consistent and efficient.
> > 
> > Reported-by: Jon Hunter <jonathanh@nvidia.com>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks a lot Jon.

-- 
viresh

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Arnd Bergmann]

On Monday 15 February 2016 21:56:42 Viresh Kumar wrote:
> We are currently required to do two checks for regulator pointer:
> IS_ERR() and IS_NULL().
> 
> And multiple instances are reported, about both of these not being used
> consistently and so resulting in crashes.
> 
> Fix that by initializing regulator pointer with an error value and
> checking it only against an error.
> 
> This makes code consistent and efficient.

There is usually something else wrong if you have to check for both.
Why exactly do you need to check for both IS_ERR and NULL?

> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> index d7cd4e265766..146b6197d598 100644
> --- a/drivers/base/power/opp/core.c
> +++ b/drivers/base/power/opp/core.c
> @@ -257,7 +257,7 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
>  	}
>  
>  	reg = dev_opp->regulator;
> -	if (IS_ERR_OR_NULL(reg)) {
> +	if (IS_ERR(reg)) {
>  		/* Regulator may not be required for device */
>  		if (reg)
>  			dev_err(dev, "%s: Invalid regulator (%ld)\n", __func__,
> @@ -798,6 +798,9 @@ static struct device_opp *_add_device_opp(struct device *dev)
>  		of_node_put(np);
>  	}
>  
> +	/* Set regulator to a non-NULL error value */
> +	dev_opp->regulator = ERR_PTR(-EFAULT);
> +
>  	/* Find clk for the device */
>  	dev_opp->clk = clk_get(dev, NULL);
>  	if (IS_ERR(dev_opp->clk)) {

-EFAULT has a very specific meaning (accessing an invalid pointer from
user space), I don't think you want that one.

	Arnd

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Rafael J. Wysocki]

On Mon, Feb 15, 2016 at 9:38 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 15 February 2016 21:56:42 Viresh Kumar wrote:
>> We are currently required to do two checks for regulator pointer:
>> IS_ERR() and IS_NULL().
>>
>> And multiple instances are reported, about both of these not being used
>> consistently and so resulting in crashes.
>>
>> Fix that by initializing regulator pointer with an error value and
>> checking it only against an error.
>>
>> This makes code consistent and efficient.
>
> There is usually something else wrong if you have to check for both.
> Why exactly do you need to check for both IS_ERR and NULL?
>
>> diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
>> index d7cd4e265766..146b6197d598 100644
>> --- a/drivers/base/power/opp/core.c
>> +++ b/drivers/base/power/opp/core.c
>> @@ -257,7 +257,7 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
>>       }
>>
>>       reg = dev_opp->regulator;
>> -     if (IS_ERR_OR_NULL(reg)) {
>> +     if (IS_ERR(reg)) {
>>               /* Regulator may not be required for device */
>>               if (reg)
>>                       dev_err(dev, "%s: Invalid regulator (%ld)\n", __func__,
>> @@ -798,6 +798,9 @@ static struct device_opp *_add_device_opp(struct device *dev)
>>               of_node_put(np);
>>       }
>>
>> +     /* Set regulator to a non-NULL error value */
>> +     dev_opp->regulator = ERR_PTR(-EFAULT);
>> +
>>       /* Find clk for the device */
>>       dev_opp->clk = clk_get(dev, NULL);
>>       if (IS_ERR(dev_opp->clk)) {
>
> -EFAULT has a very specific meaning (accessing an invalid pointer from
> user space), I don't think you want that one.

Yeah, agreed.

That should be something like -ENXIO IMO.

Thanks,
Rafael

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Viresh Kumar]

On 15-02-16, 22:13, Rafael J. Wysocki wrote:
> That should be something like -ENXIO IMO.

Thanks for modifying and applying the patch :)

-- 
viresh

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Rafael J. Wysocki]

On Tuesday, February 16, 2016 06:17:16 AM Viresh Kumar wrote:
> On 15-02-16, 22:13, Rafael J. Wysocki wrote:
> > That should be something like -ENXIO IMO.
> 
> Thanks for modifying and applying the patch :)

Well, you're welcome.

I wanted it to be fixed in the tomorrow's linux-next so people can boot their
systems again.

Thanks,
Rafael

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Viresh Kumar]

Cc'ing Mark as well.

On 15-02-16, 21:38, Arnd Bergmann wrote:
> There is usually something else wrong if you have to check for both.
> Why exactly do you need to check for both IS_ERR and NULL?

And here is the reasoning behind it:
- It is normally said that 'NULL' is a valid clk. The same is
  applicable to regulators as well, right? At least, that is what
  below says:

  commit 4a511de96d69 ("cpufreq: cpufreq-cpu0: NULL is a valid
  regulator")

- And so I left the regulator pointer to NULL in OPP core.
- But then I realized that its not safe to call many regulator core
  APIs with NULL regulator, as those caused the crashes reported by
  multiple people now.
- clk APIs guarantee that they return early when NULL clk is passed to
  them.
- Do we need to do the same for regulator core as well ?

- And so I initialized the pointer to an error value now, as
  initializing it to NULL (possibly a valid regulator, in theory)
  isn't the right thing to do.

> > diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
> > index d7cd4e265766..146b6197d598 100644
> > --- a/drivers/base/power/opp/core.c
> > +++ b/drivers/base/power/opp/core.c
> > @@ -257,7 +257,7 @@ unsigned long dev_pm_opp_get_max_volt_latency(struct device *dev)
> >  	}
> >  
> >  	reg = dev_opp->regulator;
> > -	if (IS_ERR_OR_NULL(reg)) {
> > +	if (IS_ERR(reg)) {
> >  		/* Regulator may not be required for device */
> >  		if (reg)
> >  			dev_err(dev, "%s: Invalid regulator (%ld)\n", __func__,
> > @@ -798,6 +798,9 @@ static struct device_opp *_add_device_opp(struct device *dev)
> >  		of_node_put(np);
> >  	}
> >  
> > +	/* Set regulator to a non-NULL error value */
> > +	dev_opp->regulator = ERR_PTR(-EFAULT);
> > +
> >  	/* Find clk for the device */
> >  	dev_opp->clk = clk_get(dev, NULL);
> >  	if (IS_ERR(dev_opp->clk)) {
> 
> -EFAULT has a very specific meaning (accessing an invalid pointer from
> user space), I don't think you want that one.

Sorry, wasn't aware of those requirements. What Rafael suggested is
the right thing to do then.

-- 
viresh

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Mark Brown]

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

On Tue, Feb 16, 2016 at 06:30:59AM +0530, Viresh Kumar wrote:

> - And so I left the regulator pointer to NULL in OPP core.
> - But then I realized that its not safe to call many regulator core
>   APIs with NULL regulator, as those caused the crashes reported by
>   multiple people now.
> - clk APIs guarantee that they return early when NULL clk is passed to
>   them.
> - Do we need to do the same for regulator core as well ?

No, NULL is explicitly not something you can substitute in,
essentially all the users are just not bothering to implement error
checking and we don't want to encourage that.  The set of use cases
where we legitimately have optional supplies is very small, much smaller
than clocks, because it makes the electrical engineering a lot harder.

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

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Arnd Bergmann]

On Tuesday 16 February 2016 01:56:16 Mark Brown wrote:
> On Tue, Feb 16, 2016 at 06:30:59AM +0530, Viresh Kumar wrote:
> 
> > - And so I left the regulator pointer to NULL in OPP core.
> > - But then I realized that its not safe to call many regulator core
> >   APIs with NULL regulator, as those caused the crashes reported by
> >   multiple people now.
> > - clk APIs guarantee that they return early when NULL clk is passed to
> >   them.
> > - Do we need to do the same for regulator core as well ?
> 
> No, NULL is explicitly not something you can substitute in,
> essentially all the users are just not bothering to implement error
> checking and we don't want to encourage that.  The set of use cases
> where we legitimately have optional supplies is very small, much smaller
> than clocks, because it makes the electrical engineering a lot harder.
> 

I must have misinterpreted the idea behind that API as well then.

>From this function definition:

/*
 * Make sure client drivers will still build on systems with no software
 * controllable voltage or current regulators.
 */             
static inline struct regulator *__must_check regulator_get(struct device *dev,
        const char *id)
{       
        /* Nothing except the stubbed out regulator API should be
         * looking at the value except to check if it is an error
         * value. Drivers are free to handle NULL specifically by
         * skipping all regulator API calls, but they don't have to.
         * Drivers which don't, should make sure they properly handle
         * corner cases of the API, such as regulator_get_voltage()
         * returning 0.
         */             
        return NULL;
}

my reading was that the expected behavior in any driver was:

* call regulator_get()
* if IS_ERR(), fail device probe function, never use invalid
  pointer other than PTR_ERR()
* if NULL, and regulator is required, fail probe so we never
  use the regulator
* if NULL, and regulators are optional, continue with the NULL
  value.
* drivers never look into the regulator pointer, and only
  pass it into regulator APIs which can cope with the NULL
  value when CONFIG_REGULATOR is disabled.

That would be similar to what we have for clocks. Which part of
my interpretation is wrong?

	Arnd

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Mark Brown]

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

On Tue, Feb 16, 2016 at 10:10:44AM +0100, Arnd Bergmann wrote:
> On Tuesday 16 February 2016 01:56:16 Mark Brown wrote:

> > No, NULL is explicitly not something you can substitute in,
> > essentially all the users are just not bothering to implement error
> > checking and we don't want to encourage that.  The set of use cases
> > where we legitimately have optional supplies is very small, much smaller
> > than clocks, because it makes the electrical engineering a lot harder.

> I must have misinterpreted the idea behind that API as well then.

> From this function definition:

> static inline struct regulator *__must_check regulator_get(struct device *dev,
>         const char *id)
> {       
>         /* Nothing except the stubbed out regulator API should be
>          * looking at the value except to check if it is an error
>          * value. Drivers are free to handle NULL specifically by
>          * skipping all regulator API calls, but they don't have to.
>          * Drivers which don't, should make sure they properly handle
>          * corner cases of the API, such as regulator_get_voltage()
>          * returning 0.
>          */             
>         return NULL;
> }

This is the stubbed regulator API which is only ever used with the stub
regulator API, it uses NULL to give a non-error pointer it can return to
well written callers so they don't know they are running with the stubs.
We are explicitly using NULL because callers should treat it as a valid
regulator.

> my reading was that the expected behavior in any driver was:

> * call regulator_get()
> * if IS_ERR(), fail device probe function, never use invalid
>   pointer other than PTR_ERR()
> * if NULL, and regulator is required, fail probe so we never
>   use the regulator

No, drivers should never look at the value of the pointer other than to
check it for error.  If there is a problem of any kind an error will be
returned.

> * if NULL, and regulators are optional, continue with the NULL
>   value.

No, we always return an error pointer if we fail to get a regulator.
The difference with optional regulators is in how we handle the
situation where we have full constraints and a regulator is not mapped
in, normally we assume there must be one with no software control but we
need to work around buggy bindings as the device would be non-functional
without power.

> * drivers never look into the regulator pointer, and only
>   pass it into regulator APIs which can cope with the NULL
>   value when CONFIG_REGULATOR is disabled.

> That would be similar to what we have for clocks. Which part of
> my interpretation is wrong?

See above.

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

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Arnd Bergmann]

On Tuesday 16 February 2016 13:11:08 Mark Brown wrote:
> On Tue, Feb 16, 2016 at 10:10:44AM +0100, Arnd Bergmann wrote:
> > On Tuesday 16 February 2016 01:56:16 Mark Brown wrote:
> 
> > > No, NULL is explicitly not something you can substitute in,
> > > essentially all the users are just not bothering to implement error
> > > checking and we don't want to encourage that.  The set of use cases
> > > where we legitimately have optional supplies is very small, much smaller
> > > than clocks, because it makes the electrical engineering a lot harder.
> 
> > I must have misinterpreted the idea behind that API as well then.
> 
> > From this function definition:
> 
> > static inline struct regulator *__must_check regulator_get(struct device *dev,
> >         const char *id)
> > {       
> >         /* Nothing except the stubbed out regulator API should be
> >          * looking at the value except to check if it is an error
> >          * value. Drivers are free to handle NULL specifically by
> >          * skipping all regulator API calls, but they don't have to.
> >          * Drivers which don't, should make sure they properly handle
> >          * corner cases of the API, such as regulator_get_voltage()
> >          * returning 0.
> >          */             
> >         return NULL;
> > }
> 
> This is the stubbed regulator API which is only ever used with the stub
> regulator API, it uses NULL to give a non-error pointer it can return to
> well written callers so they don't know they are running with the stubs.
> We are explicitly using NULL because callers should treat it as a valid
> regulator.

Right, that is what I understood.

> > my reading was that the expected behavior in any driver was:
> 
> > * call regulator_get()
> > * if IS_ERR(), fail device probe function, never use invalid
> >   pointer other than PTR_ERR()
> > * if NULL, and regulator is required, fail probe so we never
> >   use the regulator
> 
> No, drivers should never look at the value of the pointer other than to
> check it for error.  If there is a problem of any kind an error will be
> returned.
>
> > * if NULL, and regulators are optional, continue with the NULL
> >   value.
> 
> No, we always return an error pointer if we fail to get a regulator.
> The difference with optional regulators is in how we handle the
> situation where we have full constraints and a regulator is not mapped
> in, normally we assume there must be one with no software control but we
> need to work around buggy bindings as the device would be non-functional
> without power.

Sorry, I should not have said "optional" here, which has a specific
meaning in the API. I meant a driver that can work with either
CONFIG_REGULATOR enabled or disabled (which is something slightly
different).

I guess a driver needing to know whether regulators are built-in
should check 'if (IS_ENABLED(CONFIG_REGULATOR))' rather than
checking the return code for NULL.

	Arnd

Re: [PATCH] PM / OPP: Initialize regulator pointer to an error value

[Mark Brown]

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

On Tue, Feb 16, 2016 at 04:12:39PM +0100, Arnd Bergmann wrote:
> On Tuesday 16 February 2016 13:11:08 Mark Brown wrote:

> > No, we always return an error pointer if we fail to get a regulator.
> > The difference with optional regulators is in how we handle the
> > situation where we have full constraints and a regulator is not mapped
> > in, normally we assume there must be one with no software control but we
> > need to work around buggy bindings as the device would be non-functional
> > without power.

> Sorry, I should not have said "optional" here, which has a specific
> meaning in the API. I meant a driver that can work with either
> CONFIG_REGULATOR enabled or disabled (which is something slightly
> different).

Ah, right!

> I guess a driver needing to know whether regulators are built-in
> should check 'if (IS_ENABLED(CONFIG_REGULATOR))' rather than
> checking the return code for NULL.

Yes, that's the expected interface here if anyone does need to check
although for most users it's expected that the stubs will be sufficient
and they don't need to check at all.

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