[PATCH] PM / OPP: Fix NULL pointer dereference crash when disabling OPPs

[PATCH] PM / OPP: Fix NULL pointer dereference crash when disabling OPPs

[Jon Hunter]

Commit 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by
the regulator") disables OPPs that are not supported by the regulator.
This is causes a crash on Tegra124 Jetson TK1 when using the DFLL clock
source for the CPU. The DFLL manages the voltage itself and so there is
no regulator specified for the OPPs and so we get a crash when we try to
dereference the regulator pointer. Fix this by checking to see if the
regulator IS_ERR_OR_NULL before dereferencing it.

Fixes: 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by the
regulator")

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/base/power/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index ab711c2c3e00..d7cd4e265766 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -975,7 +975,7 @@ static bool _opp_supported_by_regulators(struct dev_pm_opp *opp,
 {
 	struct regulator *reg = dev_opp->regulator;
 
-	if (!IS_ERR(reg) &&
+	if (!IS_ERR_OR_NULL(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",
-- 
2.1.4

Re: [PATCH] PM / OPP: Fix NULL pointer dereference crash when disabling OPPs

[Viresh Kumar]

On 11-02-16, 11:25, Jon Hunter wrote:
> Commit 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by
> the regulator") disables OPPs that are not supported by the regulator.
> This is causes a crash on Tegra124 Jetson TK1 when using the DFLL clock
> source for the CPU. The DFLL manages the voltage itself and so there is
> no regulator specified for the OPPs and so we get a crash when we try to
> dereference the regulator pointer. Fix this by checking to see if the
> regulator IS_ERR_OR_NULL before dereferencing it.
> 
> Fixes: 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by the
> regulator")
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/base/power/opp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Sorry about that :(

Acked-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

-- 
viresh

Re: [PATCH] PM / OPP: Fix NULL pointer dereference crash when disabling OPPs

[Viresh Kumar]

On 11-02-16, 11:25, Jon Hunter wrote:
> Commit 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by
> the regulator") disables OPPs that are not supported by the regulator.
> This is causes a crash on Tegra124 Jetson TK1 when using the DFLL clock
> source for the CPU. The DFLL manages the voltage itself and so there is
> no regulator specified for the OPPs and so we get a crash when we try to
> dereference the regulator pointer. Fix this by checking to see if the
> regulator IS_ERR_OR_NULL before dereferencing it.
> 
> Fixes: 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by the
> regulator")
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/base/power/opp/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Sorry about that :(

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH] PM / OPP: Fix NULL pointer dereference crash when disabling OPPs

[Rafael J. Wysocki]

On Thu, Feb 11, 2016 at 12:34 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 11-02-16, 11:25, Jon Hunter wrote:
>> Commit 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by
>> the regulator") disables OPPs that are not supported by the regulator.
>> This is causes a crash on Tegra124 Jetson TK1 when using the DFLL clock
>> source for the CPU. The DFLL manages the voltage itself and so there is
>> no regulator specified for the OPPs and so we get a crash when we try to
>> dereference the regulator pointer. Fix this by checking to see if the
>> regulator IS_ERR_OR_NULL before dereferencing it.
>>
>> Fixes: 7d34d56ef334 ("PM / OPP: Disable OPPs that aren't supported by the
>> regulator")
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/base/power/opp/core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Sorry about that :(
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied, thanks!

Rafael

[PATCH] PM / OPP: Fix NULL pointer dereference crash when setting the OPP

[Jon Hunter]

Commit 6a0712f6f199 ("PM / OPP: Add dev_pm_opp_set_rate()") causes a
crash on Tegra124 Jetson TK1 when using the DFLL clock source for the
CPU.  The DFLL manages the voltage itself and so there is no regulator
specified for the OPPs and so we get a crash when we try to dereference
the regulator pointer.  Fix this by checking to see if the regulator
IS_ERR_OR_NULL before dereferencing it.

Fixes: 6a0712f6f199 ("PM / OPP: Add dev_pm_opp_set_rate()")

Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---

I am not sure why I did not catch this instance of the bug last week
when I submitted the patch to fix the NULL pointer dereference in
_opp_supported_by_regulators(). May be I forgot to go back and test
on HEAD after bisecting? Anyway, both this fix and the one from last
week are necessary to get the kernel booting again on Tegra124 Jetson
TK1.

 drivers/base/power/opp/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index d7cd4e265766..82ad5ae72427 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -564,7 +564,7 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	int ret;
 
 	/* Regulator not available for device */
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_dbg(dev, "%s: regulator not available: %ld\n", __func__,
 			PTR_ERR(reg));
 		return 0;
-- 
2.1.4

Re: [PATCH] PM / OPP: Fix NULL pointer dereference crash when setting the OPP

[Viresh Kumar]

On 15-02-16, 13:59, Jon Hunter wrote:
> Commit 6a0712f6f199 ("PM / OPP: Add dev_pm_opp_set_rate()") causes a
> crash on Tegra124 Jetson TK1 when using the DFLL clock source for the
> CPU.  The DFLL manages the voltage itself and so there is no regulator
> specified for the OPPs and so we get a crash when we try to dereference
> the regulator pointer.  Fix this by checking to see if the regulator
> IS_ERR_OR_NULL before dereferencing it.
> 
> Fixes: 6a0712f6f199 ("PM / OPP: Add dev_pm_opp_set_rate()")
> 
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 
> I am not sure why I did not catch this instance of the bug last week
> when I submitted the patch to fix the NULL pointer dereference in
> _opp_supported_by_regulators(). May be I forgot to go back and test
> on HEAD after bisecting? Anyway, both this fix and the one from last
> week are necessary to get the kernel booting again on Tegra124 Jetson
> TK1.

I want to fix it a bit differently, will send out in a different mail
to make it easy for Rafael to apply it.

-- 
viresh

[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

[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 --]