PM / OPP: fix debugfs files for 64-bit
diff mbox

Message ID 5706174.BUsqiXljxZ@wuerfel
State Not Applicable, archived
Headers show

Commit Message

Arnd Bergmann Oct. 7, 2015, 7:35 a.m. UTC
The recently added debugfs support for OPP creates files using the
debugfs_create_bool() and debugfs_create_u32() interfaces, but
casts the data argument to u32*, which is broken on some architectures.

In case of debugfs_create_bool(), the API has changed as of 621a5f7ad9cd
("debugfs: Pass bool pointer to debugfs_create_bool()"), so we now get
a warning about the new interface in linux-next, which contains both
patches. Removing the cast makes it work in linux-next, and makes it
warn in cases where it does not work.

For debugfs_create_u32(), the current usage is broken on 64-bit
architectures when the values exceed the range of 32-bit variables
(which should not happen), or when the kernel is built for as
big-endian. This patch removes the casts and changes the types
to u32 to make them match and print the correct value on all
architectures.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 5cb5fdbf38770 ("PM / OPP: Add debugfs support")
---
Found while building ARM allmodconfig


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

Comments

Viresh Kumar Oct. 7, 2015, 10:59 a.m. UTC | #1
Hi Arnd,

On 07-10-15, 09:35, Arnd Bergmann wrote:
> The recently added debugfs support for OPP creates files using the
> debugfs_create_bool() and debugfs_create_u32() interfaces, but
> casts the data argument to u32*, which is broken on some architectures.
> 
> In case of debugfs_create_bool(), the API has changed as of 621a5f7ad9cd
> ("debugfs: Pass bool pointer to debugfs_create_bool()"), so we now get
> a warning about the new interface in linux-next, which contains both
> patches. Removing the cast makes it work in linux-next, and makes it
> warn in cases where it does not work.
> 
> For debugfs_create_u32(), the current usage is broken on 64-bit
> architectures when the values exceed the range of 32-bit variables
> (which should not happen), or when the kernel is built for as
> big-endian. This patch removes the casts and changes the types
> to u32 to make them match and print the correct value on all
> architectures.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 5cb5fdbf38770 ("PM / OPP: Add debugfs support")
> ---
> Found while building ARM allmodconfig

Thanks. I just got back from vacations and this was the first thing I
was going to look at. :)

So, there are two problems here and I feel they should be fixed
separately.

The bool thing is simple to fix, just remove the cast as you did.
Though it will keep warning if we use pm/linux-next branch directly,
but will work well with linux-next.

The second problem isn't that trivial. Just making unsigned longs as
u32 doesn't look like the right solution to me (at least). They are
unsigned long, to match the expected types with other frameworks like
clock and regulator.

What about adding something like debugfs_create_unsigned_long() for
such cases?

@Greg ??
Viresh Kumar Oct. 7, 2015, 11:03 a.m. UTC | #2
On 07-10-15, 16:29, Viresh Kumar wrote:
> What about adding something like debugfs_create_unsigned_long() for
> such cases?

And that can be as simple (or ugly, not sure if its buggy) as:

debugfs_create_unsigned_long()
{
        if (sizeof(unsigned long) == sizeof(u32))
                return debugfs_create_u32();
        if (sizeof(unsigned long) == sizeof(u64))
                return debugfs_create_u64();

        return -EINVAL;
}
Greg KH Oct. 7, 2015, 11:07 a.m. UTC | #3
On Wed, Oct 07, 2015 at 04:33:02PM +0530, Viresh Kumar wrote:
> On 07-10-15, 16:29, Viresh Kumar wrote:
> > What about adding something like debugfs_create_unsigned_long() for
> > such cases?
> 
> And that can be as simple (or ugly, not sure if its buggy) as:
> 
> debugfs_create_unsigned_long()
> {
>         if (sizeof(unsigned long) == sizeof(u32))
>                 return debugfs_create_u32();
>         if (sizeof(unsigned long) == sizeof(u64))
>                 return debugfs_create_u64();
> 
>         return -EINVAL;
> }

Why would you be wanting to create a "unsigned long" as an api anyway?
Just force it to be u64 all the time, can't you do that?

thanks,

