[PATCH] clk: remove unreachable code

[PATCH] clk: remove unreachable code

[Rajagopal Venkat]

while reparenting a clock, NULL check is done for clock in
consideration and its new parent. So re-check is not required.
If done, else part becomes unreachable.

Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
---
 drivers/clk/clk.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 251e45d..f896584 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1048,10 +1048,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 
 	hlist_del(&clk->child_node);
 
-	if (new_parent)
-		hlist_add_head(&clk->child_node, &new_parent->children);
-	else
-		hlist_add_head(&clk->child_node, &clk_orphan_list);
+	hlist_add_head(&clk->child_node, &new_parent->children);
 
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	if (!inited)
-- 
1.7.10.4

Re: [PATCH] clk: remove unreachable code

[Tushar Behera]

On 01/08/2013 06:33 PM, Rajagopal Venkat wrote:
> while reparenting a clock, NULL check is done for clock in
> consideration and its new parent. So re-check is not required.
> If done, else part becomes unreachable.
> 
> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> ---
>  drivers/clk/clk.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 251e45d..f896584 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1048,10 +1048,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>  
>  	hlist_del(&clk->child_node);
>  
> -	if (new_parent)
> -		hlist_add_head(&clk->child_node, &new_parent->children);
> -	else
> -		hlist_add_head(&clk->child_node, &clk_orphan_list);
> +	hlist_add_head(&clk->child_node, &new_parent->children);
>  
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>  	if (!inited)
> 

The same logic holds good for following piece of code too.

1060 |-------if (new_parent)
1061 |-------|-------new_parent_d = new_parent->dentry;
1062 |-------else
1063 |-------|-------new_parent_d = orphandir;


-- 
Tushar Behera

Re: [PATCH] clk: remove unreachable code

[Rajagopal Venkat]

On 9 January 2013 11:20, Tushar Behera <tushar.behera-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 01/08/2013 06:33 PM, Rajagopal Venkat wrote:
>> while reparenting a clock, NULL check is done for clock in
>> consideration and its new parent. So re-check is not required.
>> If done, else part becomes unreachable.
>>
>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/clk/clk.c |    5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 251e45d..f896584 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1048,10 +1048,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>
>>       hlist_del(&clk->child_node);
>>
>> -     if (new_parent)
>> -             hlist_add_head(&clk->child_node, &new_parent->children);
>> -     else
>> -             hlist_add_head(&clk->child_node, &clk_orphan_list);
>> +     hlist_add_head(&clk->child_node, &new_parent->children);
>>
>>  #ifdef CONFIG_COMMON_CLK_DEBUG
>>       if (!inited)
>>
>
> The same logic holds good for following piece of code too.
>
> 1060 |-------if (new_parent)
> 1061 |-------|-------new_parent_d = new_parent->dentry;
> 1062 |-------else
> 1063 |-------|-------new_parent_d = orphandir;

Yes. Thanks for pointing out.

>
>
> --
> Tushar Behera



-- 
Regards,
Rajagopal

[PATCH] clk: remove unreachable code

[Rajagopal Venkat]

while reparenting a clock, NULL check is done for clock in
consideration and its new parent. So re-check is not required.
If done, else part becomes unreachable.

Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
---
 drivers/clk/clk.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 251e45d..1c4097c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1040,7 +1040,6 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 {
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	struct dentry *d;
-	struct dentry *new_parent_d;
 #endif
 
 	if (!clk || !new_parent)
@@ -1048,22 +1047,14 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 
 	hlist_del(&clk->child_node);
 
-	if (new_parent)
-		hlist_add_head(&clk->child_node, &new_parent->children);
-	else
-		hlist_add_head(&clk->child_node, &clk_orphan_list);
+	hlist_add_head(&clk->child_node, &new_parent->children);
 
 #ifdef CONFIG_COMMON_CLK_DEBUG
 	if (!inited)
 		goto out;
 
-	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);
+			new_parent->dentry, clk->name);
 	if (d)
 		clk->dentry = d;
 	else
-- 
1.7.10.4

Re: [PATCH] clk: remove unreachable code

[Tushar Behera]

On 01/09/2013 11:59 AM, Rajagopal Venkat wrote:
> while reparenting a clock, NULL check is done for clock in
> consideration and its new parent. So re-check is not required.
> If done, else part becomes unreachable.
> 
> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
> ---

It is good to have revision history of the patches (version number and
changelog).

