| Message ID | 1423052841-15194-1-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }
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(+)