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

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

[Christophe Leroy]

Le 13/02/2023 à 20:23, 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, placing the WARN_ON() in a
> helper function so the reported file:line indicates exactly what went
> wrong.
> 
> checkpatch complains about __attribute__((weak)) in the patch, so
> change that to __weak, and align the line continuations as well.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
> Prompted by my attempts to do some pseries-specific setup during
> rtas_initialize() and being puzzled for a while that it wasn't
> working.
> 
> Changes in v2:
> - Use WARN_ON(), not WARN().
> - Introduce __machine_is() helper function so the line reported is
>    accurate.
> - Update __attribute__((weak)) to __weak for checkpatch's sake.
> - Link to v1: https://lore.kernel.org/r/20230210-warn-on-machine-is-before-probe-machine-v1-1-f0cba57125fb@linux.ibm.com
> ---
>   arch/powerpc/include/asm/machdep.h | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index 378b8d5836a7..459736d5e511 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -3,6 +3,7 @@
>   #define _ASM_POWERPC_MACHDEP_H
>   #ifdef __KERNEL__
>   
> +#include <linux/compiler.h>
>   #include <linux/seq_file.h>
>   #include <linux/init.h>
>   #include <linux/dma-mapping.h>
> @@ -220,11 +221,16 @@ 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; \
> +static inline bool __machine_is(const struct machdep_calls *md)
> +{
> +	WARN_ON(!machine_id); // complain if used before probe_machine()
> +	return machine_id == md;
> +}
> +
> +#define machine_is(name)                                        \

Misaligned back-slash ?

> +	({                                                      \
> +		extern struct machdep_calls mach_##name __weak; \
> +		__machine_is(&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,

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

[Nathan Lynch]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 13/02/2023 à 20:23, 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, placing the WARN_ON() in a
>> helper function so the reported file:line indicates exactly what went
>> wrong.
>> 
>> checkpatch complains about __attribute__((weak)) in the patch, so
>> change that to __weak, and align the line continuations as well.
>> 
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

thanks!

>> @@ -220,11 +221,16 @@ 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; \
>> +static inline bool __machine_is(const struct machdep_calls *md)
>> +{
>> +	WARN_ON(!machine_id); // complain if used before probe_machine()
>> +	return machine_id == md;
>> +}
>> +
>> +#define machine_is(name)                                        \
>
> Misaligned back-slash ?

An artifact of the patch format + tabs for indentation. It should be
aligned with the rest when looking at the file itself after the patch is
applied.

>
>> +	({                                                      \
>> +		extern struct machdep_calls mach_##name __weak; \
>> +		__machine_is(&mach_##name);                     \
>>   	})
>>   
>>   static inline void log_error(char *buf, unsigned int err_type, int fatal)

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

[Michael Ellerman]

On Mon, 13 Feb 2023 13:23:51 -0600, Nathan Lynch wrote:
> machine_is() can't provide correct results before probe_machine() has
> run. Warn when it's used too early in boot, placing the WARN_ON() in a
> helper function so the reported file:line indicates exactly what went
> wrong.
> 
> checkpatch complains about __attribute__((weak)) in the patch, so
> change that to __weak, and align the line continuations as well.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/machdep: warn when machine_is() used too early
      https://git.kernel.org/powerpc/c/388defd5e4180a48e068d7ba9b024ce0ca957968

cheers

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

[Nathan Lynch via B4 Submission Endpoint]

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, placing the WARN_ON() in a
helper function so the reported file:line indicates exactly what went
wrong.

checkpatch complains about __attribute__((weak)) in the patch, so
change that to __weak, and align the line continuations as well.

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.

Changes in v2:
- Use WARN_ON(), not WARN().
- Introduce __machine_is() helper function so the line reported is
  accurate.
- Update __attribute__((weak)) to __weak for checkpatch's sake.
- Link to v1: https://lore.kernel.org/r/20230210-warn-on-machine-is-before-probe-machine-v1-1-f0cba57125fb@linux.ibm.com
---
 arch/powerpc/include/asm/machdep.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 378b8d5836a7..459736d5e511 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -3,6 +3,7 @@
 #define _ASM_POWERPC_MACHDEP_H
 #ifdef __KERNEL__
 
+#include <linux/compiler.h>
 #include <linux/seq_file.h>
 #include <linux/init.h>
 #include <linux/dma-mapping.h>
@@ -220,11 +221,16 @@ 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; \
+static inline bool __machine_is(const struct machdep_calls *md)
+{
+	WARN_ON(!machine_id); // complain if used before probe_machine()
+	return machine_id == md;
+}
+
+#define machine_is(name)                                        \
+	({                                                      \
+		extern struct machdep_calls mach_##name __weak; \
+		__machine_is(&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>