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

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

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

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 <colin.king@canonical.com>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <colin.king@canonical.com>
> 
> 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 <colin.king@canonical.com>
> ---
>  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.tretter@pengutronix.de to
reply

>         clk_hw_unregister_divider(divider);
>  }
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michael Tretter]

On Wed, 10 Feb 2021 18:49:38 +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> 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 <colin.king@canonical.com>
> ---
>  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
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <colin.king@canonical.com>
> > 
> > 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 <colin.king@canonical.com>
> > ---
> >  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.tretter@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);
> >  }
> >
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> 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 <colin.king@canonical.com>
>> ---
>>  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
>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <colin.king@canonical.com>
> > > 
> > > 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 <colin.king@canonical.com>
> > > ---
> > >  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.tretter@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.
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <colin.king@canonical.com>
>>>>
>>>> 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 <colin.king@canonical.com>
>>>> ---
>>>>  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.tretter@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);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michael Tretter]

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 <colin.king@canonical.com>
> >>>>
> >>>> 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 <colin.king@canonical.com>
> >>>> ---
> >>>>  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.tretter@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.tretter@pengutronix.de

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel