re: PM / OPP: Add clock-latency-ns support

re: PM / OPP: Add clock-latency-ns support

[Dan Carpenter]

Hello Viresh Kumar,

The patch 3ca9bb33c627: "PM / OPP: Add clock-latency-ns support" from
Jul 29, 2015, leads to the following static checker warning:

	drivers/base/power/opp.c:949 _opp_add_static_v2()
	warn: passing casted pointer '&new_opp->clock_latency_ns' to 'of_property_read_u32()' 64 vs 32.

drivers/base/power/opp.c
   946          new_opp->np = np;
   947          new_opp->dynamic = false;
   948          new_opp->available = true;
   949          of_property_read_u32(np, "clock-latency-ns",
   950                               (u32 *)&new_opp->clock_latency_ns);
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^

This code will break on 64 bit, big endian machines.  I doin't know if
that is an issue for this driver.  I saw this was in the power/
directory and I spent a minute googling to see if PowerPC machines are
big endian...  :P

   951  
   952          ret = opp_get_microvolt(new_opp, dev);
   953          if (ret)
   954                  goto free_opp;
   955  

Also:

	drivers/base/power/opp.c:956 _opp_add_static_v2()
	warn: passing casted pointer '&new_opp->u_amp' to 'of_property_read_u32()' 64 vs 32.

regards,
dan carpenter

Re: PM / OPP: Add clock-latency-ns support

[Viresh Kumar]

On 10-08-15, 19:38, Dan Carpenter wrote:
> Hello Viresh Kumar,
> 
> The patch 3ca9bb33c627: "PM / OPP: Add clock-latency-ns support" from
> Jul 29, 2015, leads to the following static checker warning:
> 
> 	drivers/base/power/opp.c:949 _opp_add_static_v2()
> 	warn: passing casted pointer '&new_opp->clock_latency_ns' to 'of_property_read_u32()' 64 vs 32.
> 
> drivers/base/power/opp.c
>    946          new_opp->np = np;
>    947          new_opp->dynamic = false;
>    948          new_opp->available = true;
>    949          of_property_read_u32(np, "clock-latency-ns",
>    950                               (u32 *)&new_opp->clock_latency_ns);
>                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This code will break on 64 bit, big endian machines.  I doin't know if
> that is an issue for this driver.  I saw this was in the power/
> directory and I spent a minute googling to see if PowerPC machines are
> big endian...  :P
> 
>    951  
>    952          ret = opp_get_microvolt(new_opp, dev);
>    953          if (ret)
>    954                  goto free_opp;
>    955  
> 
> Also:
> 
> 	drivers/base/power/opp.c:956 _opp_add_static_v2()
> 	warn: passing casted pointer '&new_opp->u_amp' to 'of_property_read_u32()' 64 vs 32.

Hi Dan,

The problem is that the value here is of type 'unsigned long' which is
32/64 bit on 32/64 bit machines.

So, I looked at how other places in code has done it, and that's what
I found.

-- 
viresh

Re: PM / OPP: Add clock-latency-ns support

[Dan Carpenter]

On Tue, Aug 11, 2015 at 01:42:28PM +0530, Viresh Kumar wrote:
> On 10-08-15, 19:38, Dan Carpenter wrote:
> > Hello Viresh Kumar,
> > 
> > The patch 3ca9bb33c627: "PM / OPP: Add clock-latency-ns support" from
> > Jul 29, 2015, leads to the following static checker warning:
> > 
> > 	drivers/base/power/opp.c:949 _opp_add_static_v2()
> > 	warn: passing casted pointer '&new_opp->clock_latency_ns' to 'of_property_read_u32()' 64 vs 32.
> > 
> > drivers/base/power/opp.c
> >    946          new_opp->np = np;
> >    947          new_opp->dynamic = false;
> >    948          new_opp->available = true;
> >    949          of_property_read_u32(np, "clock-latency-ns",
> >    950                               (u32 *)&new_opp->clock_latency_ns);
> >                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > This code will break on 64 bit, big endian machines.  I doin't know if
> > that is an issue for this driver.  I saw this was in the power/
> > directory and I spent a minute googling to see if PowerPC machines are
> > big endian...  :P
> > 
> >    951  
> >    952          ret = opp_get_microvolt(new_opp, dev);
> >    953          if (ret)
> >    954                  goto free_opp;
> >    955  
> > 
> > Also:
> > 
> > 	drivers/base/power/opp.c:956 _opp_add_static_v2()
> > 	warn: passing casted pointer '&new_opp->u_amp' to 'of_property_read_u32()' 64 vs 32.
> 
> Hi Dan,
> 
> The problem is that the value here is of type 'unsigned long' which is
> 32/64 bit on 32/64 bit machines.

