[PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Cédric Le Goater]

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.

Cc: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
[ clg: - wrote a commit log
       - introduced XIVE_ESB_INVALID ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/include/asm/xive-regs.h |  1 +
 arch/powerpc/sysdev/xive/common.c    | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

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;
-- 
2.21.1

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Cédric Le Goater]

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+



> Cc: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> [ clg: - wrote a commit log
>        - introduced XIVE_ESB_INVALID ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  arch/powerpc/include/asm/xive-regs.h |  1 +
>  arch/powerpc/sysdev/xive/common.c    | 15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> 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;
> 

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Michael Ellerman]

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

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Cédric Le Goater]

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. 

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Greg Kurz]

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. 
> 

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Michael Ellerman]

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

Re: [PATCH] powerpc/xive: discard ESB load value when interrupt is invalid

[Cédric Le Goater]

>> 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.