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

From: Stephen Boyd
Date: Tue Jan 02 2018 - 18:06:46 EST


On 01/02, Geert Uytterhoeven wrote:
> On Tue, Jan 2, 2018 at 8:23 PM, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> 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