| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <537FCAA0.6080106@codeaurora.org>
Date: Fri, 23 May 2014 15:24:32 -0700
From: Saravana Kannan <skannan@...eaurora.org>
To: Peter De Schrijver <pdeschrijver@...dia.com>
CC: linux-tegra@...r.kernel.org,
Mike Turquette <mturquette@...aro.org>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH] clk: flatten clk tree in debugfs
On 05/23/2014 03:59 AM, Peter De Schrijver wrote:
> This patch flattens the clk tree in CCF debugfs. Instead of representing the
> clocks and their hierarchy as a directory structure under
> /sys/kernel/debug/clk, each clock gets a single directory directly under
> /sys/kernel/debug/clk.
>
> While this may seem strange, here's way I think this is the right thing to do:
>
> 1) a directory structure cannot be 'snapshotted' atomically, therefore it's
> not in general possible to get a consistent view of the clocktree, because
> clocks can be reparented at any time. This was solved by adding clk_dump
> and clk_summary, which do guarantee an atomic snapshot of the tree.
> Therefore I think the directory structure doesn't add any value.
>
> 2) When writing userspace programs which use the files in the clock
> directories (eg. for testing purposes), it's impossible to know for sure
> where a certain clock will be, because it might have been reparented by the
> time you figured out the path from clk_dump. This makes writing such
> programs more difficult than it should be.
>
> So because the directory structure doesn't give any information we don't
> already provide by other means and it makes certain usecases more difficult
> than the should be, I think we should move to a flat directory containing
> one subdir per clock.
Completely agree and a huge ACK to this idea.
>
> Signed-off-by: Peter De Schrijver <pdeschrijver@...dia.com>
> ---
> drivers/clk/clk.c | 54 +++-------------------------------------------------
> 1 files changed, 4 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index dff0373..53c6b4f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -306,7 +306,7 @@ static int clk_debug_create_subtree(struct clk *clk, struct dentry *pdentry)
> goto out;
>
> hlist_for_each_entry(child, &clk->children, child_node)
> - clk_debug_create_subtree(child, clk->dentry);
> + clk_debug_create_subtree(child, pdentry);
>
> ret = 0;
> out:
> @@ -326,29 +326,20 @@ out:
> */
> static int clk_debug_register(struct clk *clk)
> {
> - struct clk *parent;
> struct dentry *pdentry;
> int ret = 0;
>
> if (!inited)
> goto out;
>
> - parent = clk->parent;
> -
> /*
> * Check to see if a clk is a root clk. Also check that it is
> * safe to add this clk to debugfs
> */
> - if (!parent)
> - if (clk->flags & CLK_IS_ROOT)
> - pdentry = rootdir;
> - else
> - pdentry = orphandir;
> + if (clk->flags & CLK_IS_ROOT)
> + pdentry = rootdir;
> else
> - if (parent->dentry)
> - pdentry = parent->dentry;
> - else
> - goto out;
> + pdentry = orphandir;
I'm confused by this code. Shouldn't pdentry always be the same? Do we
need a dir for orphans? Also, I'm not sure the code is actually right?
Looks like you are putting everything but the root into orphandir?
>
> ret = clk_debug_create_subtree(clk, pdentry);
>
> @@ -372,39 +363,6 @@ static void clk_debug_unregister(struct clk *clk)
> }
>
> /**
> - * clk_debug_reparent - reparent clk node in the debugfs clk tree
> - * @clk: the clk being reparented
> - * @new_parent: the new clk parent, may be NULL
> - *
> - * Rename clk entry in the debugfs clk tree if debugfs has been
> - * initialized. Otherwise it bails out early since the debugfs clk tree
> - * will be created lazily by clk_debug_init as part of a late_initcall.
> - *
> - * Caller must hold prepare_lock.
> - */
> -static void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> -{
> - struct dentry *d;
> - struct dentry *new_parent_d;
> -
> - if (!inited)
> - return;
> -
> - if (new_parent)
> - new_parent_d = new_parent->dentry;
> - else
> - new_parent_d = orphandir;
> -
> - d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
> - new_parent_d, clk->name);
> - if (d)
> - clk->dentry = d;
> - else
> - pr_debug("%s: failed to rename debugfs entry for %s\n",
> - __func__, clk->name);
> -}
> -
> -/**
> * clk_debug_init - lazily create the debugfs clk tree visualization
> *
> * clks are often initialized very early during boot before memory can
> @@ -1277,9 +1235,6 @@ static void __clk_set_parent_after(struct clk *clk, struct clk *parent,
> clk_disable(old_parent);
> __clk_unprepare(old_parent);
> }
> -
> - /* update debugfs with new clk tree topology */
> - clk_debug_reparent(clk, parent);
> }
>
> static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index)
> @@ -1685,7 +1640,6 @@ out:
> void __clk_reparent(struct clk *clk, struct clk *new_parent)
> {
> clk_reparent(clk, new_parent);
> - clk_debug_reparent(clk, new_parent);
This should also make a lot of set rate/parent calls faster even when
debug is enabled.
-Saravana
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists