linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
@ 2022-04-18 15:02 Kuppuswamy Sathyanarayanan
  2022-05-11 15:19 ` Sathyanarayanan Kuppuswamy
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2022-04-18 15:02 UTC (permalink / raw)
  To: Bjorn Helgaas, Russell Currey, Oliver OHalloran
  Cc: linux-pci, linuxppc-dev, linux-kernel, Kuppuswamy Sathyanarayanan

Currently the aer_irq() handler returns IRQ_NONE for cases without bits
PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
assumption is incorrect.

Consider a scenario where aer_irq() is triggered for a correctable
error, and while we process the error and before we clear the error
status in "Root Error Status" register, if the same kind of error
is triggered again, since aer_irq() only clears events it saw, the
multi-bit error is left in tact. This will cause the interrupt to fire
again, resulting in entering aer_irq() with just the multi-bit error
logged in the "Root Error Status" register.

Repeated AER recovery test has revealed this condition does happen
and this prevents any new interrupt from being triggered. Allow to
process interrupt even if only multi-correctable (BIT 1) or
multi-uncorrectable bit (BIT 3) is set.

Also note that, for cases with only multi-bit error is set, since this
is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
zero or some junk value. So we cannot cleanly process this error
information using aer_isr_one_error(). All we are attempting with this
fix is to make sure error interrupt processing can continue in this
scenario.

This error can be reproduced by making following changes to the
aer_irq() function and by executing the given test commands.

 static irqreturn_t aer_irq(int irq, void *context)
         struct aer_err_source e_src = {};

         pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
				&e_src.status);
 +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
 +		e_src.status);
         if (!(e_src.status & AER_ERR_STATUS_MASK))
                 return IRQ_NONE;

 +       mdelay(5000);

 # Prep injection data for a correctable error.
 $ cd /sys/kernel/debug/apei/einj
 $ echo 0x00000040 > error_type
 $ echo 0x4 > flags
 $ echo 0x891000 > param4

 # Root Error Status is initially clear
 $ setpci -s <Dev ID> ECAP0001+0x30.w
 0000

 # Inject one error
 $ echo 1 > error_inject

 # Interrupt received
 pcieport <Dev ID>: AER: Root Error Status 0001

 # Inject another error (within 5 seconds)
 $ echo 1 > error_inject

 # You will get a new IRQ with only multiple ERR_COR bit set
 pcieport <Dev ID>: AER: Root Error Status 0002

Currently, the above issue has been only reproduced in the ICL server
platform.

[Eric: proposed reproducing steps]
Fixes: 4696b828ca37 ("PCI/AER: Hoist aerdrv.c, aer_inject.c up to drivers/pci/pcie/")
Reported-by: Eric Badger <ebadger@purestorage.com>
Reviewed-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v2:
 * Added more details to the commit log.
 * Rebased on v5.18-rc1.

Changes since v1:
 * Added Fixes tag.
 * Included reproducing steps proposed by Eric.

 drivers/pci/pcie/aer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 9fa1f97e5b27..7952e5efd6cf 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -101,6 +101,11 @@ struct aer_stats {
 #define ERR_COR_ID(d)			(d & 0xffff)
 #define ERR_UNCOR_ID(d)			(d >> 16)
 
+#define AER_ERR_STATUS_MASK		(PCI_ERR_ROOT_UNCOR_RCV |	\
+					PCI_ERR_ROOT_COR_RCV |		\
+					PCI_ERR_ROOT_MULTI_COR_RCV |	\
+					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
+
 static int pcie_aer_disable;
 static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
 
@@ -1196,7 +1201,7 @@ static irqreturn_t aer_irq(int irq, void *context)
 	struct aer_err_source e_src = {};
 
 	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status);
-	if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))
+	if (!(e_src.status & AER_ERR_STATUS_MASK))
 		return IRQ_NONE;
 
 	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_ERR_SRC, &e_src.id);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-04-18 15:02 [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly Kuppuswamy Sathyanarayanan
@ 2022-05-11 15:19 ` Sathyanarayanan Kuppuswamy
  2022-05-11 23:27 ` Bjorn Helgaas
  2022-05-11 23:40 ` Bjorn Helgaas
  2 siblings, 0 replies; 7+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2022-05-11 15:19 UTC (permalink / raw)
  To: Bjorn Helgaas, Russell Currey, Oliver OHalloran
  Cc: linux-pci, linuxppc-dev, linux-kernel

