[RFC PATCH] clk: flatten clk tree in debugfs

[RFC PATCH] clk: flatten clk tree in debugfs

[Peter De Schrijver]

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.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.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;
 
 	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);
 	__clk_recalc_accuracies(clk);
 	__clk_recalc_rates(clk, POST_RATE_CHANGE);
 }
-- 
1.7.7.rc0.72.g4b5ea.dirty

[RFC PATCH] clk: flatten clk tree in debugfs

[Saravana Kannan]

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

[RFC PATCH] clk: flatten clk tree in debugfs

[Peter De Schrijver]

On Sat, May 24, 2014 at 12:24:32AM +0200, Saravana Kannan wrote:
> 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.
> 

Thanks.

> >
> > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.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? 

Indeed. This code is most likely wrong... I don't have a strong opinion
if we need an orphan dir or if we can just have a file to list orphaned
clocks. I would still like to be able to see if there are orphaned clocks.
You could argue that the same race exists for the orphan dir. It is possible
for the parent to be registered while you're traversing the orphan dir and
cause a clock to move around. In practice this seems rather unlikely to happen
to me?

> Looks like you are putting everything but the root into orphandir?
> 

Yes...

Cheers,

Peter.

[RFC PATCH] clk: flatten clk tree in debugfs

[Saravana Kannan]

On 05/26/2014 04:14 AM, Peter De Schrijver wrote:
> On Sat, May 24, 2014 at 12:24:32AM +0200, Saravana Kannan wrote:
>> 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.
>>
>
> Thanks.
>
>>>
>>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.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?
>
> Indeed. This code is most likely wrong... I don't have a strong opinion
> if we need an orphan dir or if we can just have a file to list orphaned
> clocks.
I don't have a strong opinion either. A file is probably safer in case 
we add debug files that allow changes to the clock. In which case, we 
don't want orphan clocks to be modifiable.

> I would still like to be able to see if there are orphaned clocks.
Agree

> You could argue that the same race exists for the orphan dir. It is possible
> for the parent to be registered while you're traversing the orphan dir and
> cause a clock to move around. In practice this seems rather unlikely to happen
> to me?
I think you read too much into my comment. I wasn't in any way against 
knowing orphaned clocks from debugfs. I was just referring to that 
specific point in code, the clock might always need to pick the same 
dentry. I might be wrong about the code too (I just looked at the diff).


>> Looks like you are putting everything but the root into orphandir?
>>
>
> Yes...

Now I'm confused. Does some other code path unorphan and move them into 
the clock debug "root" dir later on?

Mike, others,

Any objections to this idea? If there's not much opposition, then maybe 
Peter can actually spend time fixing and testing this patch?

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[RFC PATCH] clk: flatten clk tree in debugfs

[Mike Turquette]

Quoting Saravana Kannan (2014-05-28 10:47:46)
> On 05/26/2014 04:14 AM, Peter De Schrijver wrote:
> > On Sat, May 24, 2014 at 12:24:32AM +0200, Saravana Kannan wrote:
> >> 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.
> >>
> >
> > Thanks.
> >
> >>>
> >>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.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?
> >
> > Indeed. This code is most likely wrong... I don't have a strong opinion
> > if we need an orphan dir or if we can just have a file to list orphaned
> > clocks.
> I don't have a strong opinion either. A file is probably safer in case 
> we add debug files that allow changes to the clock. In which case, we 
> don't want orphan clocks to be modifiable.
> 
> > I would still like to be able to see if there are orphaned clocks.
> Agree

Yes, easy orphan clock detection should not go away. How about an
orphan_summary sysfs file? That means that orphan clocks will still get
their own directory under the newly flattened hierarchy, we can remove
the orphans directory completely and it is still easy to find orphan
regressions with a quick `cat /sys/kernel/debug/clk/orphan_summary`.

> 
> > You could argue that the same race exists for the orphan dir. It is possible
> > for the parent to be registered while you're traversing the orphan dir and
> > cause a clock to move around. In practice this seems rather unlikely to happen
> > to me?
> I think you read too much into my comment. I wasn't in any way against 
> knowing orphaned clocks from debugfs. I was just referring to that 
> specific point in code, the clock might always need to pick the same 
> dentry. I might be wrong about the code too (I just looked at the diff).
> 
> 
> >> Looks like you are putting everything but the root into orphandir?
> >>
> >
> > Yes...
> 
> Now I'm confused. Does some other code path unorphan and move them into 
> the clock debug "root" dir later on?

