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

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

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

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.

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

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?

> > 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

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.