Re: [PATCH] powerpc/machdep: warn when machine_is() used too early

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 11/02/2023 à 00:56, Nathan Lynch via B4 Submission Endpoint a écrit :
>>> From: Nathan Lynch <nathanl@linux.ibm.com>
>>> 
>>> machine_is() can't provide correct results before probe_machine() has
>>> run. Warn when it's used too early in boot.
>>> 
>>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>>> ---
>>> Prompted by my attempts to do some pseries-specific setup during
>>> rtas_initialize() and being puzzled for a while that it wasn't
>>> working.
>>> ---
>>>   arch/powerpc/include/asm/machdep.h | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
>>> index 378b8d5836a7..8c0a799d18cd 100644
>>> --- a/arch/powerpc/include/asm/machdep.h
>>> +++ b/arch/powerpc/include/asm/machdep.h
>>> @@ -220,11 +220,13 @@ extern struct machdep_calls *machine_id;
>>>   	EXPORT_SYMBOL(mach_##name);				\
>>>   	struct machdep_calls mach_##name __machine_desc =
>>>   
>>> -#define machine_is(name) \
>>> -	({ \
>>> -		extern struct machdep_calls mach_##name \
>>> -			__attribute__((weak));		 \
>>> -		machine_id == &mach_##name; \
>>> +#define machine_is(name)                                            \
>>> +	({                                                          \
>>> +		extern struct machdep_calls mach_##name             \
>>> +			__attribute__((weak));                      \
>>> +		WARN(!machine_id,                                   \
>>> +		     "machine_is() called before probe_machine()"); \
>>
>> Is a WARN() really necessary ? WARN() is less optimised than WARN_ON(), 
>> especially on PPC64.
>>
>> This should never ever happen so a WARN_ON(!machine_id) should be 
>> enough, the developper that hits it is able to go to the given file:line 
>> and understand what happened.
>
> Yeah I agree, WARN_ON() should be sufficient here, and should generate
> slightly better code. We have > 100 uses of machine_is(), so keeping
> each small is desirable.

Sure, I'll change it.