[PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
rtas_os_term() is called during panic. Its behavior depends on a couple of conditions in the /rtas node of the device tree, the traversal of which entails locking and local IRQ state changes. If the kernel panics while devtree_lock is held, rtas_os_term() as currently written could hang. Instead of discovering the relevant characteristics at panic time, cache them in file-static variables at boot. Note the lookup for "ibm,extended-os-term" is converted to of_property_read_bool() since it is a boolean property, not a RTAS function token. Signed-off-by: Nathan Lynch --- arch/powerpc/kernel/rtas.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index c12dd5ed5e00..81e4996012b7 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) /* Must be in the RMO region, so we place it here */ static char rtas_os_term_buf[2048]; +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; +static bool ibm_extended_os_term; void rtas_os_term(char *str) { @@ -958,14 +960,13 @@ void rtas_os_term(char *str) * this property may terminate the partition which we want to avoid * since it interferes with panic_timeout. */ - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term")) + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term) return; snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); do { - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL, + status = rtas_call(ibm_os_term_token, 1, 1, NULL, __pa(rtas_os_term_buf)); } while (rtas_busy_delay(status)); @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry); rtas.entry = no_entry ? rtas.base : entry; + /* +* Discover these now to avoid device tree lookups in the +* panic path. +*/ + ibm_os_term_token = rtas_token("ibm,os-term"); + ibm_extended_os_term = of_property_read_bool(rtas.dev, "ibm,extended-os-term"); + /* If RTAS was found, allocate the RMO buffer for it and look for * the stop-self token if any */ -- 2.37.1
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote: > rtas_os_term() is called during panic. Its behavior depends on a > couple of conditions in the /rtas node of the device tree, the > traversal of which entails locking and local IRQ state changes. If > the > kernel panics while devtree_lock is held, rtas_os_term() as currently > written could hang. > > Instead of discovering the relevant characteristics at panic time, > cache them in file-static variables at boot. Note the lookup for > "ibm,extended-os-term" is converted to of_property_read_bool() since > it is a boolean property, not a RTAS function token. > > Signed-off-by: Nathan Lynch This seems sensible, minor comment below. Reviewed-by: Andrew Donnellan > --- > arch/powerpc/kernel/rtas.c | 14 +++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c12dd5ed5e00..81e4996012b7 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) > > /* Must be in the RMO region, so we place it here */ > static char rtas_os_term_buf[2048]; > +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; s32 and int are obviously identical, but rtas_token is declared using int, as are the other variables where we cache various tokens. > +static bool ibm_extended_os_term; > > void rtas_os_term(char *str) > { > @@ -958,14 +960,13 @@ void rtas_os_term(char *str) > * this property may terminate the partition which we want to > avoid > * since it interferes with panic_timeout. > */ > - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || > - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os- > term")) > + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || > !ibm_extended_os_term) > return; > > snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); > > do { > - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, > NULL, > + status = rtas_call(ibm_os_term_token, 1, 1, NULL, > __pa(rtas_os_term_buf)); > } while (rtas_busy_delay(status)); > > @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) > no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", > &entry); > rtas.entry = no_entry ? rtas.base : entry; > > + /* > + * Discover these now to avoid device tree lookups in the > + * panic path. > + */ > + ibm_os_term_token = rtas_token("ibm,os-term"); > + ibm_extended_os_term = of_property_read_bool(rtas.dev, > "ibm,extended-os-term"); > + > /* If RTAS was found, allocate the RMO buffer for it and look > for > * the stop-self token if any > */ -- Andrew DonnellanOzLabs, ADL Canberra a...@linux.ibm.com IBM Australia Limited
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: > rtas_os_term() is called during panic. Its behavior depends on a > couple of conditions in the /rtas node of the device tree, the > traversal of which entails locking and local IRQ state changes. If the > kernel panics while devtree_lock is held, rtas_os_term() as currently > written could hang. Nice. > > Instead of discovering the relevant characteristics at panic time, > cache them in file-static variables at boot. Note the lookup for > "ibm,extended-os-term" is converted to of_property_read_bool() since > it is a boolean property, not a RTAS function token. Small nit, but you could do that at the query site unless you were going to start using ibm,os-term without the extended capability. Reviewed-by: Nicholas Piggin > > Signed-off-by: Nathan Lynch > --- > arch/powerpc/kernel/rtas.c | 14 +++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c12dd5ed5e00..81e4996012b7 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) > > /* Must be in the RMO region, so we place it here */ > static char rtas_os_term_buf[2048]; > +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; > +static bool ibm_extended_os_term; > > void rtas_os_term(char *str) > { > @@ -958,14 +960,13 @@ void rtas_os_term(char *str) >* this property may terminate the partition which we want to avoid >* since it interferes with panic_timeout. >*/ > - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || > - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term")) > + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term) > return; > > snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); > > do { > - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL, > + status = rtas_call(ibm_os_term_token, 1, 1, NULL, > __pa(rtas_os_term_buf)); > } while (rtas_busy_delay(status)); > > @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) > no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry); > rtas.entry = no_entry ? rtas.base : entry; > > + /* > + * Discover these now to avoid device tree lookups in the > + * panic path. > + */ > + ibm_os_term_token = rtas_token("ibm,os-term"); > + ibm_extended_os_term = of_property_read_bool(rtas.dev, > "ibm,extended-os-term"); > + > /* If RTAS was found, allocate the RMO buffer for it and look for >* the stop-self token if any >*/ > -- > 2.37.1
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
Andrew Donnellan writes: >> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c >> index c12dd5ed5e00..81e4996012b7 100644 >> --- a/arch/powerpc/kernel/rtas.c >> +++ b/arch/powerpc/kernel/rtas.c >> @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) >> >> /* Must be in the RMO region, so we place it here */ >> static char rtas_os_term_buf[2048]; >> +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; > > s32 and int are obviously identical, but rtas_token is declared using > int, as are the other variables where we cache various tokens. Right... I think it's better practice to use an explicitly sized type where the data is directly derived from the device tree and ultimately passed to the firmware call interface. Gradually enacting this while tolerating some cosmetic inconsistency in the code seems OK to me, but I'm open to other opinions.
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
"Nicholas Piggin" writes: > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: >> rtas_os_term() is called during panic. Its behavior depends on a >> couple of conditions in the /rtas node of the device tree, the >> traversal of which entails locking and local IRQ state changes. If the >> kernel panics while devtree_lock is held, rtas_os_term() as currently >> written could hang. > > Nice. > >> >> Instead of discovering the relevant characteristics at panic time, >> cache them in file-static variables at boot. Note the lookup for >> "ibm,extended-os-term" is converted to of_property_read_bool() since >> it is a boolean property, not a RTAS function token. > > Small nit, but you could do that at the query site unless you > were going to start using ibm,os-term without the extended > capability. I'm unsure that this is what you're suggesting, but we don't want to use of_property_read_bool() in this context either, because it has the same undesirable qualities as rtas_token(). > Reviewed-by: Nicholas Piggin Thanks!
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote: > "Nicholas Piggin" writes: > > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: > >> rtas_os_term() is called during panic. Its behavior depends on a > >> couple of conditions in the /rtas node of the device tree, the > >> traversal of which entails locking and local IRQ state changes. If the > >> kernel panics while devtree_lock is held, rtas_os_term() as currently > >> written could hang. > > > > Nice. > > > >> > >> Instead of discovering the relevant characteristics at panic time, > >> cache them in file-static variables at boot. Note the lookup for > >> "ibm,extended-os-term" is converted to of_property_read_bool() since > >> it is a boolean property, not a RTAS function token. > > > > Small nit, but you could do that at the query site unless you > > were going to start using ibm,os-term without the extended > > capability. > > I'm unsure that this is what you're suggesting, but we don't want to use > of_property_read_bool() in this context either, because it has the same > undesirable qualities as rtas_token(). I mean rtas_initialize() could do if (of_property_read_bool(rtas.dev, "ibm,extended-os-term")) ibm_os_term_token = rtas_token("ibm,os-term"); Thanks, Nick
Re: [PATCH 03/13] powerpc/rtas: avoid device tree lookups in rtas_os_term()
"Nicholas Piggin" writes: > On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote: >> "Nicholas Piggin" writes: >> > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: >> >> rtas_os_term() is called during panic. Its behavior depends on a >> >> couple of conditions in the /rtas node of the device tree, the >> >> traversal of which entails locking and local IRQ state changes. If the >> >> kernel panics while devtree_lock is held, rtas_os_term() as currently >> >> written could hang. >> > >> > Nice. >> > >> >> >> >> Instead of discovering the relevant characteristics at panic time, >> >> cache them in file-static variables at boot. Note the lookup for >> >> "ibm,extended-os-term" is converted to of_property_read_bool() since >> >> it is a boolean property, not a RTAS function token. >> > >> > Small nit, but you could do that at the query site unless you >> > were going to start using ibm,os-term without the extended >> > capability. >> >> I'm unsure that this is what you're suggesting, but we don't want to use >> of_property_read_bool() in this context either, because it has the same >> undesirable qualities as rtas_token(). > > I mean rtas_initialize() could do > > if (of_property_read_bool(rtas.dev, "ibm,extended-os-term")) > ibm_os_term_token = rtas_token("ibm,os-term"); Oh of course, thanks. Since I need to do a v2 anyway, I'll make that change.