[next] soc: xilinx: vcu: remove deadcode on null divider check

Commit Message

Colin King Feb. 10, 2021, 6:49 p.m. UTC

From: Colin Ian King <[e-mail]>

The pointer 'divider' has previously been null checked followed by
a return, hence the subsequent null check is redundant deadcode
that can be removed.  Clean up the code and remove it.

Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
Signed-off-by: Colin Ian King <[e-mail]>
---
 drivers/clk/xilinx/xlnx_vcu.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Stephen Boyd Feb. 11, 2021, 3:28 a.m. UTC | #1

Quoting Colin King (2021-02-10 10:49:38)
> From: Colin Ian King <[e-mail]>
> 
> The pointer 'divider' has previously been null checked followed by
> a return, hence the subsequent null check is redundant deadcode
> that can be removed.  Clean up the code and remove it.
> 
> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
> Signed-off-by: Colin Ian King <[e-mail]>
> ---
>  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> index d66b1315114e..607936d7a413 100644
> --- a/drivers/clk/xilinx/xlnx_vcu.c
> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>  
>         mux = clk_hw_get_parent(divider);
>         clk_hw_unregister_mux(mux);
> -       if (!divider)
> -               return;
> -

This code is pretty confusing. Waiting for [e-mail] to
reply

>         clk_hw_unregister_divider(divider);
>  }
>

Michael Tretter Feb. 11, 2021, 7:31 a.m. UTC | #2

On Wed, 10 Feb 2021 18:49:38 +0000, Colin King wrote:
> From: Colin Ian King <[e-mail]>
> 
> The pointer 'divider' has previously been null checked followed by
> a return, hence the subsequent null check is redundant deadcode
> that can be removed.  Clean up the code and remove it.
> 
> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
> Signed-off-by: Colin Ian King <[e-mail]>
> ---
>  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> index d66b1315114e..607936d7a413 100644
> --- a/drivers/clk/xilinx/xlnx_vcu.c
> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>  
>  	mux = clk_hw_get_parent(divider);
>  	clk_hw_unregister_mux(mux);
> -	if (!divider)
> -		return;
> -
>  	clk_hw_unregister_divider(divider);

Thanks for pointing this out. There is actually a different bug there.

There should have been a check for !mux before unregistering the mux:

	mux = clk_hw_get_parent(divider);
	clk_hw_unregister_divider(divider);
	if (!mux)
		return;

	clk_hw_unregister_mux(mux);

Michael

>  }
>  
> -- 
> 2.30.0
> 
>

Michael Tretter Feb. 11, 2021, 7:39 a.m. UTC | #3

On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote:
> Quoting Colin King (2021-02-10 10:49:38)
> > From: Colin Ian King <[e-mail]>
> > 
> > The pointer 'divider' has previously been null checked followed by
> > a return, hence the subsequent null check is redundant deadcode
> > that can be removed.  Clean up the code and remove it.
> > 
> > Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
> > Signed-off-by: Colin Ian King <[e-mail]>
> > ---
> >  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> > index d66b1315114e..607936d7a413 100644
> > --- a/drivers/clk/xilinx/xlnx_vcu.c
> > +++ b/drivers/clk/xilinx/xlnx_vcu.c
> > @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> >  
> >         mux = clk_hw_get_parent(divider);
> >         clk_hw_unregister_mux(mux);
> > -       if (!divider)
> > -               return;
> > -
> 
> This code is pretty confusing. Waiting for [e-mail] to
> reply

Can you elaborate what you find confusing about this code. I would gladly try
to clarify and improve the code.

What happens here is that the driver registers a mux -> divider -> gate chain
for each output clock, but only stores the gate clock. When unregistering the
clocks, the driver starts at the gate and walks up to the mux while
unregistering the clocks.

Michael

> 
> >         clk_hw_unregister_divider(divider);
> >  }
> >
>

Colin King Feb. 11, 2021, 9:41 a.m. UTC | #4

