| Message ID | 743509d913cbc0e725bea52281be03b009e02bb5.1455553501.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15-02-16, 22:13, Rafael J. Wysocki wrote:
> That should be something like -ENXIO IMO.
Thanks for modifying and applying the patch :)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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);
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(-)