[PATCH] AER: aer_root_reset() non-native handling

[PATCH] AER: aer_root_reset() non-native handling

[Sean V Kelley]

If an OS has not been granted AER control via _OSC, then
the OS should not make changes to PCI_ERR_ROOT_COMMAND and
PCI_ERR_ROOT_STATUS related registers. Per section 4.5.1 of
the System Firmware Intermediary (SFI) _OSC and DPC Updates
ECN [1], this bit also covers these aspects of the PCI
Express Advanced Error Reporting. Further, the handling of
clear/enable of PCI_ERROR_ROOT_COMMAND when wrapped around
PCI_ERR_ROOT_STATUS should have no effect and be removed.
Based on the above and earlier discussion [2], make the
following changes:

Add a check for the native case (i.e., AER control via _OSC)
Re-order and remove some of the handling:
- clear PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
- do reset
- clear PCI_ERR_ROOT_STATUS
- enable PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK

to this:

- clear PCI_ERR_ROOT_STATUS
- do reset

The current "clear, reset, enable" order suggests that the reset
might cause errors that we should ignore. But I am unable to find a
reference and the clearing of PCI_ERR_ROOT_STATUS does not require them.

[1] System Firmware Intermediary (SFI) _OSC and DPC Updates ECN, Feb 24,
    2020, affecting PCI Firmware Specification, Rev. 3.2
    https://members.pcisig.com/wg/PCI-SIG/document/14076
[2] https://lore.kernel.org/linux-pci/20201020162820.GA370938@bjorn-Precision-5520/

Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
---
 drivers/pci/pcie/aer.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 65dff5f3457a..bbfb07842d89 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -1361,23 +1361,14 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
 	u32 reg32;
 	int rc;
 
-
-	/* Disable Root's interrupt in response to error messages */
-	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	if (pcie_aer_is_native(dev)) {
+		/* Clear Root Error Status */
+		pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
+		pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
+	}
 
 	rc = pci_bus_error_reset(dev);
-	pci_info(dev, "Root Port link has been reset\n");
-
-	/* Clear Root Error Status */
-	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
-	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
-
-	/* Enable Root Port's interrupt in response to error messages */
-	pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
-	reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
-	pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
+	pci_info(dev, "Root Port link has been reset (%d)\n", rc);
 
 	return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
 }
-- 
2.29.2

Re: [PATCH] AER: aer_root_reset() non-native handling

[Ethan Zhao]

On Sat, Oct 31, 2020 at 6:36 AM Sean V Kelley <sean.v.kelley@intel.com> wrote:
>
> If an OS has not been granted AER control via _OSC, then
> the OS should not make changes to PCI_ERR_ROOT_COMMAND and
> PCI_ERR_ROOT_STATUS related registers. Per section 4.5.1 of
> the System Firmware Intermediary (SFI) _OSC and DPC Updates
> ECN [1], this bit also covers these aspects of the PCI
> Express Advanced Error Reporting. Further, the handling of
> clear/enable of PCI_ERROR_ROOT_COMMAND when wrapped around
> PCI_ERR_ROOT_STATUS should have no effect and be removed.
> Based on the above and earlier discussion [2], make the
> following changes:
>
> Add a check for the native case (i.e., AER control via _OSC)
> Re-order and remove some of the handling:
> - clear PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
> - do reset
> - clear PCI_ERR_ROOT_STATUS
> - enable PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
>
> to this:
>
> - clear PCI_ERR_ROOT_STATUS
> - do reset
>
> The current "clear, reset, enable" order suggests that the reset
> might cause errors that we should ignore. But I am unable to find a
> reference and the clearing of PCI_ERR_ROOT_STATUS does not require them.
>
> [1] System Firmware Intermediary (SFI) _OSC and DPC Updates ECN, Feb 24,
>     2020, affecting PCI Firmware Specification, Rev. 3.2
>     https://members.pcisig.com/wg/PCI-SIG/document/14076
> [2] https://lore.kernel.org/linux-pci/20201020162820.GA370938@bjorn-Precision-5520/
>
> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
> ---
>  drivers/pci/pcie/aer.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 65dff5f3457a..bbfb07842d89 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1361,23 +1361,14 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev)
>         u32 reg32;
>         int rc;
>
> -
> -       /* Disable Root's interrupt in response to error messages */
> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
There may be some reasons to disable interrupt first and then do resetting,
clear status and re-enable interrupt.
Perhaps to avoid error noise,  what would happen if the resetting
causes errors itself ?

Thanks,
Ethan