greg k-h
--
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
Viresh Kumar Oct. 7, 2015, 11:21 a.m. UTC | #4
On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> Why would you be wanting to create a "unsigned long" as an api anyway?
> Just force it to be u64 all the time, can't you do that?

Okay, so the variable in question (lets say frequency) is an 'unsigned
long' and that's how all the APIs of clock framework expect/define
it.

And you are probably saying that we do this:

unsigned long freq;

debugfs_create_u64((u64 *)&freq);

Right? Or are you asking to update clock APIs to be converted to u64?

This will break things on a 32 bit architecture where unsigned long is
32 bits long, as doing this will overwrite the next 32 bits:

*freq = XYZ;
Greg KH Oct. 7, 2015, 12:57 p.m. UTC | #5
On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote:
> On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> > Why would you be wanting to create a "unsigned long" as an api anyway?
> > Just force it to be u64 all the time, can't you do that?
> 
> Okay, so the variable in question (lets say frequency) is an 'unsigned
> long' and that's how all the APIs of clock framework expect/define
> it.
> 
> And you are probably saying that we do this:
> 
> unsigned long freq;
> 
> debugfs_create_u64((u64 *)&freq);
> 
> Right? Or are you asking to update clock APIs to be converted to u64?

Yes, they should be u64 as I doubt you want to debug problems that you
have in the driver where it works on a 64bit system but doesn't on a
32bit one.

thanks,

greg k-h
--
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
Viresh Kumar Oct. 7, 2015, 1:03 p.m. UTC | #6
Cc'ing Mike and Stephen..

On 07-10-15, 13:57, Greg Kroah-Hartman wrote:
> On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote:
> > On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> > > Why would you be wanting to create a "unsigned long" as an api anyway?
> > > Just force it to be u64 all the time, can't you do that?
> > 
> > Okay, so the variable in question (lets say frequency) is an 'unsigned
> > long' and that's how all the APIs of clock framework expect/define
> > it.
> > 
> > And you are probably saying that we do this:
> > 
> > unsigned long freq;
> > 
> > debugfs_create_u64((u64 *)&freq);
> > 
> > Right? Or are you asking to update clock APIs to be converted to u64?
> 
> Yes, they should be u64 as I doubt you want to debug problems that you
> have in the driver where it works on a 64bit system but doesn't on a
> 32bit one.

Firstly changing the clock API (and other similar APIs) to make
frequency u64 instead of 'unsigned long', looks like a giant effort.
There are too many users of those API, etc..

Over that, it might be good performance wise to use u32 for 32 bit
systems and u64 for 64 bit one, to represent clock frequency and maybe
that's why we chose unsigned long there.
Russell King - ARM Linux Oct. 7, 2015, 4:33 p.m. UTC | #7
On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote:
> On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> > Why would you be wanting to create a "unsigned long" as an api anyway?
> > Just force it to be u64 all the time, can't you do that?
> 
> Okay, so the variable in question (lets say frequency) is an 'unsigned
> long' and that's how all the APIs of clock framework expect/define
> it.
> 
> And you are probably saying that we do this:
> 
> unsigned long freq;
> 
> debugfs_create_u64((u64 *)&freq);
> 
> Right? Or are you asking to update clock APIs to be converted to u64?

Don't you even think about doing that - that's totally broken no matter
what, and this is a good example of why casts are Bad.

debugfs_create_u64() will create a debugfs object that will want to
access a 64-bit value, but the value pointed to is only 32-bit.  The
net result is that the debugfs file ends up reading or writing the
neigbouring 32-bits, which may potentially be unaligned.

The variable pointed to for debugfs_create_u64() must be a 64-bit
value.  No casts allowed.

An alternative would be to have debugfs_create_ulong() or similar
which has accessors for standard C types.

I don't think forcing subsystems wanting to use debugfs to have to use
u{8,16,32,64} types for internal data is on - these are for things that
we want to say "we want this to be a 8, 16, 32 or 64-bit type
respectively" which is not what most subsystems need to do with
internal data.  This looks like a debugfs cockup to me. :)
Viresh Kumar Oct. 7, 2015, 4:36 p.m. UTC | #8
On 07-10-15, 17:33, Russell King - ARM Linux wrote:
> Don't you even think about doing that - that's totally broken no matter
> what, and this is a good example of why casts are Bad.
> 
> debugfs_create_u64() will create a debugfs object that will want to
> access a 64-bit value, but the value pointed to is only 32-bit.  The
> net result is that the debugfs file ends up reading or writing the
> neigbouring 32-bits, which may potentially be unaligned.