On 11/02/2021 07:31, Michael Tretter wrote:
> On Wed, 10 Feb 2021 18:49:38 +0000, Colin King wrote:
>> From: Colin Ian King <[e-mail]>
>>
>> The pointer 'divider' has previously been null checked followed by
>> a return, hence the subsequent null check is redundant deadcode
>> that can be removed.  Clean up the code and remove it.
>>
>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
>> Signed-off-by: Colin Ian King <[e-mail]>
>> ---
>>  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
>> index d66b1315114e..607936d7a413 100644
>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>>  
>>  	mux = clk_hw_get_parent(divider);
>>  	clk_hw_unregister_mux(mux);
>> -	if (!divider)
>> -		return;
>> -
>>  	clk_hw_unregister_divider(divider);
> 
> Thanks for pointing this out. There is actually a different bug there.
> 
> There should have been a check for !mux before unregistering the mux:
> 
> 	mux = clk_hw_get_parent(divider);
> 	clk_hw_unregister_divider(divider);
> 	if (!mux)
> 		return;

Ah, that makes sense, I'll send a V2.

> 
> 	clk_hw_unregister_mux(mux);
> 
> Michael
> 
>>  }
>>  
>> -- 
>> 2.30.0
>>
>>

Stephen Boyd Feb. 11, 2021, 7:05 p.m. UTC | #5

Quoting Michael Tretter (2021-02-10 23:39:06)
> On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote:
> > Quoting Colin King (2021-02-10 10:49:38)
> > > From: Colin Ian King <[e-mail]>
> > > 
> > > The pointer 'divider' has previously been null checked followed by
> > > a return, hence the subsequent null check is redundant deadcode
> > > that can be removed.  Clean up the code and remove it.
> > > 
> > > Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
> > > Signed-off-by: Colin Ian King <[e-mail]>
> > > ---
> > >  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> > > index d66b1315114e..607936d7a413 100644
> > > --- a/drivers/clk/xilinx/xlnx_vcu.c
> > > +++ b/drivers/clk/xilinx/xlnx_vcu.c
> > > @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> > >  
> > >         mux = clk_hw_get_parent(divider);
> > >         clk_hw_unregister_mux(mux);
> > > -       if (!divider)
> > > -               return;
> > > -
> > 
> > This code is pretty confusing. Waiting for [e-mail] to
> > reply
> 
> Can you elaborate what you find confusing about this code. I would gladly try
> to clarify and improve the code.

The fact that pointers are being checked and then bailing out of the
function early, vs. doing something if the pointer is non-NULL.

> 
> What happens here is that the driver registers a mux -> divider -> gate chain
> for each output clock, but only stores the gate clock. When unregistering the
> clocks, the driver starts at the gate and walks up to the mux while
> unregistering the clocks.
>

Colin King March 18, 2021, 12:48 p.m. UTC | #6

On 11/02/2021 19:05, Stephen Boyd wrote:
> Quoting Michael Tretter (2021-02-10 23:39:06)
>> On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote:
>>> Quoting Colin King (2021-02-10 10:49:38)
>>>> From: Colin Ian King <[e-mail]>
>>>>
>>>> The pointer 'divider' has previously been null checked followed by
>>>> a return, hence the subsequent null check is redundant deadcode
>>>> that can be removed.  Clean up the code and remove it.
>>>>
>>>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
>>>> Signed-off-by: Colin Ian King <[e-mail]>
>>>> ---
>>>>  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
>>>>  1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
>>>> index d66b1315114e..607936d7a413 100644
>>>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>>>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>>>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>>>>  
>>>>         mux = clk_hw_get_parent(divider);
>>>>         clk_hw_unregister_mux(mux);
>>>> -       if (!divider)
>>>> -               return;
>>>> -
>>>
>>> This code is pretty confusing. Waiting for [e-mail] to
>>> reply
>>
>> Can you elaborate what you find confusing about this code. I would gladly try
>> to clarify and improve the code.
> 
> The fact that pointers are being checked and then bailing out of the
> function early, vs. doing something if the pointer is non-NULL.
> 
>>
>> What happens here is that the driver registers a mux -> divider -> gate chain
>> for each output clock, but only stores the gate clock. When unregistering the
>> clocks, the driver starts at the gate and walks up to the mux while
>> unregistering the clocks.
>>

