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

[Rohan McLure]

> On 15 May 2023, at 3:50 pm, Nicholas Piggin <npiggin at gmail.com> wrote:
> 
> 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 at 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.

Apologies, yep I was meant to remove this WRITE_ONCE now that
the read-side has data_race. Sorry for the confusion.

> 
> 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
>