From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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
Quoting Colin King (2021-02-10 10:49:38)
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected] to
reply
> clk_hw_unregister_divider(divider);
> }
>
On Wed, 10 Feb 2021 18:49:38 +0000, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 [email protected] 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);
> > }
> >
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
>>
>>
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 [email protected] 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.
>
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 [email protected] 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);
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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> 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 [email protected] 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] [email protected]
Michael