Yeah, that's what I was also saying. Its broken.

> The variable pointed to for debugfs_create_u64() must be a 64-bit
> value.  No casts allowed.
> 
> An alternative would be to have debugfs_create_ulong()

And that's what I suggested.
Greg KH Oct. 7, 2015, 5:19 p.m. UTC | #9
On Wed, Oct 07, 2015 at 06:33:24PM +0530, Viresh Kumar wrote:
> Cc'ing Mike and Stephen..
> 
> On 07-10-15, 13:57, Greg Kroah-Hartman wrote:
> > On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote:
> > > On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> > > > Why would you be wanting to create a "unsigned long" as an api anyway?
> > > > Just force it to be u64 all the time, can't you do that?
> > > 
> > > Okay, so the variable in question (lets say frequency) is an 'unsigned
> > > long' and that's how all the APIs of clock framework expect/define
> > > it.
> > > 
> > > And you are probably saying that we do this:
> > > 
> > > unsigned long freq;
> > > 
> > > debugfs_create_u64((u64 *)&freq);
> > > 
> > > Right? Or are you asking to update clock APIs to be converted to u64?
> > 
> > Yes, they should be u64 as I doubt you want to debug problems that you
> > have in the driver where it works on a 64bit system but doesn't on a
> > 32bit one.
> 
> Firstly changing the clock API (and other similar APIs) to make
> frequency u64 instead of 'unsigned long', looks like a giant effort.
> There are too many users of those API, etc..
> 
> Over that, it might be good performance wise to use u32 for 32 bit
> systems and u64 for 64 bit one, to represent clock frequency and maybe
> that's why we chose unsigned long there.

Ok, then stop exporting it in debugfs and everyone will be happy :)

greg k-h
--
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
Viresh Kumar Oct. 7, 2015, 5:27 p.m. UTC | #10
On 07-10-15, 18:19, Greg Kroah-Hartman wrote:
> Ok, then stop exporting it in debugfs and everyone will be happy :)