Hi Bjorn,

On 4/18/22 8:02 AM, Kuppuswamy Sathyanarayanan wrote:
> Currently the aer_irq() handler returns IRQ_NONE for cases without bits
> PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
> assumption is incorrect.
> 
> Consider a scenario where aer_irq() is triggered for a correctable
> error, and while we process the error and before we clear the error
> status in "Root Error Status" register, if the same kind of error
> is triggered again, since aer_irq() only clears events it saw, the
> multi-bit error is left in tact. This will cause the interrupt to fire
> again, resulting in entering aer_irq() with just the multi-bit error
> logged in the "Root Error Status" register.
> 
> Repeated AER recovery test has revealed this condition does happen
> and this prevents any new interrupt from being triggered. Allow to
> process interrupt even if only multi-correctable (BIT 1) or
> multi-uncorrectable bit (BIT 3) is set.
> 
> Also note that, for cases with only multi-bit error is set, since this
> is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
> zero or some junk value. So we cannot cleanly process this error
> information using aer_isr_one_error(). All we are attempting with this
> fix is to make sure error interrupt processing can continue in this
> scenario.
> 
> This error can be reproduced by making following changes to the
> aer_irq() function and by executing the given test commands.
> 
>   static irqreturn_t aer_irq(int irq, void *context)
>           struct aer_err_source e_src = {};
> 
>           pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
> 				&e_src.status);
>   +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
>   +		e_src.status);
>           if (!(e_src.status & AER_ERR_STATUS_MASK))
>                   return IRQ_NONE;
> 
>   +       mdelay(5000);
> 
>   # Prep injection data for a correctable error.
>   $ cd /sys/kernel/debug/apei/einj
>   $ echo 0x00000040 > error_type
>   $ echo 0x4 > flags
>   $ echo 0x891000 > param4
> 
>   # Root Error Status is initially clear
>   $ setpci -s <Dev ID> ECAP0001+0x30.w
>   0000
> 
>   # Inject one error
>   $ echo 1 > error_inject
> 
>   # Interrupt received
>   pcieport <Dev ID>: AER: Root Error Status 0001
> 
>   # Inject another error (within 5 seconds)
>   $ echo 1 > error_inject
> 
>   # You will get a new IRQ with only multiple ERR_COR bit set
>   pcieport <Dev ID>: AER: Root Error Status 0002
> 
> Currently, the above issue has been only reproduced in the ICL server
> platform.
> 
> [Eric: proposed reproducing steps]
> Fixes: 4696b828ca37 ("PCI/AER: Hoist aerdrv.c, aer_inject.c up to drivers/pci/pcie/")
> Reported-by: Eric Badger <ebadger@purestorage.com>
> Reviewed-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 

Any comments on this patch? I'm wondering whether you are expecting any
changes to be done to it. Please let me know.

