Messages in this thread | | | From | "Sean V Kelley" <> | Subject | Re: [PATCH] AER: aer_root_reset() non-native handling | Date | Tue, 03 Nov 2020 15:08:48 -0800 |
| |
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, >> ®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 >>
| |