Hehe, not really. See I am not happy :(

So, what do you suggest? Should we create debugfs_create_ulong()? And
how to implement that? With existing u32/u64 APIs or from scratch?
Greg KH Oct. 7, 2015, 5:39 p.m. UTC | #11
On Wed, Oct 07, 2015 at 10:57:01PM +0530, Viresh Kumar wrote:
> On 07-10-15, 18:19, Greg Kroah-Hartman wrote:
> > Ok, then stop exporting it in debugfs and everyone will be happy :)
> 
> Hehe, not really. See I am not happy :(

Why not?  Why does this have to be exported in debugfs?  Just delete it,
who cares about it?  It's just "debugging".

> So, what do you suggest? Should we create debugfs_create_ulong()?

Ick, do we _really_ need and want that?

> And how to implement that? With existing u32/u64 APIs or from scratch?

It's probably easy to just do in a few lines "from scratch", if you
really need/want it, right?

thanks,

greg k-h
--
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
Viresh Kumar Oct. 7, 2015, 6 p.m. UTC | #12
On 07-10-15, 18:39, Greg Kroah-Hartman wrote:
> Why not?  Why does this have to be exported in debugfs?  Just delete it,
> who cares about it?  It's just "debugging".

It is really useful to see all the freq/voltage combinations on which
a device can work on, specially the CPU. And there are some serious
users of it as well..

Android has hacked the cpufreq layer today, to export something
similar in a hacky way, as they wanted to estimate power ratings based
on Hz/uV values. This will kill them as well..

And its really useful for debugging as well, to see if all went well.

Maybe debugfs is not really the best place for this, but yeah, this is
very useful.
Arnd Bergmann Oct. 7, 2015, 7:12 p.m. UTC | #13
On Wednesday 07 October 2015 18:33:24 Viresh Kumar wrote:
> Cc'ing Mike and Stephen..
> 
> On 07-10-15, 13:57, Greg Kroah-Hartman wrote:
> > On Wed, Oct 07, 2015 at 04:51:49PM +0530, Viresh Kumar wrote:
> > > On 07-10-15, 12:07, Greg Kroah-Hartman wrote:
> > > > Why would you be wanting to create a "unsigned long" as an api anyway?
> > > > Just force it to be u64 all the time, can't you do that?
> > > 
> > > Okay, so the variable in question (lets say frequency) is an 'unsigned
> > > long' and that's how all the APIs of clock framework expect/define
> > > it.
> > > 
> > > And you are probably saying that we do this:
> > > 
> > > unsigned long freq;
> > > 
> > > debugfs_create_u64((u64 *)&freq);
> > > 
> > > Right? Or are you asking to update clock APIs to be converted to u64?
> > 
> > Yes, they should be u64 as I doubt you want to debug problems that you
> > have in the driver where it works on a 64bit system but doesn't on a
> > 32bit one.
> 
> Firstly changing the clock API (and other similar APIs) to make
> frequency u64 instead of 'unsigned long', looks like a giant effort.
> There are too many users of those API, etc..
> 
> Over that, it might be good performance wise to use u32 for 32 bit
> systems and u64 for 64 bit one, to represent clock frequency and maybe
> that's why we chose unsigned long there.

I think it clearly makes sense to have a fixed length for each of these
members: either 32 bit is enough to represent all possible values, then
there is no need to make them 'long' on 64-bit architectures, or 32 bit
is not enough and then the code is broken on 32-bit architectures today
and should be fixed.

In my patch, I assumed that if 32-bit architectures work fine today, then
we don't need more range on 64-bit architectures either.

	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
Viresh Kumar Oct. 8, 2015, 7:48 a.m. UTC | #14
On 07-10-15, 21:12, Arnd Bergmann wrote:
> I think it clearly makes sense to have a fixed length for each of these
> members:
>
> either 32 bit is enough to represent all possible values, then
> there is no need to make them 'long' on 64-bit architectures, or 32 bit
> is not enough and then the code is broken on 32-bit architectures today
> and should be fixed.

I agree.

But I am not 100% sure why it was done this way to start with.
Probably this is the logic behind that:
- Max clock rate supported by a u32 is ~ 4.295 GHz
- People expected that, we will not reach this rate for 32 bit systems
  but for 64 bit ones.
- If above is true, then making it u64 for all will generate not very
  optimized code for 32bit systems, as we need to fetch two 32bit
  values everytime then.
- And making it u32 for 64 bit systems wouldn't be great as well, as
  we need to mask out half of the read value.

Ofcourse, Mike and Stephen can correct me here :)

> In my patch, I assumed that if 32-bit architectures work fine today, then
> we don't need more range on 64-bit architectures either.

The problem here is that we haven't fixed it properly.
- clock framework expects it to be unsigned long
- DT is sending a 64 bit value in Hz
- But we are storing and exposing it in u32

That's weird, isn't it?

So, either we update clock API and other similar APIs to u64 or u32
(which may not be the right thing to do), Or we keep it unsigned long
here as well and add debugfs_create_ulong().
Alan Stern Oct. 8, 2015, 2:25 p.m. UTC | #15
On Thu, 8 Oct 2015, Viresh Kumar wrote:

> > In my patch, I assumed that if 32-bit architectures work fine today, then
> > we don't need more range on 64-bit architectures either.
> 
> The problem here is that we haven't fixed it properly.
> - clock framework expects it to be unsigned long
> - DT is sending a 64 bit value in Hz
> - But we are storing and exposing it in u32
> 
> That's weird, isn't it?
> 
> So, either we update clock API and other similar APIs to u64 or u32
> (which may not be the right thing to do), Or we keep it unsigned long
> here as well and add debugfs_create_ulong().

I don't see why debugfs can't accomodate C's builtin types, rather than 
insisting on predetermined sizes.  After, the printf family of 
functions does that: %u vs. %lu.  In fact, there's no way to tell 
printf that a particular value is 32 bits or 64 bits.

Alan Stern

