linux-kernel - Re: [PATCH v3] clocksource: timer-sp804: Fix read_current_timer() issue when clock source is not registered

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] [day] [month] [year] [list]

Message-ID: <aBsVsZu50MMJkI0q@mai.linaro.org>
Date: Wed, 7 May 2025 10:11:29 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Stephen Eta Zhou <stephen.eta.zhou@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] clocksource: timer-sp804: Fix read_current_timer()
 issue when clock source is not registered

On Mon, Apr 14, 2025 at 10:55:59AM +0800, Stephen Eta Zhou wrote:
> Register a valid read_current_timer() function for the
> SP804 timer on ARM32.
> 
> On ARM32 platforms, when the SP804 timer is selected as the clocksource,
> the driver does not register a valid read_current_timer() function.
> As a result, features that rely on this API—such as rdseed—consistently
> return incorrect values.
> 
> To fix this, a delay_timer structure is registered during the SP804
> driver's initialization. The read_current_timer() function is implemented
> using the existing sp804_read() logic, and the timer frequency is reused
> from the already-initialized clocksource.
> 
> Signed-off-by: Stephen Eta Zhou <stephen.eta.zhou@...il.com>
> ---
> Changes in v3:
> - Updated the commit message for clarity and structure
> - Link to v2: https://lore.kernel.org/all/BYAPR12MB3205D7A2BAA2712C89E03C4FD5D42@BYAPR12MB3205.namprd12.prod.outlook.com
> 
> Changes in v2:
> - Added static keyword to struct delay_timer delay
> - Integrate sp804_read_delay_timer_read and
>   struct delay_timer delay together
> - I moved the acquisition of delay.freq to
>   sp804_clocksource_and_sched_clock_init.
>   sp804_clocksource_and_sched_clock_init has already
>   acquired and judged freq, so I can use it directly,
>   and in this way I don’t need to consider whether to
>   use clk1 or clk2, which can ensure that the clock source
>   is available and reliable.
> - Added detailed description information in Commit
> - Link to v1: https://lore.kernel.org/all/BYAPR12MB3205C9C87EB560CA0CC4984BD5FB2@BYAPR12MB3205.namprd12.prod.outlook.com
> ---
>  drivers/clocksource/timer-sp804.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
> index cd1916c0532507fb3ce7a11bfab4815906e326d5..3194c743ad1d7d3862c63a32fb1297d8db291008 100644
> --- a/drivers/clocksource/timer-sp804.c
> +++ b/drivers/clocksource/timer-sp804.c
> @@ -21,6 +21,10 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
>  
> +#ifdef CONFIG_ARM
> +#include <linux/delay.h>
> +#endif
> +
>  #include "timer-sp.h"
>  
>  /* Hisilicon 64-bit timer(a variant of ARM SP804) */
> @@ -102,6 +106,15 @@ static u64 notrace sp804_read(void)
>  	return ~readl_relaxed(sched_clkevt->value);
>  }
>  
> +#ifdef CONFIG_ARM
> +static struct delay_timer delay;
> +
> +static unsigned long sp804_read_delay_timer_read(void)
> +{
> +	return sp804_read();
> +}
> +#endif
> +

#ifdef CONFIG_ARM
static struct delay_timer delay;
static unsigned long sp804_read_delay_timer_read(void)
{
	return sp804_read();
}

static void sp804_register_delay_timer(int freq)
{
	delay.freq = freq;
	delay.read_current_timer = sp804_read_delay_timer_read;
	register_current_timer_delay(&delay);
}
#else
static inline void sp804_register_delay_timer(int freq) {}
#endif

>  static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>  							 const char *name,
>  							 struct clk *clk,
> @@ -114,6 +127,10 @@ static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>  	if (rate < 0)
>  		return -EINVAL;
>  
> +#ifdef CONFIG_ARM
> +	delay.freq = rate;
> +#endif

drop the above

        sp804_register_delay_timer(rate);

>  	clkevt = sp804_clkevt_get(base);
>  
>  	writel(0, clkevt->ctrl);
> @@ -318,6 +335,12 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
>  		if (ret)
>  			goto err;
>  	}
> +
> +#ifdef CONFIG_ARM
> +	delay.read_current_timer = sp804_read_delay_timer_read;
> +	register_current_timer_delay(&delay);
> +#endif
> +

drop the above

>  	initialized = true;
>  
>  	return 0;
> 
> ---
> base-commit: 7ee983c850b40043ac4751836fbd9a2b4d0c5937
> change-id: 20250411-sp804-fix-read_current_timer-1c7c5a448c6c
> 
> Best regards,
> -- 
> Stephen Eta Zhou <stephen.eta.zhou@...il.com>
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog