Re: [PATCH v2 05/11] powerpc: Mark accesses to power_save callback in arch_cpu_idle

["Nicholas Piggin" <npiggin@gmail.com>]

On Wed May 10, 2023 at 1:31 PM AEST, Rohan McLure wrote:
> The power_save callback can be overwritten by another core at boot time.
> Specifically, null values will be replaced exactly once with the callback
> suitable for the particular platform (PowerNV / pseries lpars), making
> this value a good candidate for __ro_after_init.
>
> Even with this the case, KCSAN sees unmarked reads to the callback
> variable, and notices that unfortunate compiler reorderings could lead
> to distinct function pointers being read. In reality this is impossible,
> so don't instrument at this read.
>
> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> v2: Mark instances at init where the callback is written to, and
> data_race() read as there is no capacity for the value to change
> underneath.
> ---
>  arch/powerpc/kernel/idle.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> index b1c0418b25c8..43d96c0e3b96 100644
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -35,7 +35,7 @@ EXPORT_SYMBOL(cpuidle_disable);
>  
>  static int __init powersave_off(char *arg)
>  {
> -	ppc_md.power_save = NULL;
> +	WRITE_ONCE(ppc_md.power_save, NULL);
>  	cpuidle_disable = IDLE_POWERSAVE_OFF;
>  	return 1;
>  }

Shouldn't need the WRITE_ONCE if you don't need a READ_ONCE. Does
data_race work here too? What about the other writers? Does
KCSAN know it's single threaded in early boot so skips marking,
but perhaps this comes later? Would be good to have a little
comment if so.

Thanks,
Nick

> @@ -43,10 +43,13 @@ __setup("powersave=off", powersave_off);
>  
>  void arch_cpu_idle(void)
>  {
> +	/* power_save callback assigned only at init so no data race */
> +	void (*power_save)(void) = data_race(ppc_md.power_save);
> +
>  	ppc64_runlatch_off();
>  
> -	if (ppc_md.power_save) {
> -		ppc_md.power_save();
> +	if (power_save) {
> +		power_save();
>  		/*
>  		 * Some power_save functions return with
>  		 * interrupts enabled, some don't.
> -- 
> 2.37.2