| Message ID | 20200113130118.27969-1-clg@kaod.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc/xive: discard ESB load value when interrupt is invalid | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (20862247a368dbb75d6e97d82345999adaacf3cc) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Cédric Le Goater <clg@kaod.org> writes: > On 1/13/20 2:01 PM, Cédric Le Goater wrote: >> From: Frederic Barrat <fbarrat@linux.ibm.com> >> >> A load on an ESB page returning all 1's means that the underlying >> device has invalidated the access to the PQ state of the interrupt >> through mmio. It may happen, for example when querying a PHB interrupt >> while the PHB is in an error state. >> >> In that case, we should consider the interrupt to be invalid when >> checking its state in the irq_get_irqchip_state() handler. > > > and we need also these tags : > > Fixes: da15c03b047d ("powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race") > Cc: stable@vger.kernel.org # v5.3+ I added those, although it's v5.4+, as the offending commit was first included in v5.4-rc1. cheers
On 1/14/20 2:14 AM, Michael Ellerman wrote: > Cédric Le Goater <clg@kaod.org> writes: >> On 1/13/20 2:01 PM, Cédric Le Goater wrote: >>> From: Frederic Barrat <fbarrat@linux.ibm.com> >>> >>> A load on an ESB page returning all 1's means that the underlying >>> device has invalidated the access to the PQ state of the interrupt >>> through mmio. It may happen, for example when querying a PHB interrupt >>> while the PHB is in an error state. >>> >>> In that case, we should consider the interrupt to be invalid when >>> checking its state in the irq_get_irqchip_state() handler. >> >> >> and we need also these tags : >> >> Fixes: da15c03b047d ("powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race") >> Cc: stable@vger.kernel.org # v5.3+ > > I added those, although it's v5.4+, as the offending commit was first > included in v5.4-rc1. Ah yes. I mistook the merge tag of the branch used for the PR (v5.3-rc2) Thanks, C.
On Tue, 14 Jan 2020 08:44:54 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 1/14/20 2:14 AM, Michael Ellerman wrote: > > Cédric Le Goater <clg@kaod.org> writes: > >> On 1/13/20 2:01 PM, Cédric Le Goater wrote: > >>> From: Frederic Barrat <fbarrat@linux.ibm.com> > >>> > >>> A load on an ESB page returning all 1's means that the underlying > >>> device has invalidated the access to the PQ state of the interrupt > >>> through mmio. It may happen, for example when querying a PHB interrupt > >>> while the PHB is in an error state. > >>> > >>> In that case, we should consider the interrupt to be invalid when > >>> checking its state in the irq_get_irqchip_state() handler. > >> > >> > >> and we need also these tags : > >> > >> Fixes: da15c03b047d ("powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race") > >> Cc: stable@vger.kernel.org # v5.3+ > > > > I added those, although it's v5.4+, as the offending commit was first > > included in v5.4-rc1. > > Ah yes. I mistook the merge tag of the branch used for the PR (v5.3-rc2) > You might want to use 'git tag --contains': [greg@bahia kernel-linus]$ git tag --contains da15c03b047d for-linus kvm-5.4-2 next-20191118 next-20191126 tags/kvm-5.4-1 tags/kvm-5.4-2 v5.4 v5.4-rc1 v5.4-rc2 v5.4-rc3 v5.4-rc4 v5.4-rc5 v5.4-rc6 v5.4-rc7 v5.4-rc8 v5.5-rc1 > Thanks, > > C. >
Greg Kurz <groug@kaod.org> writes: > On Tue, 14 Jan 2020 08:44:54 +0100 > Cédric Le Goater <clg@kaod.org> wrote: >> On 1/14/20 2:14 AM, Michael Ellerman wrote: >> > Cédric Le Goater <clg@kaod.org> writes: >> >> On 1/13/20 2:01 PM, Cédric Le Goater wrote: >> >>> From: Frederic Barrat <fbarrat@linux.ibm.com> >> >>> >> >>> A load on an ESB page returning all 1's means that the underlying >> >>> device has invalidated the access to the PQ state of the interrupt >> >>> through mmio. It may happen, for example when querying a PHB interrupt >> >>> while the PHB is in an error state. >> >>> >> >>> In that case, we should consider the interrupt to be invalid when >> >>> checking its state in the irq_get_irqchip_state() handler. >> >> >> >> >> >> and we need also these tags : >> >> >> >> Fixes: da15c03b047d ("powerpc/xive: Implement get_irqchip_state method for XIVE to fix shutdown race") >> >> Cc: stable@vger.kernel.org # v5.3+ >> > >> > I added those, although it's v5.4+, as the offending commit was first >> > included in v5.4-rc1. >> >> Ah yes. I mistook the merge tag of the branch used for the PR (v5.3-rc2) >> > > You might want to use 'git tag --contains': > > [greg@bahia kernel-linus]$ git tag --contains da15c03b047d > for-linus > kvm-5.4-2 > next-20191118 > next-20191126 > tags/kvm-5.4-1 > tags/kvm-5.4-2 > v5.4 > v5.4-rc1 Or: $ git describe --match "v[0-9]*" --contains da15c03b047d v5.4-rc1~99^2~134^2~17 cheers
>> You might want to use 'git tag --contains': >> >> [greg@bahia kernel-linus]$ git tag --contains da15c03b047d >> for-linus >> kvm-5.4-2 >> next-20191118 >> next-20191126 >> tags/kvm-5.4-1 >> tags/kvm-5.4-2 >> v5.4 >> v5.4-rc1 > > Or: > > $ git describe --match "v[0-9]*" --contains da15c03b047d > v5.4-rc1~99^2~134^2~17 Nice. I am adding this command to my git aliases. Thanks, C.
diff --git a/arch/powerpc/include/asm/xive-regs.h b/arch/powerpc/include/asm/xive-regs.h index f2dfcd50a2d3..33aee7490cbb 100644 --- a/arch/powerpc/include/asm/xive-regs.h +++ b/arch/powerpc/include/asm/xive-regs.h @@ -39,6 +39,7 @@ #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 +#define XIVE_ESB_INVALID 0xFF /* * Thread Management (aka "TM") registers diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c index f5fadbd2533a..9651ca061828 100644 --- a/arch/powerpc/sysdev/xive/common.c +++ b/arch/powerpc/sysdev/xive/common.c @@ -972,12 +972,21 @@ static int xive_get_irqchip_state(struct irq_data *data, enum irqchip_irq_state which, bool *state) { struct xive_irq_data *xd = irq_data_get_irq_handler_data(data); + u8 pq; switch (which) { case IRQCHIP_STATE_ACTIVE: - *state = !xd->stale_p && - (xd->saved_p || - !!(xive_esb_read(xd, XIVE_ESB_GET) & XIVE_ESB_VAL_P)); + pq = xive_esb_read(xd, XIVE_ESB_GET); + + /* + * The esb value being all 1's means we couldn't get + * the PQ state of the interrupt through mmio. It may + * happen, for example when querying a PHB interrupt + * while the PHB is in an error state. We consider the + * interrupt to be inactive in that case. + */ + *state = (pq != XIVE_ESB_INVALID) && !xd->stale_p && + (xd->saved_p || !!(pq & XIVE_ESB_VAL_P)); return 0; default: return -EINVAL;