> Changes since v2:
>   * Added more details to the commit log.
>   * Rebased on v5.18-rc1.
> 
> Changes since v1:
>   * Added Fixes tag.
>   * Included reproducing steps proposed by Eric.
> 
>   drivers/pci/pcie/aer.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 9fa1f97e5b27..7952e5efd6cf 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -101,6 +101,11 @@ struct aer_stats {
>   #define ERR_COR_ID(d)			(d & 0xffff)
>   #define ERR_UNCOR_ID(d)			(d >> 16)
>   
> +#define AER_ERR_STATUS_MASK		(PCI_ERR_ROOT_UNCOR_RCV |	\
> +					PCI_ERR_ROOT_COR_RCV |		\
> +					PCI_ERR_ROOT_MULTI_COR_RCV |	\
> +					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
> +
>   static int pcie_aer_disable;
>   static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>   
> @@ -1196,7 +1201,7 @@ static irqreturn_t aer_irq(int irq, void *context)
>   	struct aer_err_source e_src = {};
>   
>   	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status);
> -	if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))
> +	if (!(e_src.status & AER_ERR_STATUS_MASK))
>   		return IRQ_NONE;
>   
>   	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_ERR_SRC, &e_src.id);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-04-18 15:02 [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly Kuppuswamy Sathyanarayanan
  2022-05-11 15:19 ` Sathyanarayanan Kuppuswamy
@ 2022-05-11 23:27 ` Bjorn Helgaas
  2022-05-11 23:39   ` Sathyanarayanan Kuppuswamy
  2022-05-11 23:40 ` Bjorn Helgaas
  2 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 23:27 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Bjorn Helgaas, Russell Currey, Oliver OHalloran, linux-pci,
	linuxppc-dev, linux-kernel

On Mon, Apr 18, 2022 at 03:02:37PM +0000, Kuppuswamy Sathyanarayanan wrote:
> Currently the aer_irq() handler returns IRQ_NONE for cases without bits
> PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
> assumption is incorrect.
> 
> Consider a scenario where aer_irq() is triggered for a correctable
> error, and while we process the error and before we clear the error
> status in "Root Error Status" register, if the same kind of error
> is triggered again, since aer_irq() only clears events it saw, the
> multi-bit error is left in tact. This will cause the interrupt to fire
> again, resulting in entering aer_irq() with just the multi-bit error
> logged in the "Root Error Status" register.
> 
> Repeated AER recovery test has revealed this condition does happen
> and this prevents any new interrupt from being triggered. Allow to
> process interrupt even if only multi-correctable (BIT 1) or
> multi-uncorrectable bit (BIT 3) is set.
> 
> Also note that, for cases with only multi-bit error is set, since this
> is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
> zero or some junk value. So we cannot cleanly process this error
> information using aer_isr_one_error(). All we are attempting with this
> fix is to make sure error interrupt processing can continue in this
> scenario.
> 
> This error can be reproduced by making following changes to the
> aer_irq() function and by executing the given test commands.
> 
>  static irqreturn_t aer_irq(int irq, void *context)
>          struct aer_err_source e_src = {};
> 
>          pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
> 				&e_src.status);
>  +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
>  +		e_src.status);
>          if (!(e_src.status & AER_ERR_STATUS_MASK))
>                  return IRQ_NONE;
> 
>  +       mdelay(5000);
> 
>  # Prep injection data for a correctable error.
>  $ cd /sys/kernel/debug/apei/einj
>  $ echo 0x00000040 > error_type
>  $ echo 0x4 > flags
>  $ echo 0x891000 > param4
> 
>  # Root Error Status is initially clear
>  $ setpci -s <Dev ID> ECAP0001+0x30.w
>  0000
> 
>  # Inject one error
>  $ echo 1 > error_inject
> 
>  # Interrupt received
>  pcieport <Dev ID>: AER: Root Error Status 0001
> 
>  # Inject another error (within 5 seconds)
>  $ echo 1 > error_inject
> 
>  # You will get a new IRQ with only multiple ERR_COR bit set
>  pcieport <Dev ID>: AER: Root Error Status 0002
> 
> Currently, the above issue has been only reproduced in the ICL server
> platform.
> 
> [Eric: proposed reproducing steps]
> Fixes: 4696b828ca37 ("PCI/AER: Hoist aerdrv.c, aer_inject.c up to drivers/pci/pcie/")

4696b828ca37 only *moves* drivers/pci/pcie/aer/aerdrv.c to
drivers/pci/pcie/aer.c, so I don't think it's related.

I think the actual change of interest is e167bfcaa4cd ("PCI: aerdrv:
remove magical ROOT_ERR_STATUS_MASKS") [1].  It looks like we did
exactly what you propose before that commit.

I can update this unless you disagree.

[1] https://git.kernel.org/linus/e167bfcaa4cd

> Reported-by: Eric Badger <ebadger@purestorage.com>
> Reviewed-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> Changes since v2:
>  * Added more details to the commit log.
>  * Rebased on v5.18-rc1.
> 
> Changes since v1:
>  * Added Fixes tag.
>  * Included reproducing steps proposed by Eric.
> 
>  drivers/pci/pcie/aer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 9fa1f97e5b27..7952e5efd6cf 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -101,6 +101,11 @@ struct aer_stats {
>  #define ERR_COR_ID(d)			(d & 0xffff)
>  #define ERR_UNCOR_ID(d)			(d >> 16)
>  
> +#define AER_ERR_STATUS_MASK		(PCI_ERR_ROOT_UNCOR_RCV |	\
> +					PCI_ERR_ROOT_COR_RCV |		\
> +					PCI_ERR_ROOT_MULTI_COR_RCV |	\
> +					PCI_ERR_ROOT_MULTI_UNCOR_RCV)
> +
>  static int pcie_aer_disable;
>  static pci_ers_result_t aer_root_reset(struct pci_dev *dev);
>  
> @@ -1196,7 +1201,7 @@ static irqreturn_t aer_irq(int irq, void *context)
>  	struct aer_err_source e_src = {};
>  
>  	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status);
> -	if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))
> +	if (!(e_src.status & AER_ERR_STATUS_MASK))
>  		return IRQ_NONE;
>  
>  	pci_read_config_dword(rp, aer + PCI_ERR_ROOT_ERR_SRC, &e_src.id);
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-05-11 23:27 ` Bjorn Helgaas
@ 2022-05-11 23:39   ` Sathyanarayanan Kuppuswamy
  0 siblings, 0 replies; 7+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2022-05-11 23:39 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Bjorn Helgaas, Russell Currey, Oliver OHalloran, linux-pci,
	linuxppc-dev, linux-kernel



