PM / OPP: of_property_count_u32_elems() can return errors

Message ID 46a47430b8d65f509d47fe3ad1264c6b23086d61.1442508974.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Sept. 17, 2015, 5 p.m.
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(-)

Comments

Stephen Boyd Sept. 17, 2015, 6:13 p.m. | #1
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;
Viresh Kumar Sept. 19, 2015, 3:21 a.m. | #2
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?
Stephen Boyd Sept. 19, 2015, 10:22 p.m. | #3
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.
Viresh Kumar Sept. 22, 2015, 4:35 p.m. | #4
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 :)

Patch hide | download patch | download mbox

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