Hi Mike, Stephen,
When exposing data access through debugfs, the correct
debugfs_create_*() functions must be used, matching the data types.
This patch series remove all casts from data pointers passed to
debugfs_create_*() functions, as such casts prevent the compiler from flagging
bugs, and fixes wrong usage exposed by that.
Please refer to the individual patches for a changelog.
Thanks!
Geert Uytterhoeven (4):
clk: Improve flags doc for of_clk_detect_critical()
clk: Use octal instead of symbolic permissions
clk: Show symbolic clock flags in debugfs
clk: Fix debugfs_create_*() usage
drivers/clk/clk.c | 92 +++++++++++++++++++++++++++++++++-----------
include/linux/clk-provider.h | 2 +
2 files changed, 72 insertions(+), 22 deletions(-)
--
2.7.4
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
When exposing data access through debugfs, the correct
debugfs_create_*() functions must be used, matching the data types.
Remove all casts from data pointers passed to debugfs_create_*()
functions, as such casts prevent the compiler from flagging bugs.
clk_core.rate and .accuracy are "unsigned long", hence casting their
addresses to "u32 *" exposed the wrong halves on big-endian 64-bit
systems. Fix this by using debugfs_create_ulong() instead.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
v2:
- Fix .flags differently in a separate patch,
- Split off of_clk_detect_critical() comment update into a separate
patch.
---
drivers/clk/clk.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7cb5143c654cd78f..972dd05dbf5558b5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2650,18 +2650,16 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
core->dentry = d;
- d = debugfs_create_u32("clk_rate", 0444, core->dentry,
- (u32 *)&core->rate);
+ d = debugfs_create_ulong("clk_rate", 0444, core->dentry, &core->rate);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_accuracy", 0444, core->dentry,
- (u32 *)&core->accuracy);
+ d = debugfs_create_ulong("clk_accuracy", 0444, core->dentry,
+ &core->accuracy);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_phase", 0444, core->dentry,
- (u32 *)&core->phase);
+ d = debugfs_create_u32("clk_phase", 0444, core->dentry, &core->phase);
if (!d)
goto err_out;
@@ -2671,22 +2669,22 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
goto err_out;
d = debugfs_create_u32("clk_prepare_count", 0444, core->dentry,
- (u32 *)&core->prepare_count);
+ &core->prepare_count);
if (!d)
goto err_out;
d = debugfs_create_u32("clk_enable_count", 0444, core->dentry,
- (u32 *)&core->enable_count);
+ &core->enable_count);
if (!d)
goto err_out;
d = debugfs_create_u32("clk_protect_count", 0444, core->dentry,
- (u32 *)&core->protect_count);
+ &core->protect_count);
if (!d)
goto err_out;
d = debugfs_create_u32("clk_notifier_count", 0444, core->dentry,
- (u32 *)&core->notifier_count);
+ &core->notifier_count);
if (!d)
goto err_out;
--
2.7.4
Octal permissions are preferred, as they are easier to read than
symbolic permissions. Hence replace "S_IRUGO" by "0444".
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
v2:
- New.
---
drivers/clk/clk.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8c4769c50a71be93..240b1d427fadab66 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2600,48 +2600,48 @@ 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,
+ d = debugfs_create_u32("clk_rate", 0444, core->dentry,
(u32 *)&core->rate);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_accuracy", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_accuracy", 0444, core->dentry,
(u32 *)&core->accuracy);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_phase", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_phase", 0444, core->dentry,
(u32 *)&core->phase);
if (!d)
goto err_out;
- d = debugfs_create_x32("clk_flags", S_IRUGO, core->dentry,
+ d = debugfs_create_x32("clk_flags", 0444, core->dentry,
(u32 *)&core->flags);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_prepare_count", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_prepare_count", 0444, core->dentry,
(u32 *)&core->prepare_count);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_enable_count", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_enable_count", 0444, core->dentry,
(u32 *)&core->enable_count);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_protect_count", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_protect_count", 0444, core->dentry,
(u32 *)&core->protect_count);
if (!d)
goto err_out;
- d = debugfs_create_u32("clk_notifier_count", S_IRUGO, core->dentry,
+ d = debugfs_create_u32("clk_notifier_count", 0444, core->dentry,
(u32 *)&core->notifier_count);
if (!d)
goto err_out;
if (core->num_parents > 1) {
- d = debugfs_create_file("clk_possible_parents", S_IRUGO,
+ d = debugfs_create_file("clk_possible_parents", 0444,
core->dentry, core, &possible_parents_fops);
if (!d)
goto err_out;
@@ -2737,22 +2737,22 @@ static int __init clk_debug_init(void)
if (!rootdir)
return -ENOMEM;
- d = debugfs_create_file("clk_summary", S_IRUGO, rootdir, &all_lists,
+ d = debugfs_create_file("clk_summary", 0444, rootdir, &all_lists,
&clk_summary_fops);
if (!d)
return -ENOMEM;
- d = debugfs_create_file("clk_dump", S_IRUGO, rootdir, &all_lists,
+ d = debugfs_create_file("clk_dump", 0444, rootdir, &all_lists,
&clk_dump_fops);
if (!d)
return -ENOMEM;
- d = debugfs_create_file("clk_orphan_summary", S_IRUGO, rootdir,
+ d = debugfs_create_file("clk_orphan_summary", 0444, rootdir,
&orphan_list, &clk_summary_fops);
if (!d)
return -ENOMEM;
- d = debugfs_create_file("clk_orphan_dump", S_IRUGO, rootdir,
+ d = debugfs_create_file("clk_orphan_dump", 0444, rootdir,
&orphan_list, &clk_dump_fops);
if (!d)
return -ENOMEM;
--
2.7.4
The "flags" parameter passed to of_clk_detect_critical() cannot be a
pointer to a real clk_core.flags field, as clk_core is private to the
clock framework internals.
Change the comment to refer to top-level framework flags instead.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
v2:
- Split off from "clk: Fix debugfs_create_*() usage" into a separate
patch,
- Change comment to match <linux/clk-provider.h> naming.
---
drivers/clk/clk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5ec580914089510a..8c4769c50a71be93 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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 top-level framework flags
*
* Detects if the clock-critical property exists and, if so, sets the
* corresponding CLK_IS_CRITICAL flag.
--
2.7.4
Currently the virtual "clk_flags" file in debugfs shows the numeric
value of the top-level framework flags for the specified clock.
Hence the user must manually interpret these values.
Moreover, on big-endian 64-bit systems, the wrong half of the value is
shown, due to the cast from "unsigned long *" to "u32 *".
Fix both issues by showing the symbolic flag names instead.
Any non-standard flags are shown as a hex number.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
v2:
- New.
---
drivers/clk/clk.c | 54 ++++++++++++++++++++++++++++++++++++++++++--
include/linux/clk-provider.h | 2 ++
2 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 240b1d427fadab66..7cb5143c654cd78f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2559,6 +2559,56 @@ static const struct file_operations clk_dump_fops = {
.release = single_release,
};
+static const struct {
+ unsigned long flag;
+ const char *name;
+} clk_flags[] = {
+ { CLK_SET_RATE_GATE, "CLK_SET_RATE_GATE", },
+ { CLK_SET_PARENT_GATE, "CLK_SET_PARENT_GATE", },
+ { CLK_SET_RATE_PARENT, "CLK_SET_RATE_PARENT", },
+ { CLK_IGNORE_UNUSED, "CLK_IGNORE_UNUSED", },
+ { CLK_IS_BASIC, "CLK_IS_BASIC", },
+ { CLK_GET_RATE_NOCACHE, "CLK_GET_RATE_NOCACHE", },
+ { CLK_SET_RATE_NO_REPARENT, "CLK_SET_RATE_NO_REPARENT", },
+ { CLK_GET_ACCURACY_NOCACHE, "CLK_GET_ACCURACY_NOCACHE", },
+ { CLK_RECALC_NEW_RATES, "CLK_RECALC_NEW_RATES", },
+ { CLK_SET_RATE_UNGATE, "CLK_SET_RATE_UNGATE", },
+ { CLK_IS_CRITICAL, "CLK_IS_CRITICAL", },
+ { CLK_OPS_PARENT_ENABLE, "CLK_OPS_PARENT_ENABLE", },
+};
+
+static int clk_flags_dump(struct seq_file *s, void *data)
+{
+ struct clk_core *core = s->private;
+ unsigned long flags = core->flags;
+ unsigned int i;
+
+ for (i = 0; flags && i < ARRAY_SIZE(clk_flags); i++) {
+ if (flags & clk_flags[i].flag) {
+ seq_printf(s, "%s\n", clk_flags[i].name);
+ flags &= ~clk_flags[i].flag;
+ }
+ }
+ if (flags) {
+ /* Unknown flags */
+ seq_printf(s, "0x%lx\n", flags);
+ }
+
+ return 0;
+}
+
+static int clk_flags_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, clk_flags_dump, inode->i_private);
+}
+
+static const struct file_operations clk_flags_fops = {
+ .open = clk_flags_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
static int possible_parents_dump(struct seq_file *s, void *data)
{
struct clk_core *core = s->private;
@@ -2615,8 +2665,8 @@ static int clk_debug_create_one(struct clk_core *core, struct dentry *pdentry)
if (!d)
goto err_out;
- d = debugfs_create_x32("clk_flags", 0444, core->dentry,
- (u32 *)&core->flags);
+ d = debugfs_create_file("clk_flags", 0444, core->dentry, core,
+ &clk_flags_fops);
if (!d)
goto err_out;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 411db2423bd45818..73add58e7d666083 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -20,6 +20,8 @@
* flags used across common struct clk. these flags should only affect the
* top-level framework. custom flags for dealing with hardware specifics
* belong in struct clk_foo
+ *
+ * Please update clk_flags[] in drivers/clk/clk.c when making changes here!
*/
#define CLK_SET_RATE_GATE BIT(0) /* must be gated across rate change */
#define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re-parent */
--
2.7.4
On 01/03, Geert Uytterhoeven wrote:
> Currently the virtual "clk_flags" file in debugfs shows the numeric
> value of the top-level framework flags for the specified clock.
> Hence the user must manually interpret these values.
>
> Moreover, on big-endian 64-bit systems, the wrong half of the value is
> shown, due to the cast from "unsigned long *" to "u32 *".
>
> Fix both issues by showing the symbolic flag names instead.
> Any non-standard flags are shown as a hex number.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> v2:
> - New.
> ---
> drivers/clk/clk.c | 54 ++++++++++++++++++++++++++++++++++++++++++--
> include/linux/clk-provider.h | 2 ++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 240b1d427fadab66..7cb5143c654cd78f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2559,6 +2559,56 @@ static const struct file_operations clk_dump_fops = {
> .release = single_release,
> };
>
> +static const struct {
> + unsigned long flag;
> + const char *name;
> +} clk_flags[] = {
> + { CLK_SET_RATE_GATE, "CLK_SET_RATE_GATE", },
> + { CLK_SET_PARENT_GATE, "CLK_SET_PARENT_GATE", },
> + { CLK_SET_RATE_PARENT, "CLK_SET_RATE_PARENT", },
> + { CLK_IGNORE_UNUSED, "CLK_IGNORE_UNUSED", },
> + { CLK_IS_BASIC, "CLK_IS_BASIC", },
> + { CLK_GET_RATE_NOCACHE, "CLK_GET_RATE_NOCACHE", },
> + { CLK_SET_RATE_NO_REPARENT, "CLK_SET_RATE_NO_REPARENT", },
> + { CLK_GET_ACCURACY_NOCACHE, "CLK_GET_ACCURACY_NOCACHE", },
> + { CLK_RECALC_NEW_RATES, "CLK_RECALC_NEW_RATES", },
> + { CLK_SET_RATE_UNGATE, "CLK_SET_RATE_UNGATE", },
> + { CLK_IS_CRITICAL, "CLK_IS_CRITICAL", },
> + { CLK_OPS_PARENT_ENABLE, "CLK_OPS_PARENT_ENABLE", },
> +};
Thanks!
I wonder if it can be a little simpler with something like the
below patch squashed in? It would also be nice to detect if we
fail to add another flag, but that may mean we need to make the
flags into some sort of enum that we also set equal to BIT(x) and
then have a case statement in the for loop instead of an array
lookup. Not sure that's a big win.
---8<---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e1fcef8614d5..c8ea2dd32251 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -24,6 +24,7 @@
#include <linux/pm_runtime.h>
#include <linux/sched.h>
#include <linux/clkdev.h>
+#include <linux/stringify.h>
#include "clk.h"
@@ -2558,18 +2559,20 @@ static const struct {
unsigned long flag;
const char *name;
} clk_flags[] = {
- { CLK_SET_RATE_GATE, "CLK_SET_RATE_GATE", },
- { CLK_SET_PARENT_GATE, "CLK_SET_PARENT_GATE", },
- { CLK_SET_RATE_PARENT, "CLK_SET_RATE_PARENT", },
- { CLK_IGNORE_UNUSED, "CLK_IGNORE_UNUSED", },
- { CLK_IS_BASIC, "CLK_IS_BASIC", },
- { CLK_GET_RATE_NOCACHE, "CLK_GET_RATE_NOCACHE", },
- { CLK_SET_RATE_NO_REPARENT, "CLK_SET_RATE_NO_REPARENT", },
- { CLK_GET_ACCURACY_NOCACHE, "CLK_GET_ACCURACY_NOCACHE", },
- { CLK_RECALC_NEW_RATES, "CLK_RECALC_NEW_RATES", },
- { CLK_SET_RATE_UNGATE, "CLK_SET_RATE_UNGATE", },
- { CLK_IS_CRITICAL, "CLK_IS_CRITICAL", },
- { CLK_OPS_PARENT_ENABLE, "CLK_OPS_PARENT_ENABLE", },
+#define ENTRY(f) { f, __stringify(f) }
+ ENTRY(CLK_SET_RATE_GATE),
+ ENTRY(CLK_SET_PARENT_GATE),
+ ENTRY(CLK_SET_RATE_PARENT),
+ ENTRY(CLK_IGNORE_UNUSED),
+ ENTRY(CLK_IS_BASIC),
+ ENTRY(CLK_GET_RATE_NOCACHE),
+ ENTRY(CLK_SET_RATE_NO_REPARENT),
+ ENTRY(CLK_GET_ACCURACY_NOCACHE),
+ ENTRY(CLK_RECALC_NEW_RATES),
+ ENTRY(CLK_SET_RATE_UNGATE),
+ ENTRY(CLK_IS_CRITICAL),
+ ENTRY(CLK_OPS_PARENT_ENABLE),
+#undef ENTRY
};
static int clk_flags_dump(struct seq_file *s, void *data)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Stephen,
On Wed, Jan 10, 2018 at 3:02 AM, Stephen Boyd <[email protected]> wrote:
> On 01/03, Geert Uytterhoeven wrote:
>> Currently the virtual "clk_flags" file in debugfs shows the numeric
>> value of the top-level framework flags for the specified clock.
>> Hence the user must manually interpret these values.
>>
>> Moreover, on big-endian 64-bit systems, the wrong half of the value is
>> shown, due to the cast from "unsigned long *" to "u32 *".
>>
>> Fix both issues by showing the symbolic flag names instead.
>> Any non-standard flags are shown as a hex number.
>>
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
> I wonder if it can be a little simpler with something like the
> below patch squashed in? It would also be nice to detect if we
Sure, I didn't bother adding a macro for that as the list isn't that long,
and fairly static.
But feel free to fold in that part.
> fail to add another flag, but that may mean we need to make the
> flags into some sort of enum that we also set equal to BIT(x) and
> then have a case statement in the for loop instead of an array
> lookup. Not sure that's a big win.
Yes, you need an enum for that.
The case statement and for loop can be avoided by indexing the array
by enum value.
And the check for unknown flags can be moved to clock registration time
later, if you deem that's the way forward.
Thanks!
> @@ -2558,18 +2559,20 @@ static const struct {
> unsigned long flag;
> const char *name;
> } clk_flags[] = {
> - { CLK_SET_RATE_GATE, "CLK_SET_RATE_GATE", },
> - { CLK_SET_PARENT_GATE, "CLK_SET_PARENT_GATE", },
> - { CLK_SET_RATE_PARENT, "CLK_SET_RATE_PARENT", },
> - { CLK_IGNORE_UNUSED, "CLK_IGNORE_UNUSED", },
> - { CLK_IS_BASIC, "CLK_IS_BASIC", },
> - { CLK_GET_RATE_NOCACHE, "CLK_GET_RATE_NOCACHE", },
> - { CLK_SET_RATE_NO_REPARENT, "CLK_SET_RATE_NO_REPARENT", },
> - { CLK_GET_ACCURACY_NOCACHE, "CLK_GET_ACCURACY_NOCACHE", },
> - { CLK_RECALC_NEW_RATES, "CLK_RECALC_NEW_RATES", },
> - { CLK_SET_RATE_UNGATE, "CLK_SET_RATE_UNGATE", },
> - { CLK_IS_CRITICAL, "CLK_IS_CRITICAL", },
> - { CLK_OPS_PARENT_ENABLE, "CLK_OPS_PARENT_ENABLE", },
> +#define ENTRY(f) { f, __stringify(f) }
> + ENTRY(CLK_SET_RATE_GATE),
> + ENTRY(CLK_SET_PARENT_GATE),
> + ENTRY(CLK_SET_RATE_PARENT),
> + ENTRY(CLK_IGNORE_UNUSED),
> + ENTRY(CLK_IS_BASIC),
> + ENTRY(CLK_GET_RATE_NOCACHE),
> + ENTRY(CLK_SET_RATE_NO_REPARENT),
> + ENTRY(CLK_GET_ACCURACY_NOCACHE),
> + ENTRY(CLK_RECALC_NEW_RATES),
> + ENTRY(CLK_SET_RATE_UNGATE),
> + ENTRY(CLK_IS_CRITICAL),
> + ENTRY(CLK_OPS_PARENT_ENABLE),
> +#undef ENTRY
> };
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On 01/03, Geert Uytterhoeven wrote:
> The "flags" parameter passed to of_clk_detect_critical() cannot be a
> pointer to a real clk_core.flags field, as clk_core is private to the
> clock framework internals.
>
> Change the comment to refer to top-level framework flags instead.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 01/03, Geert Uytterhoeven wrote:
> Octal permissions are preferred, as they are easier to read than
> symbolic permissions. Hence replace "S_IRUGO" by "0444".
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 01/03, Geert Uytterhoeven wrote:
> Currently the virtual "clk_flags" file in debugfs shows the numeric
> value of the top-level framework flags for the specified clock.
> Hence the user must manually interpret these values.
>
> Moreover, on big-endian 64-bit systems, the wrong half of the value is
> shown, due to the cast from "unsigned long *" to "u32 *".
>
> Fix both issues by showing the symbolic flag names instead.
> Any non-standard flags are shown as a hex number.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 01/03, Geert Uytterhoeven wrote:
> When exposing data access through debugfs, the correct
> debugfs_create_*() functions must be used, matching the data types.
>
> Remove all casts from data pointers passed to debugfs_create_*()
> functions, as such casts prevent the compiler from flagging bugs.
>
> clk_core.rate and .accuracy are "unsigned long", hence casting their
> addresses to "u32 *" exposed the wrong halves on big-endian 64-bit
> systems. Fix this by using debugfs_create_ulong() instead.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
Applied to clk-next
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project