linux-kernel - Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190617092148.GA508@ulmo>
Date:   Mon, 17 Jun 2019 11:21:48 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] clocksource/drivers/tegra: Cycles can't be 0

On Mon, Jun 17, 2019 at 02:47:43AM +0300, Dmitry Osipenko wrote:
> The minimum number of "cycles" is limited to 1 by
> clockevents_config_and_register().

Looks to me like cycles also depends on the multiplier and shift that
are computed for the clock source. It looks like the multiplier will
never be zero and the shift will always be such that it won't result in
a zero cycles either. But might be worth mentioning this in the commit
message. And it might be nice to also repeate that in a comment close to
the code, just to document this.

It took me a couple of minutes to track this all down, so I think we
should take the same amount of time to document it so that we don't have
to go through it again once we've forgotten why we made this change.

> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/clocksource/timer-tegra.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-tegra.c b/drivers/clocksource/timer-tegra.c
> index f6a8eb0d7322..090c85358fe8 100644
> --- a/drivers/clocksource/timer-tegra.c
> +++ b/drivers/clocksource/timer-tegra.c
> @@ -54,9 +54,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
>  {
>  	void __iomem *reg_base = timer_of_base(to_timer_of(evt));
>  
> -	writel_relaxed(TIMER_PTV_EN |
> -		       ((cycles > 1) ? (cycles - 1) : 0), /* n+1 scheme */
> -		       reg_base + TIMER_PTV);
> +	writel_relaxed(TIMER_PTV_EN | (cycles - 1), reg_base + TIMER_PTV);

Maybe keep the n+1 scheme comment and explain why we don't need to
handle the case where cycles < 1. That way it becomes crystal clear that
we don't need it, so we decrease the chances of somebody coming around
and trying to "fix" this.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)