--
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
Michael Turquette Oct. 19, 2015, 3:40 p.m. UTC | #16
Quoting Viresh Kumar (2015-10-08 00:48:28)
> On 07-10-15, 21:12, Arnd Bergmann wrote:
> > I think it clearly makes sense to have a fixed length for each of these
> > members:
> >
> > either 32 bit is enough to represent all possible values, then
> > there is no need to make them 'long' on 64-bit architectures, or 32 bit
> > is not enough and then the code is broken on 32-bit architectures today
> > and should be fixed.
> 
> I agree.
> 
> But I am not 100% sure why it was done this way to start with.
> Probably this is the logic behind that:
> - Max clock rate supported by a u32 is ~ 4.295 GHz
> - People expected that, we will not reach this rate for 32 bit systems
>   but for 64 bit ones.
> - If above is true, then making it u64 for all will generate not very
>   optimized code for 32bit systems, as we need to fetch two 32bit
>   values everytime then.
> - And making it u32 for 64 bit systems wouldn't be great as well, as
>   we need to mask out half of the read value.
> 
> Ofcourse, Mike and Stephen can correct me here :)

I chose unsigned long for the common clock framework _implemenation_,
because the long-standing clk.h _api_ returns this type for clk_get_rate
and passes this type in for clk_set_rate and clk_round_rate.

> 
> > In my patch, I assumed that if 32-bit architectures work fine today, then
> > we don't need more range on 64-bit architectures either.
> 
> The problem here is that we haven't fixed it properly.
> - clock framework expects it to be unsigned long
> - DT is sending a 64 bit value in Hz
> - But we are storing and exposing it in u32

Don't forget cpufreq is using unsigned int for KHz.

> 
> That's weird, isn't it?

Yes it is.

> 
> So, either we update clock API and other similar APIs to u64 or u32
> (which may not be the right thing to do), Or we keep it unsigned long
> here as well and add debugfs_create_ulong().

This comes up every now and then. I'm still trying to figure out if
sub-Hertz quantities should be considered (e.g. representing freqs in
milliHertz).

Regards,
Mike

> 
> -- 
> viresh
--
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
Russell King - ARM Linux Oct. 19, 2015, 3:53 p.m. UTC | #17
On Mon, Oct 19, 2015 at 08:40:09AM -0700, Michael Turquette wrote:
> Quoting Viresh Kumar (2015-10-08 00:48:28)
> > On 07-10-15, 21:12, Arnd Bergmann wrote:
> > > I think it clearly makes sense to have a fixed length for each of these
> > > members:
> > >
> > > either 32 bit is enough to represent all possible values, then
> > > there is no need to make them 'long' on 64-bit architectures, or 32 bit
> > > is not enough and then the code is broken on 32-bit architectures today
> > > and should be fixed.
> > 
> > I agree.
> > 
> > But I am not 100% sure why it was done this way to start with.
> > Probably this is the logic behind that:
> > - Max clock rate supported by a u32 is ~ 4.295 GHz

The limit is actually half that when you consider clk_round_rate()
returns a long, because clk_round_rate() needs to be able to return
errors.

The use of long and unsigned long was done in the knowledge that it'd
restrict to 2GHz the upper limit, partially because 64-bit math on
32-bit systems is inefficient and expensive, and I initially wanted
people to use the thing, so it had to be lean and fast.  (If I'd
started out with something as complex as the CCF, it would've been
laughed out as being "far too complex" and "we don't need this").

It also started out as a way to convey clock information to drivers
for peripherals, which commonly don't need clocks up in the GHz range,
indeed, it was rare at that time the API was designed for peripheral
clocks to be much above 100MHz.

Patch
diff mbox

diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index 25f3efef24db..1cbc0acf831a 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/opp/core.c
@@ -696,7 +696,7 @@  static int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
 			break;
 
 		/* Duplicate OPPs */
-		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %lu, volt: %lu, enabled: %d. New: freq: %lu, volt: %lu, enabled: %d\n",
+		dev_warn(dev, "%s: duplicate OPPs detected. Existing: freq: %u, volt: %u, enabled: %d. New: freq: %u, volt: %u, enabled: %d\n",
 			 __func__, opp->rate, opp->u_volt, opp->available,
 			 new_opp->rate, new_opp->u_volt, new_opp->available);
 
