[PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Geert Uytterhoeven]

Anyone may call clk_round_rate() with a zero rate value, so we have to
protect against that.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This was triggered by the bad version of "clk: Add rate constraints to
clocks", but can happen regardless, cfr.
https://lkml.org/lkml/2015/1/29/560

 drivers/clk/shmobile/clk-div6.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c
index efbaf6c81b7530b8..036a692c72195db9 100644
--- a/drivers/clk/shmobile/clk-div6.c
+++ b/drivers/clk/shmobile/clk-div6.c
@@ -90,6 +90,9 @@ static unsigned int cpg_div6_clock_calc_div(unsigned long rate,
 {
 	unsigned int div;
 
+	if (!rate)
+		rate = 1;
+
 	div = DIV_ROUND_CLOSEST(parent_rate, rate);
 	return clamp_t(unsigned int, div, 1, 64);
 }
-- 
1.9.1

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Sergei Shtylyov]

Hello.

On 02/04/2015 03:27 PM, Geert Uytterhoeven wrote:

> Anyone may call clk_round_rate() with a zero rate value, so we have to
> protect against that.

    Shouldn't this be checked and fixed up in clk_round_rate() then?

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

WBR, Sergei

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Laurent Pinchart]

Hi Sergei,

On Wednesday 04 February 2015 16:31:29 Sergei Shtylyov wrote:
> On 02/04/2015 03:27 PM, Geert Uytterhoeven wrote:
> > Anyone may call clk_round_rate() with a zero rate value, so we have to
> > protect against that.
> 
> Shouldn't this be checked and fixed up in clk_round_rate() then?

Not all implementations need to divide by the requested rate, so I don't think 
a check in the core is the best solution.

> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

On Wed, Feb 04, 2015 at 01:27:21PM +0100, Geert Uytterhoeven wrote:
> Anyone may call clk_round_rate() with a zero rate value, so we have to
> protect against that.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I agree that this should not be fixed in the core because the fixup is
really driver dependant.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Sergei Shtylyov]

Hello.

On 02/04/2015 08:32 PM, Wolfram Sang wrote:

>> Anyone may call clk_round_rate() with a zero rate value, so we have to
>> protect against that.

>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> I agree that this should not be fixed in the core because the fixup is
> really driver dependant.

    Dunno, zero frequency seems generally insane to me.

WBR, Sergei

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Mike Turquette]

Quoting Sergei Shtylyov (2015-02-04 09:45:14)
> Hello.
> 
> On 02/04/2015 08:32 PM, Wolfram Sang wrote:
> 
> >> Anyone may call clk_round_rate() with a zero rate value, so we have to
> >> protect against that.
> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> > I agree that this should not be fixed in the core because the fixup is
> > really driver dependant.
> 
>     Dunno, zero frequency seems generally insane to me.

It is useful to find the lowest frequency a clock can support. Basically
it is a search for the floor frequency.

Regards,
Mike

> 
> WBR, Sergei
> 

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Sergei Shtylyov]

Hello.

On 02/05/2015 01:01 AM, Mike Turquette wrote:

>>>> Anyone may call clk_round_rate() with a zero rate value, so we have to
>>>> protect against that.

>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

>>> I agree that this should not be fixed in the core because the fixup is
>>> really driver dependant.

>>      Dunno, zero frequency seems generally insane to me.

> It is useful to find the lowest frequency a clock can support. Basically
> it is a search for the floor frequency.

    Why not just use 1? Or are you assuming that some hardware could actually 
support 0 Hz?

> Regards,
> Mike

WBR, Sergei

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Mike Turquette]

Quoting Wolfram Sang (2015-02-04 09:32:34)
> On Wed, Feb 04, 2015 at 01:27:21PM +0100, Geert Uytterhoeven wrote:
> > Anyone may call clk_round_rate() with a zero rate value, so we have to
> > protect against that.
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I agree that this should not be fixed in the core because the fixup is
> really driver dependant.
> 

Applied to clk-next.

Thanks,
Mike

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Sergei Shtylyov]

Hello.

On 02/05/2015 01:04 AM, Sergei Shtylyov wrote:

>>>>> Anyone may call clk_round_rate() with a zero rate value, so we have to
>>>>> protect against that.

>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>>>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

>>>> I agree that this should not be fixed in the core because the fixup is
>>>> really driver dependant.

>>>      Dunno, zero frequency seems generally insane to me.