On 5/11/22 4:27 PM, Bjorn Helgaas wrote:
>> [Eric: proposed reproducing steps]
>> Fixes: 4696b828ca37 ("PCI/AER: Hoist aerdrv.c, aer_inject.c up to drivers/pci/pcie/")
> 4696b828ca37 only*moves*  drivers/pci/pcie/aer/aerdrv.c to
> drivers/pci/pcie/aer.c, so I don't think it's related.
> 
> I think the actual change of interest is e167bfcaa4cd ("PCI: aerdrv:
> remove magical ROOT_ERR_STATUS_MASKS") [1].  It looks like we did
> exactly what you propose before that commit.
> 
> I can update this unless you disagree.
> 
> [1]https://git.kernel.org/linus/e167bfcaa4cd
> 

Agree. Please update it.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-04-18 15:02 [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly Kuppuswamy Sathyanarayanan
  2022-05-11 15:19 ` Sathyanarayanan Kuppuswamy
  2022-05-11 23:27 ` Bjorn Helgaas
@ 2022-05-11 23:40 ` Bjorn Helgaas
  2022-05-12  0:29   ` Sathyanarayanan Kuppuswamy
  2 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2022-05-11 23:40 UTC (permalink / raw)
  To: Kuppuswamy Sathyanarayanan
  Cc: Bjorn Helgaas, Russell Currey, Oliver OHalloran, linux-pci,
	linuxppc-dev, linux-kernel

On Mon, Apr 18, 2022 at 03:02:37PM +0000, Kuppuswamy Sathyanarayanan wrote:
> Currently the aer_irq() handler returns IRQ_NONE for cases without bits
> PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
> assumption is incorrect.
> 
> Consider a scenario where aer_irq() is triggered for a correctable
> error, and while we process the error and before we clear the error
> status in "Root Error Status" register, if the same kind of error
> is triggered again, since aer_irq() only clears events it saw, the
> multi-bit error is left in tact. This will cause the interrupt to fire
> again, resulting in entering aer_irq() with just the multi-bit error
> logged in the "Root Error Status" register.
> 
> Repeated AER recovery test has revealed this condition does happen
> and this prevents any new interrupt from being triggered. Allow to
> process interrupt even if only multi-correctable (BIT 1) or
> multi-uncorrectable bit (BIT 3) is set.
> 
> Also note that, for cases with only multi-bit error is set, since this
> is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
> zero or some junk value. So we cannot cleanly process this error
> information using aer_isr_one_error(). All we are attempting with this
> fix is to make sure error interrupt processing can continue in this
> scenario.
> 
> This error can be reproduced by making following changes to the
> aer_irq() function and by executing the given test commands.
> 
>  static irqreturn_t aer_irq(int irq, void *context)
>          struct aer_err_source e_src = {};
> 
>          pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
> 				&e_src.status);
>  +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
>  +		e_src.status);
>          if (!(e_src.status & AER_ERR_STATUS_MASK))

Do you mean

  if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))

here?  AER_ERR_STATUS_MASK would be after this fix.

