lkml.org 
[lkml]   [2018]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] clk: Fix debugfs_create_*() usage
Hi Stephen,

On Tue, Jan 2, 2018 at 8:23 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 01/02, Geert Uytterhoeven wrote:
>> When exposing data access through debugfs, the correct
>> debugfs_create_*() functions must be used, depending on data type.
>>
>> Remove all casts from data pointers passed to debugfs_create_*()
>> functions, as such casts prevent the compiler from flagging bugs.
>>
>> clk_core.rate, .accuracy, and .flags are "unsigned long", hence casting
>> to "u32 *" exposed the wrong halves on big-endian 64-bit systems.
>>
>> Fix .rate and .accuracy, by using debugfs_create_ulong() instead.
>>
>> Fix .flags by changing the field to "unsigned int", as a change to
>> debugfs_create_x64() on 64-bit systems would change the user-visible
>> formatting in debugfs.
>> Note that __clk_get_flags() and clk_hw_get_flags() are left unchanged
>> and still return "unsigned long", to avoid having to change all their
>> users. Likewise, of_clk_detect_critical() still takes "unsigned long",
>> but the comment is updated as it is never passed a real pointer to
>> clk_core.flags.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Looks like none of the 64-bit architectures support common clock yet?
>
> arm64 does.

Sorry, I meant "64-bit big endian".

>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -58,7 +58,7 @@ struct clk_core {
>> unsigned long new_rate;
>> struct clk_core *new_parent;
>> struct clk_core *new_child;
>> - unsigned long flags;
>> + unsigned int flags;
>
> This doesn't look good.

Why not?
It's not like flags is used with bitops, which would mandate unsigned long.
And you can't start using bits 32-63 without changing flags to u64, else
the extra bits are not available on 32-bit platforms.

>> @@ -2600,43 +2600,43 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
>>
>> core->dentry = d;
>>
>> - d = debugfs_create_u32("clk_rate", S_IRUGO, core->dentry,
>> - (u32 *)&core->rate);
>> + d = debugfs_create_ulong("clk_rate", S_IRUGO, core->dentry,
>> + &core->rate);
>
> As you're changing these lines, can you also change S_IRUGO to
> the octal values. That's the preferred style now.

Yes, I can. That would be a separate patch, though.

>> d = debugfs_create_x32("clk_flags", S_IRUGO, core->dentry,
>> - (u32 *)&core->flags);
>> + &core->flags);
>
> Maybe we need a new debugfs API like debugfs_create_ulong_hex()
> or something that prints out an unsigned long as a hex value?

That's possible. I already have that locally (for another user which uses
u32 or u64 depending on platform).
My main worry was the change from 0xXXXXXXXX to 0xXXXXXXXXXXXXXXXX
on 64-bit platforms, which you don't seem to see as a blocker, as
debugfs isn't ABI?

> Probably we should change it to pretty print the values and what
> they correspond to, with words, because that's the least
> confusing thing to do with regards to endianness. So the

Endianness doesn't matter when printing u32. The hex values of
the flags are the same on big and little endian.

> clk_flags file would have something like
>
> CLK_SET_RATE_PARENT
> CLK_SET_RATE_GATE
>
> if those flags are set.

But some flags are internal to platform-specific drivers, right?

> We don't care about ABI here either. This is debugfs.

OK.

>> @@ -3927,7 +3927,7 @@ static int parent_ready(struct device_node *np)
>> * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
>> * @np: Device node pointer associated with clock provider
>> * @index: clock index
>> - * @flags: pointer to clk_core->flags
>> + * @flags: pointer to core clock flags
>
> Please split this off into another patch.

OK.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
\
 
 \ /
  Last update: 2018-01-02 22:36    [W:0.027 / U:2.296 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site