PM / OPP: fix debugfs files for 64-bit

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 <[email protected]>

Fixes 5cb5fdbf38770 ("PM / OPP: Add debugfs support")

---
Found while building ARM allmodconfig

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

Quoted 2 lines · Viresh Kumar

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?

Quoted 2 lines · Greg Kroah-Hartman

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?

Quoted 12 lines · Viresh Kumar

Quoted 2 lines · Greg Kroah-Hartman

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;

Quoted 3 lines

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.

Cc'ing Mike and Stephen..

Quoted 13 lines · Greg Kroah-Hartman

Quoted 12 lines · Viresh Kumar

Quoted 2 lines · Greg Kroah-Hartman

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;

Quoted 7 lines

Quoted 3 lines

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

Quoted 1 line · Greg Kroah-Hartman

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?

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

Cc'ing Mike and Stephen..

Quoted 13 lines · Greg Kroah-Hartman

Quoted 12 lines · Viresh Kumar

Quoted 2 lines · Greg Kroah-Hartman

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;

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 3 lines

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

Signature · 2 lines

-- 
viresh

Quoting Viresh Kumar (2015-10-08 00:48:28)

Quoted 14 lines · Arnd Bergmann

Quoted 7 lines

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

Quoted 15 lines

Quoting Viresh Kumar (2015-10-08 00:48:28)

Quoted 14 lines · Arnd Bergmann

Quoted 7 lines

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.

Signature · 3 lines

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Quoted 2 lines · Greg Kroah-Hartman

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