| Message ID | 1541697162-1926-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8d4a862276a9c30a269d368d324fb56529e6d5fd |
| Headers | show |
| Series | powerpc/xmon: Fix invocation inside lock region | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
| snowpatch_ozlabs/build-ppc64le | success | Test build-ppc64le on branch next |
| snowpatch_ozlabs/build-ppc64be | success | Test build-ppc64be on branch next |
| snowpatch_ozlabs/build-ppc64e | success | Test build-ppc64e on branch next |
| snowpatch_ozlabs/build-ppc32 | success | Test build-ppc32 on branch next |
Breno Leitao <leitao@debian.org> writes: > Currently xmon needs to get devtree_lock (through rtas_token()) during its > invocation (at crash time). If there is a crash while devtree_lock is being > held, then xmon tries to get the lock but spins forever and never get into > the interactive debugger, as in the following case: > > int *ptr = NULL; > raw_spin_lock_irqsave(&devtree_lock, flags); > *ptr = 0xdeadbeef; > > This patch avoids calling rtas_token(), thus trying to get the same lock, > at crash time. This new mechanism proposes getting the token at > initialization time (xmon_init()) and just consuming it at crash time. > > This would allow xmon to be possible invoked independent of devtree_lock > being held or not. > > Signed-off-by: Breno Leitao <leitao@debian.org> I'm not familiar with xmon code but I was curious, and for what it's worth: Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > --- > arch/powerpc/xmon/xmon.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index 36b8dc47a3c3..b566203d09c5 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -75,6 +75,9 @@ static int xmon_gate; > #define xmon_owner 0 > #endif /* CONFIG_SMP */ > > +#ifdef CONFIG_PPC_PSERIES > +static int set_indicator_token = RTAS_UNKNOWN_SERVICE; > +#endif > static unsigned long in_xmon __read_mostly = 0; > static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT); > > @@ -358,7 +361,6 @@ static inline void disable_surveillance(void) > #ifdef CONFIG_PPC_PSERIES > /* Since this can't be a module, args should end up below 4GB. */ > static struct rtas_args args; > - int token; > > /* > * At this point we have got all the cpus we can into > @@ -367,11 +369,11 @@ static inline void disable_surveillance(void) > * If we did try to take rtas.lock there would be a > * real possibility of deadlock. > */ > - token = rtas_token("set-indicator"); > - if (token == RTAS_UNKNOWN_SERVICE) > + if (set_indicator_token == RTAS_UNKNOWN_SERVICE) > return; > > - rtas_call_unlocked(&args, token, 3, 1, NULL, SURVEILLANCE_TOKEN, 0, 0); > + rtas_call_unlocked(&args, set_indicator_token, 3, 1, NULL, > + SURVEILLANCE_TOKEN, 0, 0); > > #endif /* CONFIG_PPC_PSERIES */ > } > @@ -3688,6 +3690,14 @@ static void xmon_init(int enable) > __debugger_iabr_match = xmon_iabr_match; > __debugger_break_match = xmon_break_match; > __debugger_fault_handler = xmon_fault_handler; > + > +#ifdef CONFIG_PPC_PSERIES > + /* > + * Get the token here to avoid trying to get a lock > + * during the crash, causing a deadlock. > + */ > + set_indicator_token = rtas_token("set-indicator"); > +#endif > } else { > __debugger = NULL; > __debugger_ipi = NULL; -- Thiago Jung Bauermann IBM Linux Technology Center
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index 36b8dc47a3c3..b566203d09c5 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -75,6 +75,9 @@ static int xmon_gate; #define xmon_owner 0 #endif /* CONFIG_SMP */ +#ifdef CONFIG_PPC_PSERIES +static int set_indicator_token = RTAS_UNKNOWN_SERVICE; +#endif static unsigned long in_xmon __read_mostly = 0; static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT); @@ -358,7 +361,6 @@ static inline void disable_surveillance(void) #ifdef CONFIG_PPC_PSERIES /* Since this can't be a module, args should end up below 4GB. */ static struct rtas_args args; - int token; /* * At this point we have got all the cpus we can into @@ -367,11 +369,11 @@ static inline void disable_surveillance(void) * If we did try to take rtas.lock there would be a * real possibility of deadlock. */ - token = rtas_token("set-indicator"); - if (token == RTAS_UNKNOWN_SERVICE) + if (set_indicator_token == RTAS_UNKNOWN_SERVICE) return; - rtas_call_unlocked(&args, token, 3, 1, NULL, SURVEILLANCE_TOKEN, 0, 0); + rtas_call_unlocked(&args, set_indicator_token, 3, 1, NULL, + SURVEILLANCE_TOKEN, 0, 0); #endif /* CONFIG_PPC_PSERIES */ } @@ -3688,6 +3690,14 @@ static void xmon_init(int enable) __debugger_iabr_match = xmon_iabr_match; __debugger_break_match = xmon_break_match; __debugger_fault_handler = xmon_fault_handler; + +#ifdef CONFIG_PPC_PSERIES + /* + * Get the token here to avoid trying to get a lock + * during the crash, causing a deadlock. + */ + set_indicator_token = rtas_token("set-indicator"); +#endif } else { __debugger = NULL; __debugger_ipi = NULL;
Currently xmon needs to get devtree_lock (through rtas_token()) during its invocation (at crash time). If there is a crash while devtree_lock is being held, then xmon tries to get the lock but spins forever and never get into the interactive debugger, as in the following case: int *ptr = NULL; raw_spin_lock_irqsave(&devtree_lock, flags); *ptr = 0xdeadbeef; This patch avoids calling rtas_token(), thus trying to get the same lock, at crash time. This new mechanism proposes getting the token at initialization time (xmon_init()) and just consuming it at crash time. This would allow xmon to be possible invoked independent of devtree_lock being held or not. Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/xmon/xmon.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)