>> It is useful to find the lowest frequency a clock can support. Basically
>> it is a search for the floor frequency.

>     Why not just use 1? Or are you assuming that some hardware could actually
> support 0 Hz?

    Replying to myself: yes, this has happened to me, when I forgot to 
override the EXTAL frequency in the board .dts file (default was 0).

>> Regards,
>> Mike

WBR, Sergei

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Laurent Pinchart]

Hi Sergei,

On Thursday 05 February 2015 01:14:46 Sergei Shtylyov wrote:
> On 02/05/2015 01:04 AM, Sergei Shtylyov wrote:
> >>>>> Anyone may call clk_round_rate() with a zero rate value, so we have to
> >>>>> protect against that.
> >>>>> 
> >>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> 
> >>>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>> 
> >>>> I agree that this should not be fixed in the core because the fixup is
> >>>> really driver dependant.
> >>>> 
> >>> Dunno, zero frequency seems generally insane to me.
> >> 
> >> It is useful to find the lowest frequency a clock can support. Basically
> >> it is a search for the floor frequency.
> >> 
> > Why not just use 1? Or are you assuming that some hardware could actually
> > support 0 Hz?
> 
> Replying to myself: yes, this has happened to me, when I forgot to override
> the EXTAL frequency in the board .dts file (default was 0).

So it was a good thing that the driver crashed, it let you find a bug ;-)

Jokes aside, a zero frequency is the usual way to find the lowest frequency, 
but I agree that there aren't many integers between 0 and 1. Mike, do you have 
an opinion ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Mike Turquette]

Quoting Laurent Pinchart (2015-02-05 09:19:14)
> Hi Sergei,
> 
> On Thursday 05 February 2015 01:14:46 Sergei Shtylyov wrote:
> > On 02/05/2015 01:04 AM, Sergei Shtylyov wrote:
> > >>>>> Anyone may call clk_round_rate() with a zero rate value, so we have to
> > >>>>> protect against that.
> > >>>>> 
> > >>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >>>> 
> > >>>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > >>>> 
> > >>>> I agree that this should not be fixed in the core because the fixup is
> > >>>> really driver dependant.
> > >>>> 
> > >>> Dunno, zero frequency seems generally insane to me.
> > >> 
> > >> It is useful to find the lowest frequency a clock can support. Basically
> > >> it is a search for the floor frequency.
> > >> 
> > > Why not just use 1? Or are you assuming that some hardware could actually
> > > support 0 Hz?
> > 
> > Replying to myself: yes, this has happened to me, when I forgot to override
> > the EXTAL frequency in the board .dts file (default was 0).
> 
> So it was a good thing that the driver crashed, it let you find a bug ;-)
> 
> Jokes aside, a zero frequency is the usual way to find the lowest frequency, 
> but I agree that there aren't many integers between 0 and 1. Mike, do you have 
> an opinion ?

Yes, I think we should support passing a zero rate for two reasons:

1) it's crazy to not sanitize a value that is passed into a function and
used as a divisor. This is not really a shortcoming of the framework.

2) during the fractional divider discussion there was the idea of making
unsigned long rate into something like millihertz. E.g. rate = 1000 is
1Hz. If we start cheating by passing a rate of 1 into .round_rate, then
we've just created a bug for ourselves if we ever move to millihertz.

Regards,
Mike

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH] clk: shmobile: div6: Avoid division by zero in .round_rate()

[Sergei Shtylyov]

Hello.

On 2/5/2015 8:19 PM, Laurent Pinchart wrote:

>>>>>>> Anyone may call clk_round_rate() with a zero rate value, so we have to
>>>>>>> protect against that.

>>>>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

>>>>>> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

>>>>>> I agree that this should not be fixed in the core because the fixup is
>>>>>> really driver dependant.

>>>>> Dunno, zero frequency seems generally insane to me.

>>>> It is useful to find the lowest frequency a clock can support. Basically
>>>> it is a search for the floor frequency.

>>> Why not just use 1? Or are you assuming that some hardware could actually
>>> support 0 Hz?

>> Replying to myself: yes, this has happened to me, when I forgot to override
>> the EXTAL frequency in the board .dts file (default was 0).

> So it was a good thing that the driver crashed, it let you find a bug ;-)

    None of the clock drivers crashed, but the SDHI driver hanged instead, and 
I spent much time tracing it in order to find where it hanged. :-/

WBR, Sergei