Yes. See drivers/clk/clk.c, lines 1860-1870 in the clk-next branch.

> 
> Mike, others,
> 
> Any objections to this idea? If there's not much opposition, then maybe 
> Peter can actually spend time fixing and testing this patch?

Idea seems fine. I had actually considered removing the hiearchal
directory structure completely with the advent of clk_dump and
clk_summary, but if there is value in the flattened directory structure
then I'm all for it.

Regards,
Mike

> 
> -Saravana
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation

[RFC PATCH] clk: flatten clk tree in debugfs

[Saravana Kannan]

On 05/28/2014 11:52 AM, Mike Turquette wrote:
> Quoting Saravana Kannan (2014-05-28 10:47:46)
>> On 05/26/2014 04:14 AM, Peter De Schrijver wrote:
>>> On Sat, May 24, 2014 at 12:24:32AM +0200, Saravana Kannan wrote:
>>>> 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.
>>>>
>>>
>>> Thanks.
>>>
>>>>>
>>>>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.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?
>>>
>>> Indeed. This code is most likely wrong... I don't have a strong opinion
>>> if we need an orphan dir or if we can just have a file to list orphaned
>>> clocks.
>> I don't have a strong opinion either. A file is probably safer in case
>> we add debug files that allow changes to the clock. In which case, we
>> don't want orphan clocks to be modifiable.
>>
>>> I would still like to be able to see if there are orphaned clocks.
>> Agree
>
> Yes, easy orphan clock detection should not go away. How about an
> orphan_summary sysfs file? That means that orphan clocks will still get
> their own directory under the newly flattened hierarchy, we can remove
> the orphans directory completely and it is still easy to find orphan
> regressions with a quick `cat /sys/kernel/debug/clk/orphan_summary`.
>
>>
>>> You could argue that the same race exists for the orphan dir. It is possible
>>> for the parent to be registered while you're traversing the orphan dir and
>>> cause a clock to move around. In practice this seems rather unlikely to happen
>>> to me?
>> I think you read too much into my comment. I wasn't in any way against
>> knowing orphaned clocks from debugfs. I was just referring to that
>> specific point in code, the clock might always need to pick the same
>> dentry. I might be wrong about the code too (I just looked at the diff).
>>
>>
>>>> Looks like you are putting everything but the root into orphandir?
>>>>
>>>
>>> Yes...
>>
>> Now I'm confused. Does some other code path unorphan and move them into
>> the clock debug "root" dir later on?
>
> Yes. See drivers/clk/clk.c, lines 1860-1870 in the clk-next branch.
>
>>
>> Mike, others,
>>
>> Any objections to this idea? If there's not much opposition, then maybe
>> Peter can actually spend time fixing and testing this patch?
>
> Idea seems fine. I had actually considered removing the hiearchal
> directory structure completely with the advent of clk_dump and
> clk_summary, but if there is value in the flattened directory structure
> then I'm all for it.
I think the additional value of the flattened directory probably comes 
from internal tree patches that would add set rate capability to clocks, 
etc that would be helpful with testing.

Agree with all the comments so far.

Peter,

Looks like there's some support and no opposition so far. So do you want 
to send out a real patch that's been tested and with the comments above 
taken care of?

Regards,
Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[RFC PATCH] clk: flatten clk tree in debugfs

[Peter De Schrijver]

> >>
> >> Mike, others,
> >>
> >> Any objections to this idea? If there's not much opposition, then maybe
> >> Peter can actually spend time fixing and testing this patch?
> >
> > Idea seems fine. I had actually considered removing the hiearchal
> > directory structure completely with the advent of clk_dump and
> > clk_summary, but if there is value in the flattened directory structure
> > then I'm all for it.
> I think the additional value of the flattened directory probably comes 
> from internal tree patches that would add set rate capability to clocks, 
> etc that would be helpful with testing.
> 

Yes. Eg. I posted a patch to dump a clocks registers for Tegra.

> Agree with all the comments so far.
> 
> Peter,
> 
> Looks like there's some support and no opposition so far. So do you want 
> to send out a real patch that's been tested and with the comments above 
> taken care of?

Yes. I will do that.

Cheers,

Peter.