[PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[Viresh Kumar]

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(-)

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",
-- 
2.4.0

Re: [PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[Stephen Boyd]

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;

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[Viresh Kumar]

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?

-- 
viresh

Re: [PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[Stephen Boyd]

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.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] PM / OPP: of_property_count_u32_elems() can return errors

[Viresh Kumar]

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 :)

-- 
viresh