> +       if (pcie_aer_is_native(dev)) {
> +               /* Clear Root Error Status */
> +               pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> +               pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
> +       }
>
>         rc = pci_bus_error_reset(dev);
> -       pci_info(dev, "Root Port link has been reset\n");
> -
> -       /* Clear Root Error Status */
> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, &reg32);
> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, reg32);
> -
> -       /* Enable Root Port's interrupt in response to error messages */
> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, &reg32);
> -       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, reg32);
> +       pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>
>         return rc ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>  }
> --
> 2.29.2
>

Re: [PATCH] AER: aer_root_reset() non-native handling

[Sean V Kelley]

On 1 Nov 2020, at 22:27, Ethan Zhao wrote:

> On Sat, Oct 31, 2020 at 6:36 AM Sean V Kelley 
> <sean.v.kelley@intel.com> wrote:
>>
>> If an OS has not been granted AER control via _OSC, then
>> the OS should not make changes to PCI_ERR_ROOT_COMMAND and
>> PCI_ERR_ROOT_STATUS related registers. Per section 4.5.1 of
>> the System Firmware Intermediary (SFI) _OSC and DPC Updates
>> ECN [1], this bit also covers these aspects of the PCI
>> Express Advanced Error Reporting. Further, the handling of
>> clear/enable of PCI_ERROR_ROOT_COMMAND when wrapped around
>> PCI_ERR_ROOT_STATUS should have no effect and be removed.
>> Based on the above and earlier discussion [2], make the
>> following changes:
>>
>> Add a check for the native case (i.e., AER control via _OSC)
>> Re-order and remove some of the handling:
>> - clear PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
>> - do reset
>> - clear PCI_ERR_ROOT_STATUS
>> - enable PCI_ERR_ROOT_COMMAND ROOT_PORT_INTR_ON_MESG_MASK
>>
>> to this:
>>
>> - clear PCI_ERR_ROOT_STATUS
>> - do reset
>>
>> The current "clear, reset, enable" order suggests that the reset
>> might cause errors that we should ignore. But I am unable to find a
>> reference and the clearing of PCI_ERR_ROOT_STATUS does not require 
>> them.
>>
>> [1] System Firmware Intermediary (SFI) _OSC and DPC Updates ECN, Feb 
>> 24,
>>     2020, affecting PCI Firmware Specification, Rev. 3.2
>>     https://members.pcisig.com/wg/PCI-SIG/document/14076
>> [2] 
>> https://lore.kernel.org/linux-pci/20201020162820.GA370938@bjorn-Precision-5520/
>>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com>
>> ---
>>  drivers/pci/pcie/aer.c | 21 ++++++---------------
>>  1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index 65dff5f3457a..bbfb07842d89 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -1361,23 +1361,14 @@ static pci_ers_result_t aer_root_reset(struct 
>> pci_dev *dev)
>>         u32 reg32;
>>         int rc;
>>
>> -
>> -       /* Disable Root's interrupt in response to error messages */
>> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, 
>> &reg32);
>> -       reg32 &= ~ROOT_PORT_INTR_ON_MESG_MASK;
>> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, 
>> reg32);
> There may be some reasons to disable interrupt first and then do 
> resetting,
> clear status and re-enable interrupt.
> Perhaps to avoid error noise,  what would happen if the resetting
> causes errors itself ?

I don’t have the historical context, but in looking at pci_bus_reset() 
it’s clear that it will probe slots for reset capability and then fall 
back to secondary bus reset.  It’s possible that an attempt to reset a 
slot could result in an error on some hardware.  This would potentially 
result in repeated error reporting. Not having the historical context, 
it’s best to leave in the clear/re-enabling of the interrupt when 
performing the reset.

Sean


>
> Thanks,
> Ethan
>
>> +       if (pcie_aer_is_native(dev)) {
>> +               /* Clear Root Error Status */
>> +               pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, 
>> &reg32);
>> +               pci_write_config_dword(dev, aer + 
>> PCI_ERR_ROOT_STATUS, reg32);
>> +       }
>>
>>         rc = pci_bus_error_reset(dev);
>> -       pci_info(dev, "Root Port link has been reset\n");
>> -
>> -       /* Clear Root Error Status */
>> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, 
>> &reg32);
>> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_STATUS, 
>> reg32);
>> -
>> -       /* Enable Root Port's interrupt in response to error messages 
>> */
>> -       pci_read_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, 
>> &reg32);
>> -       reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
>> -       pci_write_config_dword(dev, aer + PCI_ERR_ROOT_COMMAND, 
>> reg32);
>> +       pci_info(dev, "Root Port link has been reset (%d)\n", rc);
>>
>>         return rc ? PCI_ERS_RESULT_DISCONNECT : 
>> PCI_ERS_RESULT_RECOVERED;
>>  }
>> --
>> 2.29.2
>>