linux-kernel - Re: [PATCH] clocksource/drivers/riscv: Increase the clock source rating

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: <mhng-e60a7fcd-f497-4bca-8de3-b57e2a2d3532@palmer-ri-x1c9>
Date:   Thu, 29 Dec 2022 08:22:30 -0800 (PST)
From:   Palmer Dabbelt <palmer@...belt.com>
To:     samuel@...lland.org
CC:     daniel.lezcano@...aro.org, tglx@...utronix.de,
        prabhakar.csengg@...il.com, samuel@...lland.org,
        aou@...s.berkeley.edu, Paul Walmsley <paul.walmsley@...ive.com>,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject:     Re: [PATCH] clocksource/drivers/riscv: Increase the clock source rating

On Tue, 27 Dec 2022 16:44:44 PST (-0800), samuel@...lland.org wrote:
> RISC-V provides an architectural clock source via the time CSR. This
> clock source exposes a 64-bit counter synchronized across all CPUs.
> Because it is accessed using a CSR, it is much more efficient to read
> than MMIO clock sources. For example, on the Allwinner D1, reading the
> sun4i timer in a loop takes 131 cycles/iteration, while reading the
> RISC-V time CSR takes only 5 cycles/iteration.
>
> Adjust the RISC-V clock source rating so it is preferred over the
> various platform-specific MMIO clock sources.
>
> Signed-off-by: Samuel Holland <samuel@...lland.org>
> ---
>
>  drivers/clocksource/timer-riscv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index a0d66fabf073..55dad7965f43 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -73,7 +73,7 @@ static u64 notrace riscv_sched_clock(void)
>
>  static struct clocksource riscv_clocksource = {
>  	.name		= "riscv_clocksource",
> -	.rating		= 300,
> +	.rating		= 400,
>  	.mask		= CLOCKSOURCE_MASK(64),
>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>  	.read		= riscv_clocksource_rdtime,

I've never really understood what we're supposed to do here, it seems 
like we're just picking arbitrary ratings for the various clock drivers 
to get the one we want.  That's really a property of the whole platform, 
though, not the drivers, so trying to encode it as part of the driver 
seems awkward -- if anything I'd expect the ISA clock drivers to be the 
worst on any platform, as otherwise what's the point of adding the 
platform-specific mechanism?

That said, I'm fine with this as long as it's improving things on 
the platforms that actually exist.  IIUC it's only the D1 that has 
multiple clock drivers currently, so if it's good there it's good for 
me.  We'll go crazy trying to reason about all possible future hardware, 
so we can just sort out how to make stuff work as it shows up.  So:

Acked-by: Palmer Dabbelt <palmer@...osinc.com>
Reviewed-by: Palmer Dabbelt <palmer@...osinc.com>

I'll let the clock folks chime in, happy to take it through the RISC-V 
tree but unless someone says something I'm going to assume it's aimed 
over there.

Thanks!