* Re: [PATCH] powerpc/machdep: warn when machine_is() used too early
[not found] <20230210-warn-on-machine-is-before-probe-machine-v1-1-f0cba57125fb@linux.ibm.com>
@ 2023-02-11 7:46 ` Christophe Leroy
2023-02-13 10:45 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Christophe Leroy @ 2023-02-11 7:46 UTC (permalink / raw)
To: nathanl@linux.ibm.com, Michael Ellerman, Nicholas Piggin
Cc: linuxppc-dev@lists.ozlabs.org
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.
> + machine_id == &mach_##name; \
> })
>
> static inline void log_error(char *buf, unsigned int err_type, int fatal)
>
> ---
> base-commit: 0bfb97203f5f300777624a2ad6f8f84aea3e8658
> change-id: 20230210-warn-on-machine-is-before-probe-machine-37515b1f43bb
>
> Best regards,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/machdep: warn when machine_is() used too early
2023-02-11 7:46 ` [PATCH] powerpc/machdep: warn when machine_is() used too early Christophe Leroy
@ 2023-02-13 10:45 ` Michael Ellerman
2023-02-13 13:38 ` Nathan Lynch
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2023-02-13 10:45 UTC (permalink / raw)
To: Christophe Leroy, nathanl@linux.ibm.com, Nicholas Piggin
Cc: linuxppc-dev@lists.ozlabs.org
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.
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/machdep: warn when machine_is() used too early
2023-02-13 10:45 ` Michael Ellerman
@ 2023-02-13 13:38 ` Nathan Lynch
0 siblings, 0 replies; 4+ messages in thread
From: Nathan Lynch @ 2023-02-13 13:38 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Nicholas Piggin
Cc: linuxppc-dev@lists.ozlabs.org
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] powerpc/machdep: warn when machine_is() used too early
@ 2023-02-10 23:56 Nathan Lynch via B4 Submission Endpoint
0 siblings, 0 replies; 4+ messages in thread
From: Nathan Lynch via B4 Submission Endpoint @ 2023-02-10 23:56 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Christophe Leroy
Cc: Nathan Lynch, linuxppc-dev
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()"); \
+ machine_id == &mach_##name; \
})
static inline void log_error(char *buf, unsigned int err_type, int fatal)
---
base-commit: 0bfb97203f5f300777624a2ad6f8f84aea3e8658
change-id: 20230210-warn-on-machine-is-before-probe-machine-37515b1f43bb
Best regards,
--
Nathan Lynch <nathanl@linux.ibm.com>
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-13 13:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230210-warn-on-machine-is-before-probe-machine-v1-1-f0cba57125fb@linux.ibm.com>
2023-02-11 7:46 ` [PATCH] powerpc/machdep: warn when machine_is() used too early Christophe Leroy
2023-02-13 10:45 ` Michael Ellerman
2023-02-13 13:38 ` Nathan Lynch
2023-02-10 23:56 Nathan Lynch via B4 Submission Endpoint
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).