@@ -898,7 +898,7 @@  static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 	/* OPP to select on device suspend */
 	if (of_property_read_bool(np, "opp-suspend")) {
 		if (dev_opp->suspend_opp)
-			dev_warn(dev, "%s: Multiple suspend OPPs found (%lu %lu)\n",
+			dev_warn(dev, "%s: Multiple suspend OPPs found (%u %u)\n",
 				 __func__, dev_opp->suspend_opp->rate,
 				 new_opp->rate);
 		else
@@ -910,7 +910,7 @@  static int _opp_add_static_v2(struct device *dev, struct device_node *np)
 
 	mutex_unlock(&dev_opp_list_lock);
 
-	pr_debug("%s: turbo:%d rate:%lu uv:%lu uvmin:%lu uvmax:%lu latency:%lu\n",
+	pr_debug("%s: turbo:%d rate:%u uv:%u uvmin:%u uvmax:%u latency:%u\n",
 		 __func__, new_opp->turbo, new_opp->rate, new_opp->u_volt,
 		 new_opp->u_volt_min, new_opp->u_volt_max,
 		 new_opp->clock_latency_ns);
diff --git a/drivers/base/power/opp/debugfs.c b/drivers/base/power/opp/debugfs.c
index 865cbfa24640..45436dabae8d 100644
--- a/drivers/base/power/opp/debugfs.c
+++ b/drivers/base/power/opp/debugfs.c
@@ -38,43 +38,42 @@  int opp_debug_create_one(struct dev_pm_opp *opp, struct device_opp *dev_opp)
 	char name[15];
 
 	/* Rate is unique to each OPP, use it to give opp-name */
-	sprintf(name, "opp:%lu", opp->rate);
+	sprintf(name, "opp:%u", opp->rate);
 
 	/* Create per-opp directory */
 	d = debugfs_create_dir(name, pdentry);
 	if (!d)
 		return -ENOMEM;
 
-	if (!debugfs_create_bool("available", S_IRUGO, d,
-				 (u32 *)&opp->available))
+	if (!debugfs_create_bool("available", S_IRUGO, d, &opp->available))
 		return -ENOMEM;
 
-	if (!debugfs_create_bool("dynamic", S_IRUGO, d, (u32 *)&opp->dynamic))
+	if (!debugfs_create_bool("dynamic", S_IRUGO, d, &opp->dynamic))
 		return -ENOMEM;
 
-	if (!debugfs_create_bool("turbo", S_IRUGO, d, (u32 *)&opp->turbo))
+	if (!debugfs_create_bool("turbo", S_IRUGO, d, &opp->turbo))
 		return -ENOMEM;
 
-	if (!debugfs_create_u32("rate_hz", S_IRUGO, d, (u32 *)&opp->rate))
+	if (!debugfs_create_u32("rate_hz", S_IRUGO, d, &opp->rate))
 		return -ENOMEM;
 
 	if (!debugfs_create_u32("u_volt_target", S_IRUGO, d,
-				(u32 *)&opp->u_volt))
+				&opp->u_volt))
 		return -ENOMEM;
 
 	if (!debugfs_create_u32("u_volt_min", S_IRUGO, d,
-				(u32 *)&opp->u_volt_min))
+				&opp->u_volt_min))
 		return -ENOMEM;
 
 	if (!debugfs_create_u32("u_volt_max", S_IRUGO, d,
-				(u32 *)&opp->u_volt_max))
+				&opp->u_volt_max))
 		return -ENOMEM;
 
-	if (!debugfs_create_u32("u_amp", S_IRUGO, d, (u32 *)&opp->u_amp))
+	if (!debugfs_create_u32("u_amp", S_IRUGO, d, &opp->u_amp))
 		return -ENOMEM;
 
 	if (!debugfs_create_u32("clock_latency_ns", S_IRUGO, d,
-				(u32 *)&opp->clock_latency_ns))
+				&opp->clock_latency_ns))
 		return -ENOMEM;
 
 	opp->dentry = d;
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 0e4c27fb0c4a..ef573e650773 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -71,13 +71,13 @@  struct dev_pm_opp {
 	bool available;
 	bool dynamic;
 	bool turbo;
-	unsigned long rate;
+	u32 rate;
 
-	unsigned long u_volt;
-	unsigned long u_volt_min;
-	unsigned long u_volt_max;
-	unsigned long u_amp;
-	unsigned long clock_latency_ns;
+	u32 u_volt;
+	u32 u_volt_min;
+	u32 u_volt_max;
+	u32 u_amp;
+	u32 clock_latency_ns;
 
 	struct device_opp *dev_opp;
 	struct rcu_head rcu_head;