[v1] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

[PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Alexandra Diupina 2 months, 3 weeks ago

get_div() may return zero, so it is necessary to check
before calling DIV_ROUND_UP_ULL().

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 8701a58a5804..d0e1d591e4f2 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
 	div = get_div(double_div->reg1, double_div->shift1);
 	div *= get_div(double_div->reg2, double_div->shift2);
 
-	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+	if (!div)
+		return 0;
+	else
+		return DIV_ROUND_UP_ULL((u64)parent_rate, div);
 }
 
 static const struct clk_ops clk_double_div_ops = {
-- 
2.30.2

Re: [PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Andrew Lunn 2 months, 3 weeks ago

On Mon, Sep 09, 2024 at 04:38:07PM +0300, Alexandra Diupina wrote:
> get_div() may return zero, so it is necessary to check
> before calling DIV_ROUND_UP_ULL().
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
>  drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 8701a58a5804..d0e1d591e4f2 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
>  	div = get_div(double_div->reg1, double_div->shift1);
>  	div *= get_div(double_div->reg2, double_div->shift2);
>  
> -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> +	if (!div)
> +		return 0;

Looking at this code, it seems to me some fundamental assumption has
gone wrong here, if the dividers are 0. We want to know about this,
and a kernel taking a / 0 exception would be a good way to indicate
something is very wrong. Won't returning 0 just hide the problem, not
make it obvious?

Checking for a /0 on user input makes a lot of sense, but here, i
think you are just hiding bugs. Please consider this when making
similar changes in other parts of the kernel. Why has a /0 happened?

Tools like SVACE just point at possible problems. You then need to
look at them in detail, understand the context, and decide on the
proper fix, which might actually be, a /0 is good.

	Andrew

Re: [PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Alexandra Diupina 2 months, 3 weeks ago

Hello, Andrew!


09/09/24 17:02, Andrew Lunn пишет:
> On Mon, Sep 09, 2024 at 04:38:07PM +0300, Alexandra Diupina wrote:
>> get_div() may return zero, so it is necessary to check
>> before calling DIV_ROUND_UP_ULL().
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
>> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
>> ---
>>   drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
>> index 8701a58a5804..d0e1d591e4f2 100644
>> --- a/drivers/clk/mvebu/armada-37xx-periph.c
>> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
>> @@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
>>   	div = get_div(double_div->reg1, double_div->shift1);
>>   	div *= get_div(double_div->reg2, double_div->shift2);
>>   
>> -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
>> +	if (!div)
>> +		return 0;
> Looking at this code, it seems to me some fundamental assumption has
> gone wrong here, if the dividers are 0. We want to know about this,
> and a kernel taking a / 0 exception would be a good way to indicate
> something is very wrong. Won't returning 0 just hide the problem, not
> make it obvious?
>
> Checking for a /0 on user input makes a lot of sense, but here, i
> think you are just hiding bugs. Please consider this when making
> similar changes in other parts of the kernel. Why has a /0 happened?
>
> Tools like SVACE just point at possible problems. You then need to
> look at them in detail, understand the context, and decide on the
> proper fix, which might actually be, a /0 is good.
>
> 	Andrew

The value of div depends on double_div->reg1, double_div->reg2,
double_div->shift1, double_div->shift2.
The fields reg1, reg2, shift1, shift2 in the clk_double_div structure
are filled using the PERIPH_DOUBLEDIV macro, which is called from the
PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).

It is not clear what values can be contained in the registers at the
addresses DIV_SEL0, DIV_SEL1, DIV_SEL2 (can readl() and some bit
operations give a value > 6 in get_div()), so the final value of div can 
be zero.

I used just return 0, since the recalc_rate field in the clk_ops structure
has a comment "If the driver cannot figure out a rate for this clock,
it must return 0.". I'll fix it to kernel exception, thanks for the tip.

Re: [PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Andrew Lunn 2 months, 3 weeks ago

On Mon, Sep 09, 2024 at 05:17:08PM +0300, Alexandra Diupina wrote:
> Hello, Andrew!
> 
> 
> 09/09/24 17:02, Andrew Lunn пишет:
> > On Mon, Sep 09, 2024 at 04:38:07PM +0300, Alexandra Diupina wrote:
> > > get_div() may return zero, so it is necessary to check
> > > before calling DIV_ROUND_UP_ULL().
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> > > Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> > > ---
> > >   drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> > > index 8701a58a5804..d0e1d591e4f2 100644
> > > --- a/drivers/clk/mvebu/armada-37xx-periph.c
> > > +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> > > @@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> > >   	div = get_div(double_div->reg1, double_div->shift1);
> > >   	div *= get_div(double_div->reg2, double_div->shift2);
> > > -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> > > +	if (!div)
> > > +		return 0;
> > Looking at this code, it seems to me some fundamental assumption has
> > gone wrong here, if the dividers are 0. We want to know about this,
> > and a kernel taking a / 0 exception would be a good way to indicate
> > something is very wrong. Won't returning 0 just hide the problem, not
> > make it obvious?
> > 
> > Checking for a /0 on user input makes a lot of sense, but here, i
> > think you are just hiding bugs. Please consider this when making
> > similar changes in other parts of the kernel. Why has a /0 happened?
> > 
> > Tools like SVACE just point at possible problems. You then need to
> > look at them in detail, understand the context, and decide on the
> > proper fix, which might actually be, a /0 is good.
> > 
> > 	Andrew
> 
> The value of div depends on double_div->reg1, double_div->reg2,
> double_div->shift1, double_div->shift2.
> The fields reg1, reg2, shift1, shift2 in the clk_double_div structure
> are filled using the PERIPH_DOUBLEDIV macro, which is called from the
> PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).
> 
> It is not clear what values can be contained in the registers at the
> addresses DIV_SEL0, DIV_SEL1, DIV_SEL2 (can readl() and some bit
> operations give a value > 6 in get_div()), so the final value of div can be
> zero.
> 
> I used just return 0, since the recalc_rate field in the clk_ops structure
> has a comment "If the driver cannot figure out a rate for this clock,
> it must return 0.".

This is the sort of explanation what should be placed into the commit
message. It explains the 'Why?' of the change you made, which you
cannot determine from looking at the change itself.

> I'll fix it to kernel exception, thanks for the tip.

So giving your explanation, i can see why you went for return 0. But i
guess that comment is actually about not being able to ask the
hardware what it is doing. In this case, we can ask it, but we are
getting non-sensible values from it. So i think we should be reporting
this somehow.

	Andrew

[PATCH v2] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Alexandra Diupina 2 months, 2 weeks ago

get_div() may return zero, so it is necessary to check
before calling DIV_ROUND_UP_ULL().

Return value of get_div() depends on reg1, reg2, shift1, shift2
fields of clk_double_div structure which are filled using the
PERIPH_DOUBLEDIV macro. This macro is called from the
PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).

It is not known exactly what values can be contained in the registers
at the addresses DIV_SEL0, DIV_SEL1, DIV_SEL2, so the final value of
div can be zero. Print an error message and return 0 in this case.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v2: added explanations to the commit message and printing 
of an error message when div==0
 drivers/clk/mvebu/armada-37xx-periph.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 8701a58a5804..8e749a354ffc 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -343,7 +343,13 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
 	div = get_div(double_div->reg1, double_div->shift1);
 	div *= get_div(double_div->reg2, double_div->shift2);
 
-	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+	if (!div) {
+		pr_err("Can't recalculate the rate of clock %s\n",
+										hw->init->name);
+		return 0;
+	} else {
+		return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+	}
 }
 
 static const struct clk_ops clk_double_div_ops = {
-- 
2.30.2

Re: [PATCH v2] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Andrew Lunn 2 months, 2 weeks ago

On Tue, Sep 10, 2024 at 08:31:10PM +0300, Alexandra Diupina wrote:
> get_div() may return zero, so it is necessary to check
> before calling DIV_ROUND_UP_ULL().
> 
> Return value of get_div() depends on reg1, reg2, shift1, shift2
> fields of clk_double_div structure which are filled using the
> PERIPH_DOUBLEDIV macro. This macro is called from the
> PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).
> 
> It is not known exactly what values can be contained in the registers
> at the addresses DIV_SEL0, DIV_SEL1, DIV_SEL2, so the final value of
> div can be zero. Print an error message and return 0 in this case.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
> v2: added explanations to the commit message and printing 
> of an error message when div==0
>  drivers/clk/mvebu/armada-37xx-periph.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 8701a58a5804..8e749a354ffc 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -343,7 +343,13 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
>  	div = get_div(double_div->reg1, double_div->shift1);
>  	div *= get_div(double_div->reg2, double_div->shift2);
>  
> -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> +	if (!div) {
> +		pr_err("Can't recalculate the rate of clock %s\n",
> +										hw->init->name);

Rather odd indentation!

It is too late for this merge window. Please fix this and repost on
top of 6.12-rc1.

Thanks
	Andrew

[PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Alexandra Diupina 2 months, 1 week ago

get_div() may return zero, so it is necessary to check
before calling DIV_ROUND_UP_ULL().

Return value of get_div() depends on reg1, reg2, shift1, shift2
fields of clk_double_div structure which are filled using the
PERIPH_DOUBLEDIV macro. This macro is called from the
PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).

It is not known exactly what values can be contained in the registers
at the addresses DIV_SEL0, DIV_SEL1, DIV_SEL2, so the final value of
div can be zero. Print an error message and return 0 in this case.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
---
v3: fix indentation
v2: added explanations to the commit message and printing 
of an error message when div==0
 drivers/clk/mvebu/armada-37xx-periph.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 8701a58a5804..b32c6d4d7ee5 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
 	div = get_div(double_div->reg1, double_div->shift1);
 	div *= get_div(double_div->reg2, double_div->shift2);
 
-	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+	if (!div) {
+		pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);
+		return 0;
+	} else {
+		return DIV_ROUND_UP_ULL((u64)parent_rate, div);
+	}
 }
 
 static const struct clk_ops clk_double_div_ops = {
-- 
2.30.2

Re: [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Stephen Boyd 2 months, 1 week ago

Quoting Alexandra Diupina (2024-09-17 06:22:01)
> get_div() may return zero, so it is necessary to check
> before calling DIV_ROUND_UP_ULL().
> 
> Return value of get_div() depends on reg1, reg2, shift1, shift2
> fields of clk_double_div structure which are filled using the
> PERIPH_DOUBLEDIV macro. This macro is called from the
> PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).
> 
> It is not known exactly what values can be contained in the registers
> at the addresses DIV_SEL0, DIV_SEL1, DIV_SEL2, so the final value of
> div can be zero. Print an error message and return 0 in this case.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
> ---
> v3: fix indentation
> v2: added explanations to the commit message and printing 
> of an error message when div==0

Please stop sending as replies to previous patches.

>  drivers/clk/mvebu/armada-37xx-periph.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 8701a58a5804..b32c6d4d7ee5 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
>         div = get_div(double_div->reg1, double_div->shift1);
>         div *= get_div(double_div->reg2, double_div->shift2);
>  
> -       return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> +       if (!div) {
> +               pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);

hw->init is set to NULL after registration (see clk_register() code). If
div is 0 what does the hardware do?

> +               return 0;
> +       } else {
> +               return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> +       }
>  }
>

