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

[Michael Tretter]

On Thu, 18 Mar 2021 12:48:06 +, 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 
> 
>  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 
>  ---
>   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 m.tret...@pengutronix.de 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] 20210318144230.3438009-1-m.tret...@pengutronix.de

Michael

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

[Colin Ian King]

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 

 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 
 ---
  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 m.tret...@pengutronix.de 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);

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

[Stephen Boyd]

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 
> > > 
> > > 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 
> > > ---
> > >  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 m.tret...@pengutronix.de 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.
>

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

[Colin Ian King]

On 11/02/2021 07:31, Michael Tretter wrote:
> On Wed, 10 Feb 2021 18:49:38 +, Colin King wrote:
>> From: Colin Ian King 
>>
>> 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 
>> ---
>>  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
>>
>>

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

[Michael Tretter]

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 
> > 
> > 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 
> > ---
> >  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 m.tret...@pengutronix.de 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);
> >  }
> >
> 

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

[Michael Tretter]

On Wed, 10 Feb 2021 18:49:38 +, Colin King wrote:
> From: Colin Ian King 
> 
> 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 
> ---
>  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
> 
> 

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

[Stephen Boyd]

Quoting Colin King (2021-02-10 10:49:38)
> From: Colin Ian King 
> 
> 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 
> ---
>  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 m.tret...@pengutronix.de to
reply

> clk_hw_unregister_divider(divider);
>  }
>

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

[Colin King]

From: Colin Ian King 

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 
---
 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);
 }
 
-- 
2.30.0