| Message ID | 46a47430b8d65f509d47fe3ad1264c6b23086d61.1442508974.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/17, Viresh Kumar wrote: > +++ b/drivers/base/power/opp.c > @@ -889,13 +889,22 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, > /* TODO: Support multiple regulators */ > static int opp_get_microvolt(struct dev_pm_opp *opp, struct device *dev) > { > + struct property *prop; > u32 microvolt[3] = {0}; > int count, ret; > > - count = of_property_count_u32_elems(opp->np, "opp-microvolt"); > - if (!count) > + /* Missing property isn't a problem, but an invalid entry is */ > + prop = of_find_property(opp->np, "opp-microvolt", NULL); Prop isn't used anywhere so why not remove the local variable and test the result of this call inside the if condition? > + if (!prop) > return 0; > > + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); > + if (count < 0) { We can't test count for -EINVAL to detect the missing property because -EINVAL is also returned on a non-multiple of u32 length property? Maybe we shouldn't worry about that case and turn -EINVAL into 0. > + dev_err(dev, "%s: Invalid opp-microvolt property (%d)\n", > + __func__, count); > + return count;
On 17-09-15, 11:13, Stephen Boyd wrote: > > + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); > > + if (count < 0) { > > We can't test count for -EINVAL to detect the missing property > because -EINVAL is also returned on a non-multiple of u32 length > property? Maybe we shouldn't worry about that case and turn > -EINVAL into 0. So you are saying that we go ahead without regulators if a incorrect values are present in opp-microvolt? i.e. even if the length property was invalid, we return 0 from this function. The problem here is that we will try changing the frequency without changing the regulator in that case, and it might not be safe for the platform, isn't it?
On 09/18, Viresh Kumar wrote: > On 17-09-15, 11:13, Stephen Boyd wrote: > > > + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); > > > + if (count < 0) { > > > > We can't test count for -EINVAL to detect the missing property > > because -EINVAL is also returned on a non-multiple of u32 length > > property? Maybe we shouldn't worry about that case and turn > > -EINVAL into 0. > > So you are saying that we go ahead without regulators if a incorrect > values are present in opp-microvolt? i.e. even if the length property > was invalid, we return 0 from this function. > > The problem here is that we will try changing the frequency without > changing the regulator in that case, and it might not be safe for the > platform, isn't it? > Do we care if a platform has changed the length of the property to something that isn't a multiple of u32? That sounds very rare, that's all. I agree it's a bug.
On 19-09-15, 15:22, Stephen Boyd wrote: > On 09/18, Viresh Kumar wrote: > > On 17-09-15, 11:13, Stephen Boyd wrote: > > > > + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); > > > > + if (count < 0) { > > > > > > We can't test count for -EINVAL to detect the missing property > > > because -EINVAL is also returned on a non-multiple of u32 length > > > property? Maybe we shouldn't worry about that case and turn > > > -EINVAL into 0. > > > > So you are saying that we go ahead without regulators if a incorrect > > values are present in opp-microvolt? i.e. even if the length property > > was invalid, we return 0 from this function. > > > > The problem here is that we will try changing the frequency without > > changing the regulator in that case, and it might not be safe for the > > platform, isn't it? > > > > Do we care if a platform has changed the length of the property > to something that isn't a multiple of u32? That sounds very rare, > that's all. I agree it's a bug. Hmm, okay.. I got it now. Maybe we can update of_property_count_u32_elems() to return zero in that case, but that might not be the appropriate return value. It *can* be confused against the case where the user has written an empty property. But even in that case we are returning -ENODATA instead of 0 :)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c index 28cd75c535b0..2048164d6c53 100644 --- a/drivers/base/power/opp.c +++ b/drivers/base/power/opp.c @@ -889,13 +889,22 @@ static int _opp_add_dynamic(struct device *dev, unsigned long freq, /* TODO: Support multiple regulators */ static int opp_get_microvolt(struct dev_pm_opp *opp, struct device *dev) { + struct property *prop; u32 microvolt[3] = {0}; int count, ret; - count = of_property_count_u32_elems(opp->np, "opp-microvolt"); - if (!count) + /* Missing property isn't a problem, but an invalid entry is */ + prop = of_find_property(opp->np, "opp-microvolt", NULL); + if (!prop) return 0; + count = of_property_count_u32_elems(opp->np, "opp-microvolt"); + if (count < 0) { + dev_err(dev, "%s: Invalid opp-microvolt property (%d)\n", + __func__, count); + return count; + } + /* There can be one or three elements here */ if (count != 1 && count != 3) { dev_err(dev, "%s: Invalid number of elements in opp-microvolt property (%d)\n",
of_property_count_u32_elems() will never return 0, but a -ve error value of a positive count. And so the current !count check is wrong. Also, a missing "opp-microvolt" property isn't a problem and so we need to do of_find_property() separately to confirm that. Fixes: 274659029c9d ("PM / OPP: Add support to parse "operating-points-v2" bindings") Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- For 4.3. drivers/base/power/opp.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)