>  drivers/clk/clk.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 251e45d..1c4097c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1040,7 +1040,6 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>  {
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>  	struct dentry *d;
> -	struct dentry *new_parent_d;
>  #endif
>  
>  	if (!clk || !new_parent)
> @@ -1048,22 +1047,14 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>  
>  	hlist_del(&clk->child_node);
>  
> -	if (new_parent)
> -		hlist_add_head(&clk->child_node, &new_parent->children);
> -	else
> -		hlist_add_head(&clk->child_node, &clk_orphan_list);
> +	hlist_add_head(&clk->child_node, &new_parent->children);
>  
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>  	if (!inited)
>  		goto out;
>  
> -	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);
> +			new_parent->dentry, clk->name);
>  	if (d)
>  		clk->dentry = d;
>  	else
> 


-- 
Tushar Behera

Re: [PATCH] clk: remove unreachable code

[Mike Turquette]

Quoting Rajagopal Venkat (2013-01-08 22:29:48)
> while reparenting a clock, NULL check is done for clock in
> consideration and its new parent. So re-check is not required.
> If done, else part becomes unreachable.
> 
> Signed-off-by: Rajagopal Venkat <rajagopal.venkat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/clk/clk.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 251e45d..1c4097c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1040,7 +1040,6 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>  {
>  #ifdef CONFIG_COMMON_CLK_DEBUG
>         struct dentry *d;
> -       struct dentry *new_parent_d;
>  #endif
>  
>         if (!clk || !new_parent)
> @@ -1048,22 +1047,14 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>  
>         hlist_del(&clk->child_node);
>  
> -       if (new_parent)
> -               hlist_add_head(&clk->child_node, &new_parent->children);
> -       else
> -               hlist_add_head(&clk->child_node, &clk_orphan_list);
> +       hlist_add_head(&clk->child_node, &new_parent->children);

Rajagopal,

You found a bug, but not the right one :)

This change would result in never moving a clock into the orphan list if
the parent is missing.  The right thing to do is to allow the operation
to succeed and migrate this clock into the orphan list.  In keeping
with the clk.h api we need to treat struct clk new_parent as an opaque
cookie and not care whether or not it is NULL.

Untested patch below fixes the real bug:



>From a4d56e3ee51452366365749873710e16631e9de7 Mon Sep 17 00:00:00 2001
From: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 15 Jan 2013 14:39:06 -0800
Subject: [PATCH] clk: allow re-parenting to NULL clks

__clk_reparent presently bails early if the new parent of a clk is NULL.
This is wrong and prevents dynamically migrating clocks into the orphan
list.  The fix is to remove the NULL pointer check for new_parent in
__clk_parent.

Signed-off-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 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 593a2e4..f056230 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1186,7 +1186,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 	struct dentry *new_parent_d;
 #endif
 
-	if (!clk || !new_parent)
+	if (!clk)
 		return;
 
 	hlist_del(&clk->child_node);
-- 
1.7.10.4

Re: [PATCH] clk: remove unreachable code

[Rajagopal Venkat]

On 16 January 2013 04:15, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Rajagopal Venkat (2013-01-08 22:29:48)
>> while reparenting a clock, NULL check is done for clock in
>> consideration and its new parent. So re-check is not required.
>> If done, else part becomes unreachable.
>>
>> Signed-off-by: Rajagopal Venkat <rajagopal.venkat@linaro.org>
>> ---
>>  drivers/clk/clk.c |   13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 251e45d..1c4097c 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1040,7 +1040,6 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>  {
>>  #ifdef CONFIG_COMMON_CLK_DEBUG
>>         struct dentry *d;
>> -       struct dentry *new_parent_d;
>>  #endif
>>
>>         if (!clk || !new_parent)
>> @@ -1048,22 +1047,14 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>>
>>         hlist_del(&clk->child_node);
>>
>> -       if (new_parent)
>> -               hlist_add_head(&clk->child_node, &new_parent->children);
>> -       else
>> -               hlist_add_head(&clk->child_node, &clk_orphan_list);
>> +       hlist_add_head(&clk->child_node, &new_parent->children);
>
> Rajagopal,
>
> You found a bug, but not the right one :)
>
> This change would result in never moving a clock into the orphan list if
> the parent is missing.  The right thing to do is to allow the operation
> to succeed and migrate this clock into the orphan list.  In keeping
> with the clk.h api we need to treat struct clk new_parent as an opaque
> cookie and not care whether or not it is NULL.
>
Mike,

The only path where __clk_reparent() can get new parent as NULL is from
clk_set_parent() api. Tracing this path, here are the reasons why I submitted
this patch.

1. The clk_set_parent() function takes new parent for granted and tries to
access new_parent->rate when notifiers are registered,
2. For clocks with multiple parents, the __clk_set_parent() function also
takes new parent for granted and tries to access parent name while finding
the index of new parent from cached parent pointers.

