Re: [PATCH] clk: Fix debugfs_create_*() usage

[Stephen Boyd <sboyd@codeaurora.org>]

On 01/02, Geert Uytterhoeven wrote:
> On Tue, Jan 2, 2018 at 8:23 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 01/02, Geert Uytterhoeven wrote:

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

Fair enough. We don't need to change it if we print better
information in debugfs though. That's all I'm saying.

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

Uhhh ok. I will fold them together if you don't :)

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

That's right. Debugfs isn't an ABI.

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

Nope. Platform specific drivers shouldn't be passing internal
flags in this field. It's for the clk core to use. Perhaps we
should enforce that by failing non-core flags on registration.
I've been catching this in review so far.

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