| Message ID | 20230210-warn-on-machine-is-before-probe-machine-v1-1-f0cba57125fb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/machdep: warn when machine_is() used too early | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
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,
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
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.
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)