But if the design decision is to allow clock to be re-parented to orphan list
when new parent is missing, I can provide the new working patch.

> Untested patch below fixes the real bug:
>
>
>
> From a4d56e3ee51452366365749873710e16631e9de7 Mon Sep 17 00:00:00 2001
> From: Mike Turquette <mturquette@linaro.org>
> Date: Tue, 15 Jan 2013 14:39:06 -0800
> Subject: [PATCH] clk: allow re-parenting to NULL clks
>
> __clk_reparent presently bails early if the new parent of a clk is NULL.
> This is wrong and prevents dynamically migrating clocks into the orphan
> list.  The fix is to remove the NULL pointer check for new_parent in
> __clk_parent.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
>  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 593a2e4..f056230 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1186,7 +1186,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
>         struct dentry *new_parent_d;
>  #endif
>
> -       if (!clk || !new_parent)
> +       if (!clk)
>                 return;
>
>         hlist_del(&clk->child_node);
> --
> 1.7.10.4
>



-- 
Regards,
Rajagopal

Re: [PATCH] clk: remove unreachable code

[Mike Turquette]

Quoting Rajagopal Venkat (2013-01-16 04:45:13)
> On 16 January 2013 04:15, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > Quoting Rajagopal Venkat (2013-01-08 22:29:48)
> >> while reparenting a clock, NULL check is done for clock in
> >> consideration and its new parent. So re-check is not required.
> >> If done, else part becomes unreachable.
> >>
> >> Signed-off-by: Rajagopal Venkat <rajagopal.venkat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  drivers/clk/clk.c |   13 ++-----------
> >>  1 file changed, 2 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> >> index 251e45d..1c4097c 100644
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -1040,7 +1040,6 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >>  {
> >>  #ifdef CONFIG_COMMON_CLK_DEBUG
> >>         struct dentry *d;
> >> -       struct dentry *new_parent_d;
> >>  #endif
> >>
> >>         if (!clk || !new_parent)
> >> @@ -1048,22 +1047,14 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >>
> >>         hlist_del(&clk->child_node);
> >>
> >> -       if (new_parent)
> >> -               hlist_add_head(&clk->child_node, &new_parent->children);
> >> -       else
> >> -               hlist_add_head(&clk->child_node, &clk_orphan_list);
> >> +       hlist_add_head(&clk->child_node, &new_parent->children);
> >
> > Rajagopal,
> >
> > You found a bug, but not the right one :)
> >
> > This change would result in never moving a clock into the orphan list if
> > the parent is missing.  The right thing to do is to allow the operation
> > to succeed and migrate this clock into the orphan list.  In keeping
> > with the clk.h api we need to treat struct clk new_parent as an opaque
> > cookie and not care whether or not it is NULL.
> >
> Mike,
> 
> The only path where __clk_reparent() can get new parent as NULL is from
> clk_set_parent() api. Tracing this path, here are the reasons why I submitted
> this patch.
> 
> 1. The clk_set_parent() function takes new parent for granted and tries to
> access new_parent->rate when notifiers are registered,
> 2. For clocks with multiple parents, the __clk_set_parent() function also
> takes new parent for granted and tries to access parent name while finding
> the index of new parent from cached parent pointers.
> 
> But if the design decision is to allow clock to be re-parented to orphan list
> when new parent is missing, I can provide the new working patch.
> 

Rajagopal,

Yes, reparenting a clock to the orphan list should be supported.  Let me
know if you are going to spin a new patch or not.

Thanks,
Mike

> > Untested patch below fixes the real bug:
> >
> >
> >
> > From a4d56e3ee51452366365749873710e16631e9de7 Mon Sep 17 00:00:00 2001
> > From: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Date: Tue, 15 Jan 2013 14:39:06 -0800
> > Subject: [PATCH] clk: allow re-parenting to NULL clks
> >
> > __clk_reparent presently bails early if the new parent of a clk is NULL.
> > This is wrong and prevents dynamically migrating clocks into the orphan
> > list.  The fix is to remove the NULL pointer check for new_parent in
> > __clk_parent.
> >
> > Signed-off-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  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 593a2e4..f056230 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -1186,7 +1186,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
> >         struct dentry *new_parent_d;
> >  #endif
> >
> > -       if (!clk || !new_parent)
> > +       if (!clk)
> >                 return;
> >
> >         hlist_del(&clk->child_node);
> > --
> > 1.7.10.4
> >
> 
> 
> 
> -- 
> Regards,
> Rajagopal