Yep.  It won't work on 64 bit big endian machines as described earlier.

> 
> So, I looked at how other places in code has done it, and that's what
> I found.

It's not portable.  Sometimes we don't care about that because we know
we don't care about 64 bit big endian systems.  Hence, my email.

regards,
dan carpenter

Re: PM / OPP: Add clock-latency-ns support

[Stephen Boyd]

On 08/11, Dan Carpenter wrote:
> On Tue, Aug 11, 2015 at 01:42:28PM +0530, Viresh Kumar wrote:
> > 
> > The problem is that the value here is of type 'unsigned long' which is
> > 32/64 bit on 32/64 bit machines.
> 
> Yep.  It won't work on 64 bit big endian machines as described earlier.
> 
> > 
> > So, I looked at how other places in code has done it, and that's what
> > I found.
> 
> It's not portable.  Sometimes we don't care about that because we know
> we don't care about 64 bit big endian systems.  Hence, my email.
> 

Making it portable should be simple enough by having a temporary
variable of type u32 though.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---8<---
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 51b220e615d3..022715d1894c 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -919,6 +919,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	struct dev_pm_opp *new_opp;
 	u64 rate;
 	int ret;
+	u32 val;
 
 	/* Hold our list modification lock here */
 	mutex_lock(&dev_opp_list_lock);
@@ -946,14 +947,15 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	new_opp->np = np;
 	new_opp->dynamic = false;
 	new_opp->available = true;
-	of_property_read_u32(np, "clock-latency-ns",
-			     (u32 *)&new_opp->clock_latency_ns);
+	of_property_read_u32(np, "clock-latency-ns", &val);
+	new_opp->clock_latency_ns = val;
 
 	ret = opp_get_microvolt(new_opp, dev);
 	if (ret)
 		goto free_opp;
 
-	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
+	of_property_read_u32(np, "opp-microamp", &val);
+	new_opp->u_amp = val;
 
 	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)

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

Re: PM / OPP: Add clock-latency-ns support

[Viresh Kumar]

On 11-08-15, 11:54, Stephen Boyd wrote:
> Making it portable should be simple enough by having a temporary
> variable of type u32 though.

Right.

@Dan: Does this look fine to you?

Message-Id: <40b3ad3c99c5fe1c50d997ba5418dd602c673f13.1439362565.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 12 Aug 2015 12:20:49 +0530
Subject: [PATCH] PM / OPP: Fix static checker warning (broken 64bit big endian
 systems)

Dan Carpenter reported (generated with static checker):

drivers/base/power/opp.c:949 _opp_add_static_v2()
warn: passing casted pointer '&new_opp->clock_latency_ns' to
'of_property_read_u32()' 64 vs 32.

This code will break on 64 bit, big endian machines.

Fix this by reading the value in a u32 type variable first and then
assigning it to the unsigned long variable.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Suggested-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/opp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index 204c6c945168..a9e0af0dd9e5 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -918,6 +918,7 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	struct device_opp *dev_opp;
 	struct dev_pm_opp *new_opp;
 	u64 rate;
+	u32 val;
 	int ret;
 
 	/* Hold our list modification lock here */
@@ -946,14 +947,15 @@ static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	new_opp->np = np;
 	new_opp->dynamic = false;
 	new_opp->available = true;
-	of_property_read_u32(np, "clock-latency-ns",
-			     (u32 *)&new_opp->clock_latency_ns);
+	of_property_read_u32(np, "clock-latency-ns", &val);
+	new_opp->clock_latency_ns = val;
 
 	ret = opp_get_microvolt(new_opp, dev);
 	if (ret)
 		goto free_opp;
 
-	of_property_read_u32(np, "opp-microamp", (u32 *)&new_opp->u_amp);
+	of_property_read_u32(np, "opp-microamp", &val);
+	new_opp->u_amp = val;
 
 	ret = _opp_add(dev, new_opp, dev_opp);
 	if (ret)

Re: PM / OPP: Add clock-latency-ns support

[Dan Carpenter]

On Wed, Aug 12, 2015 at 12:27:09PM +0530, Viresh Kumar wrote:
> On 11-08-15, 11:54, Stephen Boyd wrote:
> > Making it portable should be simple enough by having a temporary
> > variable of type u32 though.
> 
> Right.
> 
> @Dan: Does this look fine to you?

Looks good.

regards,
dan carpenter