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@xxxxxxxxx> 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@xxxxxxxxx>
---
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,
®32);
- 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,
®32);
+ 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,
®32);
- 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,
®32);
- 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
