| Message ID | 20201030223443.165783-1-sean.v.kelley@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | AER: aer_root_reset() non-native handling | expand |
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 ? 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 >
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 >>
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); + 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; }
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(-)