>                  return IRQ_NONE;
> 
>  +       mdelay(5000);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-05-11 23:40 ` Bjorn Helgaas
@ 2022-05-12  0:29   ` Sathyanarayanan Kuppuswamy
  2022-05-17 22:28     ` Bjorn Helgaas
  0 siblings, 1 reply; 7+ messages in thread
From: Sathyanarayanan Kuppuswamy @ 2022-05-12  0:29 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Bjorn Helgaas, Russell Currey, Oliver OHalloran, linux-pci,
	linuxppc-dev, linux-kernel



On 5/11/22 4:40 PM, Bjorn Helgaas wrote:
> On Mon, Apr 18, 2022 at 03:02:37PM +0000, Kuppuswamy Sathyanarayanan wrote:
>> Currently the aer_irq() handler returns IRQ_NONE for cases without bits
>> PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
>> assumption is incorrect.
>>
>> Consider a scenario where aer_irq() is triggered for a correctable
>> error, and while we process the error and before we clear the error
>> status in "Root Error Status" register, if the same kind of error
>> is triggered again, since aer_irq() only clears events it saw, the
>> multi-bit error is left in tact. This will cause the interrupt to fire
>> again, resulting in entering aer_irq() with just the multi-bit error
>> logged in the "Root Error Status" register.
>>
>> Repeated AER recovery test has revealed this condition does happen
>> and this prevents any new interrupt from being triggered. Allow to
>> process interrupt even if only multi-correctable (BIT 1) or
>> multi-uncorrectable bit (BIT 3) is set.
>>
>> Also note that, for cases with only multi-bit error is set, since this
>> is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
>> zero or some junk value. So we cannot cleanly process this error
>> information using aer_isr_one_error(). All we are attempting with this
>> fix is to make sure error interrupt processing can continue in this
>> scenario.
>>
>> This error can be reproduced by making following changes to the
>> aer_irq() function and by executing the given test commands.
>>
>>   static irqreturn_t aer_irq(int irq, void *context)
>>           struct aer_err_source e_src = {};
>>
>>           pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
>> 				&e_src.status);
>>   +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
>>   +		e_src.status);
>>           if (!(e_src.status & AER_ERR_STATUS_MASK))
> 
> Do you mean
> 
>    if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))
> 
> here?  AER_ERR_STATUS_MASK would be after this fix.

Yes. You are correct. Do you want me to update it and Fixes tag
and send next version?

> 
>>                   return IRQ_NONE;
>>
>>   +       mdelay(5000);

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly
  2022-05-12  0:29   ` Sathyanarayanan Kuppuswamy