Re: [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Alexandra Diupina 2 months ago

Hi


19/09/24 13:24, Stephen Boyd пишет:
> Quoting Alexandra Diupina (2024-09-17 06:22:01)
>> get_div() may return zero, so it is necessary to check
>> before calling DIV_ROUND_UP_ULL().
>>
>> Return value of get_div() depends on reg1, reg2, shift1, shift2
>> fields of clk_double_div structure which are filled using the
>> PERIPH_DOUBLEDIV macro. This macro is called from the
>> PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).
>>
>> It is not known exactly what values can be contained in the registers
>> at the addresses DIV_SEL0, DIV_SEL1, DIV_SEL2, so the final value of
>> div can be zero. Print an error message and return 0 in this case.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
>> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru>
>> ---
>> v3: fix indentation
>> v2: added explanations to the commit message and printing
>> of an error message when div==0
> Please stop sending as replies to previous patches.
>
>>   drivers/clk/mvebu/armada-37xx-periph.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
>> index 8701a58a5804..b32c6d4d7ee5 100644
>> --- a/drivers/clk/mvebu/armada-37xx-periph.c
>> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
>> @@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
>>          div = get_div(double_div->reg1, double_div->shift1);
>>          div *= get_div(double_div->reg2, double_div->shift2);
>>   
>> -       return DIV_ROUND_UP_ULL((u64)parent_rate, div);
>> +       if (!div) {
>> +               pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);
> hw->init is set to NULL after registration (see clk_register() code). If
> div is 0 what does the hardware do?

