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@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, 
> > &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