OK, so I think I understand this better, should the order of
unregisteration be as follows:

diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index d66b1315114e..66bac8421460 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -511,11 +511,11 @@ static void xvcu_clk_hw_unregister_leaf(struct
clk_hw *hw)
                return;

        mux = clk_hw_get_parent(divider);
-       clk_hw_unregister_mux(mux);
-       if (!divider)
+       clk_hw_unregister_mux(divider);
+       if (!mux)
                return;

-       clk_hw_unregister_divider(divider);
+       clk_hw_unregister_divider(mux);

Michael Tretter March 18, 2021, 2:44 p.m. UTC | #7

On Thu, 18 Mar 2021 12:48:06 +0000, Colin Ian King wrote:
> On 11/02/2021 19:05, Stephen Boyd wrote:
> > Quoting Michael Tretter (2021-02-10 23:39:06)
> >> On Wed, 10 Feb 2021 19:28:18 -0800, Stephen Boyd wrote:
> >>> Quoting Colin King (2021-02-10 10:49:38)
> >>>> From: Colin Ian King <[e-mail]>
> >>>>
> >>>> The pointer 'divider' has previously been null checked followed by
> >>>> a return, hence the subsequent null check is redundant deadcode
> >>>> that can be removed.  Clean up the code and remove it.
> >>>>
> >>>> Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
> >>>> Signed-off-by: Colin Ian King <[e-mail]>
> >>>> ---
> >>>>  drivers/clk/xilinx/xlnx_vcu.c | 3 ---
> >>>>  1 file changed, 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> >>>> index d66b1315114e..607936d7a413 100644
> >>>> --- a/drivers/clk/xilinx/xlnx_vcu.c
> >>>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> >>>> @@ -512,9 +512,6 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> >>>>  
> >>>>         mux = clk_hw_get_parent(divider);
> >>>>         clk_hw_unregister_mux(mux);
> >>>> -       if (!divider)
> >>>> -               return;
> >>>> -
> >>>
> >>> This code is pretty confusing. Waiting for [e-mail] to
> >>> reply
> >>
> >> Can you elaborate what you find confusing about this code. I would gladly try
> >> to clarify and improve the code.
> > 
> > The fact that pointers are being checked and then bailing out of the
> > function early, vs. doing something if the pointer is non-NULL.
> > 
> >>
> >> What happens here is that the driver registers a mux -> divider -> gate chain
> >> for each output clock, but only stores the gate clock. When unregistering the
> >> clocks, the driver starts at the gate and walks up to the mux while
> >> unregistering the clocks.
> >>
> 
> OK, so I think I understand this better, should the order of
> unregisteration be as follows:
> 
> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> index d66b1315114e..66bac8421460 100644
> --- a/drivers/clk/xilinx/xlnx_vcu.c
> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> @@ -511,11 +511,11 @@ static void xvcu_clk_hw_unregister_leaf(struct
> clk_hw *hw)
>                 return;
> 
>         mux = clk_hw_get_parent(divider);
> -       clk_hw_unregister_mux(mux);
> -       if (!divider)
> +       clk_hw_unregister_mux(divider);

The order is correct, but this must be:

	clk_hw_unregister_divider(divider);

> +       if (!mux)
>                 return;
> 
> -       clk_hw_unregister_divider(divider);
> +       clk_hw_unregister_divider(mux);
> 

	clk_hw_unregister_mux(mux);

Taking the confusion expressed by Stephen into account, I rewrote the entire
function to clarify what is happening in this function. Please take a look
that patch [0] and tell me, if it is now easier to understand.

[0] [e-mail]

Michael

Patch

diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index d66b1315114e..607936d7a413 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -512,9 +512,6 @@  static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
 
 	mux = clk_hw_get_parent(divider);
 	clk_hw_unregister_mux(mux);
-	if (!divider)
-		return;
-
 	clk_hw_unregister_divider(divider);
 }