MIME-Version: 1.0
In-Reply-To: <1458123675-20001-1-git-send-email-kuleshovmail@gmail.com>
References: <1458123675-20001-1-git-send-email-kuleshovmail@gmail.com>
Date: Thu, 7 Apr 2016 21:55:32 -0700
Message-ID: <CALAqxLUO4+H7JSOd=OqeUQsXmzDu4L-p=Pa3Df+jGz5WFpxrLw@mail.gmail.com>
Subject: Re: [PATCH] clocksource: use clocksource_freq2mult() helper
From: John Stultz <john.stultz@linaro.org>
To: Alexander Kuleshov <kuleshovmail@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1504
Lines: 41

On Wed, Mar 16, 2016 at 3:21 AM, Alexander Kuleshov
<kuleshovmail@gmail.com> wrote:
> which is introduced in the 7aca0c072 commit to simplify calculation of
> the mult and shift in the clocks_calc_mult_shift().
>
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---
>  kernel/time/clocksource.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 56ece14..de57923 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -80,9 +80,7 @@ clocks_calc_mult_shift(u32 *mult, u32 *shift, u32 from, u32 to, u32 maxsec)
>          * accuracy and fits the maxsec conversion range:
>          */
>         for (sft = 32; sft > 0; sft--) {
> -               tmp = (u64) to << sft;
> -               tmp += from / 2;
> -               do_div(tmp, from);
> +               tmp = clocksource_freq2mult(from, sft, to);
>                 if ((tmp >> sftacc) == 0)
>                         break;
>         }

I'm worried you never tested this, as its clearly broken, and keeps my
systems from booting.

clocksource_freq2mult returns a u32. In the code being removed, tmp is
a u64. So this may truncate the high bits.

Since sftacc is often 32, this causes it to exit prematurely on the
first pass through the loop.

Please do make sure to boot test what you send out. I spent some time
thinking I had broken my qemu testing setup before I realized it was
this simple looking patch.

thanks
-john