| Message ID | 1498612609-13677-1-git-send-email-akshay.adiga@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4d0d7c02df680740da41f5f92a238c35796ca5be |
| Headers | show |
On Wed, 28 Jun 2017 06:46:49 +0530 Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote: > pnv_wakeup_noloss expects R12 to contain SRR1 value to determine if > the wakeup reason is an HMI in CHECK_HMI_INTERRUPT. > > When we wakeup with ESL=0, SRR1 will not contain the wakeup reason, so > there is no point setting R12 to SRR1. > > However, we don't set R12 at all and R12 contains garbage, and still > being used to check HMI assuming that it had SRR1. causing the > OPAL msglog to be filled with the following print: > HMI: Received HMI interrupt: HMER = 0x0040000000000000 > > This patch clears R12 after waking up from stop with ESL=EC=0, so that > we don't accidentally enter the HMI handler in pnv_wakeup_noloss if > the R12[42:45] corresponds to HMI as wakeup reason. > > Bug existed prior to "commit 9d29250136f6 ("powerpc/64s/idle: Avoid SRR > usage in idle sleep/wake paths") but was never hit in practice > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> > Fixes: 9d29250136f6 ("powerpc/64s/idle: Avoid SRR usage in idle > sleep/wake paths") Thanks guys, appreciate you finding and fixing my bug :) I think this looks like the best fix. Really minor nitpick but you could adjust the line widths on the comment slightly (mpe might do that when merging). Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > --- > arch/powerpc/kernel/idle_book3s.S | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S > index 1ea14b9..34794fd 100644 > --- a/arch/powerpc/kernel/idle_book3s.S > +++ b/arch/powerpc/kernel/idle_book3s.S > @@ -256,6 +256,21 @@ power_enter_stop: > bne .Lhandle_esl_ec_set > IDLE_STATE_ENTER_SEQ(PPC_STOP) > li r3,0 /* Since we didn't lose state, return 0 */ > + /* > + * pnv_wakeup_noloss expects R12 to contain SRR1 value > + * to determine if the wakeup reason is an HMI in > + * CHECK_HMI_INTERRUPT. > + * > + * However, when we wakeup with ESL=0, > + * SRR1 will not contain the wakeup reason, > + * so there is no point setting R12 to SRR1. > + * > + * Further, we clear R12 here, so that we > + * don't accidentally enter the HMI > + * in pnv_wakeup_noloss if the > + * R12[42:45] == WAKE_HMI. > + */ > + li r12, 0 > b pnv_wakeup_noloss > > .Lhandle_esl_ec_set:
Nicholas Piggin <npiggin@gmail.com> writes: > On Wed, 28 Jun 2017 06:46:49 +0530 > Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote: > >> pnv_wakeup_noloss expects R12 to contain SRR1 value to determine if >> the wakeup reason is an HMI in CHECK_HMI_INTERRUPT. >> >> When we wakeup with ESL=0, SRR1 will not contain the wakeup reason, so >> there is no point setting R12 to SRR1. >> >> However, we don't set R12 at all and R12 contains garbage, and still >> being used to check HMI assuming that it had SRR1. causing the >> OPAL msglog to be filled with the following print: >> HMI: Received HMI interrupt: HMER = 0x0040000000000000 >> >> This patch clears R12 after waking up from stop with ESL=EC=0, so that >> we don't accidentally enter the HMI handler in pnv_wakeup_noloss if >> the R12[42:45] corresponds to HMI as wakeup reason. >> >> Bug existed prior to "commit 9d29250136f6 ("powerpc/64s/idle: Avoid SRR >> usage in idle sleep/wake paths") but was never hit in practice >> >> Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> >> Fixes: 9d29250136f6 ("powerpc/64s/idle: Avoid SRR usage in idle >> sleep/wake paths") > > Thanks guys, appreciate you finding and fixing my bug :) > > I think this looks like the best fix. Really minor nitpick but you > could adjust the line widths on the comment slightly (mpe might do > that when merging). You know me too well :} cheers
On Wed, 2017-06-28 at 01:16:49 UTC, Akshay Adiga wrote: > pnv_wakeup_noloss expects R12 to contain SRR1 value to determine if > the wakeup reason is an HMI in CHECK_HMI_INTERRUPT. > > When we wakeup with ESL=0, SRR1 will not contain the wakeup reason, so > there is no point setting R12 to SRR1. > > However, we don't set R12 at all and R12 contains garbage, and still > being used to check HMI assuming that it had SRR1. causing the > OPAL msglog to be filled with the following print: > HMI: Received HMI interrupt: HMER = 0x0040000000000000 > > This patch clears R12 after waking up from stop with ESL=EC=0, so that > we don't accidentally enter the HMI handler in pnv_wakeup_noloss if > the R12[42:45] corresponds to HMI as wakeup reason. > > Bug existed prior to "commit 9d29250136f6 ("powerpc/64s/idle: Avoid SRR > usage in idle sleep/wake paths") but was never hit in practice > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> > Fixes: 9d29250136f6 ("powerpc/64s/idle: Avoid SRR usage in idle > sleep/wake paths") > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/4d0d7c02df680740da41f5f92a238c cheers
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S index 1ea14b9..34794fd 100644 --- a/arch/powerpc/kernel/idle_book3s.S +++ b/arch/powerpc/kernel/idle_book3s.S @@ -256,6 +256,21 @@ power_enter_stop: bne .Lhandle_esl_ec_set IDLE_STATE_ENTER_SEQ(PPC_STOP) li r3,0 /* Since we didn't lose state, return 0 */ + /* + * pnv_wakeup_noloss expects R12 to contain SRR1 value + * to determine if the wakeup reason is an HMI in + * CHECK_HMI_INTERRUPT. + * + * However, when we wakeup with ESL=0, + * SRR1 will not contain the wakeup reason, + * so there is no point setting R12 to SRR1. + * + * Further, we clear R12 here, so that we + * don't accidentally enter the HMI + * in pnv_wakeup_noloss if the + * R12[42:45] == WAKE_HMI. + */ + li r12, 0 b pnv_wakeup_noloss .Lhandle_esl_ec_set:
pnv_wakeup_noloss expects R12 to contain SRR1 value to determine if the wakeup reason is an HMI in CHECK_HMI_INTERRUPT. When we wakeup with ESL=0, SRR1 will not contain the wakeup reason, so there is no point setting R12 to SRR1. However, we don't set R12 at all and R12 contains garbage, and still being used to check HMI assuming that it had SRR1. causing the OPAL msglog to be filled with the following print: HMI: Received HMI interrupt: HMER = 0x0040000000000000 This patch clears R12 after waking up from stop with ESL=EC=0, so that we don't accidentally enter the HMI handler in pnv_wakeup_noloss if the R12[42:45] corresponds to HMI as wakeup reason. Bug existed prior to "commit 9d29250136f6 ("powerpc/64s/idle: Avoid SRR usage in idle sleep/wake paths") but was never hit in practice Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> Fixes: 9d29250136f6 ("powerpc/64s/idle: Avoid SRR usage in idle sleep/wake paths") --- arch/powerpc/kernel/idle_book3s.S | 15 +++++++++++++++ 1 file changed, 15 insertions(+)