Thanks for noticing the error. Yes, hw->init is set to zero,
I will replace that code with clk_hw_get_name(hw).
If the value of div is 0, should I return 0 as stated in the
comment for .recalc_rate (in struct clk_ops) or should I
return parent_rate as in some other similar rate recalculation
functions (in some other drivers)?
>
>> +               return 0;
>> +       } else {
>> +               return DIV_ROUND_UP_ULL((u64)parent_rate, div);
>> +       }
>>   }
>>

Re: [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Stephen Boyd 1 month, 3 weeks ago

Quoting Alexandra Diupina (2024-09-24 06:14:44)
> >> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> >> index 8701a58a5804..b32c6d4d7ee5 100644
> >> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> >> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> >> @@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> >>          div = get_div(double_div->reg1, double_div->shift1);
> >>          div *= get_div(double_div->reg2, double_div->shift2);
> >>   
> >> -       return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> >> +       if (!div) {
> >> +               pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);
> > hw->init is set to NULL after registration (see clk_register() code). If
> > div is 0 what does the hardware do?
> 
> Thanks for noticing the error. Yes, hw->init is set to zero,
> I will replace that code with clk_hw_get_name(hw).
> If the value of div is 0, should I return 0 as stated in the
> comment for .recalc_rate (in struct clk_ops) or should I
> return parent_rate as in some other similar rate recalculation
> functions (in some other drivers)?

It depends on what the hardware does. Does the hardware pass on the
parent rate if the divider is zero? If so, then return parent_rate. Or
does it turn off completely? If so, return zero.

Re: [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Andrew Lunn 1 month, 3 weeks ago

On Mon, Oct 07, 2024 at 03:56:29PM -0700, Stephen Boyd wrote:
> Quoting Alexandra Diupina (2024-09-24 06:14:44)
> > >> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> > >> index 8701a58a5804..b32c6d4d7ee5 100644
> > >> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> > >> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> > >> @@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> > >>          div = get_div(double_div->reg1, double_div->shift1);
> > >>          div *= get_div(double_div->reg2, double_div->shift2);
> > >>   
> > >> -       return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> > >> +       if (!div) {
> > >> +               pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);
> > > hw->init is set to NULL after registration (see clk_register() code). If
> > > div is 0 what does the hardware do?
> > 
> > Thanks for noticing the error. Yes, hw->init is set to zero,
> > I will replace that code with clk_hw_get_name(hw).
> > If the value of div is 0, should I return 0 as stated in the
> > comment for .recalc_rate (in struct clk_ops) or should I
> > return parent_rate as in some other similar rate recalculation
> > functions (in some other drivers)?
> 
> It depends on what the hardware does. Does the hardware pass on the
> parent rate if the divider is zero? If so, then return parent_rate. Or
> does it turn off completely? If so, return zero.

I don't think anybody knows what the hardware does in this
condition. I also suspect it has never happened, or if it has, nobody
has complained.

I would say, let is divide by 0, so there is an obvious kernel stack
trace and hopefully a report of the issue. It can then be investigated
in a way we can then find out what the hardware actually is doing.

   Andrew

Re: [lvc-project] [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Fedor Pchelkin 1 month, 3 weeks ago

Hi Andrew,

On Tue, 08. Oct 23:58, Andrew Lunn wrote:
> On Mon, Oct 07, 2024 at 03:56:29PM -0700, Stephen Boyd wrote:
> > Quoting Alexandra Diupina (2024-09-24 06:14:44)
> > > >> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> > > >> index 8701a58a5804..b32c6d4d7ee5 100644
> > > >> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> > > >> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> > > >> @@ -343,7 +343,12 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> > > >>          div = get_div(double_div->reg1, double_div->shift1);
> > > >>          div *= get_div(double_div->reg2, double_div->shift2);
> > > >>   
> > > >> -       return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> > > >> +       if (!div) {
> > > >> +               pr_err("Can't recalculate the rate of clock %s\n", hw->init->name);
> > > > hw->init is set to NULL after registration (see clk_register() code). If
> > > > div is 0 what does the hardware do?
> > > 
> > > Thanks for noticing the error. Yes, hw->init is set to zero,
> > > I will replace that code with clk_hw_get_name(hw).
> > > If the value of div is 0, should I return 0 as stated in the
> > > comment for .recalc_rate (in struct clk_ops) or should I
> > > return parent_rate as in some other similar rate recalculation
> > > functions (in some other drivers)?
> > 
> > It depends on what the hardware does. Does the hardware pass on the
> > parent rate if the divider is zero? If so, then return parent_rate. Or
> > does it turn off completely? If so, return zero.
> 
> I don't think anybody knows what the hardware does in this
> condition. I also suspect it has never happened, or if it has, nobody
> has complained.
> 
> I would say, let is divide by 0, so there is an obvious kernel stack
> trace and hopefully a report of the issue. It can then be investigated
> in a way we can then find out what the hardware actually is doing.

Is it worth adding some kind of WARN assertions? Or actually just leave it
for now as is?

Re: [lvc-project] [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Andrew Lunn 1 month, 3 weeks ago

> > I would say, let is divide by 0, so there is an obvious kernel stack
> > trace and hopefully a report of the issue. It can then be investigated
> > in a way we can then find out what the hardware actually is doing.
> 
> Is it worth adding some kind of WARN assertions? Or actually just leave it
> for now as is?

What actually happens on a / 0 on ARM? I assume it triggers an
exception, which will give a stack trace? If so a WARN adds no value.

	Andrew

Re: [lvc-project] [PATCH v3] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

Posted by Fedor Pchelkin 1 month, 3 weeks ago

On Wed, 09. Oct 14:23, Andrew Lunn wrote:
> > > I would say, let is divide by 0, so there is an obvious kernel stack
> > > trace and hopefully a report of the issue. It can then be investigated
> > > in a way we can then find out what the hardware actually is doing.
> > 
> > Is it worth adding some kind of WARN assertions? Or actually just leave it
> > for now as is?
> 
> What actually happens on a / 0 on ARM? I assume it triggers an
> exception, which will give a stack trace? If so a WARN adds no value.

Oh, I see. I should have better said "adding WARN assertions and bailing
out with a default value if they are violated". Thus avoiding to have a
division by zero exception. Non panic_on_warn systems would at least
survive in this case but still have a valuable trace.

Somehow more importantly, it would state in the codebase that the condition
is very-very unexpected and most probably won't ever happen but not 100%
sure because it depends on hardware behavior (as I perceive reading the
current thread).

That said, if adding such WARN-bail-out pattern seems unnecessary and just
wasteful in this situation, I don't think we have any options other than
keeping the code as is.