@ 2022-05-17 22:28     ` Bjorn Helgaas
  0 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2022-05-17 22:28 UTC (permalink / raw)
  To: Sathyanarayanan Kuppuswamy
  Cc: linux-pci, linux-kernel, Oliver OHalloran, Bjorn Helgaas, linuxppc-dev

On Wed, May 11, 2022 at 05:29:45PM -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 5/11/22 4:40 PM, Bjorn Helgaas wrote:
> > On Mon, Apr 18, 2022 at 03:02:37PM +0000, Kuppuswamy Sathyanarayanan wrote:
> > > Currently the aer_irq() handler returns IRQ_NONE for cases without bits
> > > PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this
> > > assumption is incorrect.
> > > 
> > > Consider a scenario where aer_irq() is triggered for a correctable
> > > error, and while we process the error and before we clear the error
> > > status in "Root Error Status" register, if the same kind of error
> > > is triggered again, since aer_irq() only clears events it saw, the
> > > multi-bit error is left in tact. This will cause the interrupt to fire
> > > again, resulting in entering aer_irq() with just the multi-bit error
> > > logged in the "Root Error Status" register.
> > > 
> > > Repeated AER recovery test has revealed this condition does happen
> > > and this prevents any new interrupt from being triggered. Allow to
> > > process interrupt even if only multi-correctable (BIT 1) or
> > > multi-uncorrectable bit (BIT 3) is set.
> > > 
> > > Also note that, for cases with only multi-bit error is set, since this
> > > is not the first occurrence of the error, PCI_ERR_ROOT_ERR_SRC may have
> > > zero or some junk value. So we cannot cleanly process this error
> > > information using aer_isr_one_error(). All we are attempting with this
> > > fix is to make sure error interrupt processing can continue in this
> > > scenario.
> > > 
> > > This error can be reproduced by making following changes to the
> > > aer_irq() function and by executing the given test commands.
> > > 
> > >   static irqreturn_t aer_irq(int irq, void *context)
> > >           struct aer_err_source e_src = {};
> > > 
> > >           pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS,
> > > 				&e_src.status);
> > >   +       pci_dbg(pdev->port, "Root Error Status: %04x\n",
> > >   +		e_src.status);
> > >           if (!(e_src.status & AER_ERR_STATUS_MASK))
> > 
> > Do you mean
> > 
> >    if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV)))
> > 
> > here?  AER_ERR_STATUS_MASK would be after this fix.
> 
> Yes. You are correct. Do you want me to update it and Fixes tag
> and send next version?

I moved the repro details to a bugzilla, updated the commit log as
below, and applied to pci/error for v5.19, thanks!


commit 203926da2bff ("PCI/AER: Clear MULTI_ERR_COR/UNCOR_RCV bits")
Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Date:   Mon Apr 18 15:02:37 2022 +0000

    PCI/AER: Clear MULTI_ERR_COR/UNCOR_RCV bits
    
    When a Root Port or Root Complex Event Collector receives an error Message
    e.g., ERR_COR, it sets PCI_ERR_ROOT_COR_RCV in the Root Error Status
    register and logs the Requester ID in the Error Source Identification
    register.  If it receives a second ERR_COR Message before software clears
    PCI_ERR_ROOT_COR_RCV, hardware sets PCI_ERR_ROOT_MULTI_COR_RCV and the
    Requester ID is lost.
    
    In the following scenario, PCI_ERR_ROOT_MULTI_COR_RCV was never cleared:
    
      - hardware receives ERR_COR message
      - hardware sets PCI_ERR_ROOT_COR_RCV
      - aer_irq() entered
      - aer_irq(): status = pci_read_config_dword(PCI_ERR_ROOT_STATUS)
      - aer_irq(): now status == PCI_ERR_ROOT_COR_RCV
      - hardware receives second ERR_COR message
      - hardware sets PCI_ERR_ROOT_MULTI_COR_RCV
      - aer_irq(): pci_write_config_dword(PCI_ERR_ROOT_STATUS, status)
      - PCI_ERR_ROOT_COR_RCV is cleared; PCI_ERR_ROOT_MULTI_COR_RCV is set
      - aer_irq() entered again
      - aer_irq(): status = pci_read_config_dword(PCI_ERR_ROOT_STATUS)
      - aer_irq(): now status == PCI_ERR_ROOT_MULTI_COR_RCV
      - aer_irq() exits because PCI_ERR_ROOT_COR_RCV not set
      - PCI_ERR_ROOT_MULTI_COR_RCV is still set
    
    The same problem occurred with ERR_NONFATAL/ERR_FATAL Messages and
    PCI_ERR_ROOT_UNCOR_RCV and PCI_ERR_ROOT_MULTI_UNCOR_RCV.
    
    Fix the problem by queueing an AER event and clearing the Root Error Status
    bits when any of these bits are set:
    
      PCI_ERR_ROOT_COR_RCV
      PCI_ERR_ROOT_UNCOR_RCV
      PCI_ERR_ROOT_MULTI_COR_RCV
      PCI_ERR_ROOT_MULTI_UNCOR_RCV
    
    See the bugzilla link for details from Eric about how to reproduce this
    problem.
    
    [bhelgaas: commit log, move repro details to bugzilla]
    Fixes: e167bfcaa4cd ("PCI: aerdrv: remove magical ROOT_ERR_STATUS_MASKS")
    Link: https://bugzilla.kernel.org/show_bug.cgi?id=215992
    Link: https://lore.kernel.org/r/20220418150237.1021519-1-sathyanarayanan.kuppuswamy@linux.intel.com
    Reported-by: Eric Badger <ebadger@purestorage.com>
    Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Ashok Raj <ashok.raj@intel.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-05-17 22:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-18 15:02 [PATCH v3] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly Kuppuswamy Sathyanarayanan
2022-05-11 15:19 ` Sathyanarayanan Kuppuswamy
2022-05-11 23:27 ` Bjorn Helgaas
2022-05-11 23:39   ` Sathyanarayanan Kuppuswamy
2022-05-11 23:40 ` Bjorn Helgaas
2022-05-12  0:29   ` Sathyanarayanan Kuppuswamy
2022-05-17 22:28     ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).