Re: [PATCH v9 5/8] PCI/AER: Allow clearing Error Status Register in FF mode

[Kuppuswamy Sathyanarayanan]

On 10/28/19 4:22 PM, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 04:39:01PM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> >
> > As per PCI firmware specification r3.2 Downstream Port Containment
> > Related Enhancements ECN, sec 4.5.1, table 4-6,
> That table adds bit 7, "DPC config control" to the _OSC control field
> and talks about modifying registers in the DPC capability.
>
> It doesn't say anything about firmware-first or about the OS modifying
> registers in the AER capability.  But this patch doesn't do anything
> related to DPC or _OSC.

Expectation of clearing AER registers is not explicitly mentioned in 
above DPC ECN. But it
has been clarified in the following update to this ECN.

Please check the implementation note in page 10 of
https://members.pcisig.com/wg/PCI-SIG/document/previewpdf/13563.

It specifies that in EDR mode, firmware expects OS to clear error 
registers (Check for
following string "Bring Portout of DPC, clear port error status, assign 
bus numbers to child
devices").

I will include this ECN reference in follow up update.

> > Error Disconnect
> > Recover (EDR) support allows OS to handle error recovery and
> > clearing Error Registers even in FF mode. So remove FF mode checks in
> > pci_cleanup_aer_uncorrect_error_status(), pci_aer_clear_fatal_status()
> > and pci_cleanup_aer_error_status_regs() functions.
> >
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > Acked-by: Keith Busch <keith.busch@xxxxxxxxx>
> > ---
> >   drivers/pci/pcie/aer.c | 12 ++----------
> >   1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index b45bc47d04fe..e1509bb900c5 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -383,9 +383,6 @@ int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
> >   	if (!pos)
> >   		return -EIO;
> >   
> > -	if (pcie_aer_get_firmware_first(dev))
> > -		return -EIO;
> > -
> >   	/* Clear status bits for ERR_NONFATAL errors only */
> >   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
> >   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> > @@ -406,9 +403,6 @@ void pci_aer_clear_fatal_status(struct pci_dev *dev)
> >   	if (!pos)
> >   		return;
> >   
> > -	if (pcie_aer_get_firmware_first(dev))
> > -		return;
> > -
> >   	/* Clear status bits for ERR_FATAL errors only */
> >   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
> >   	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &sev);
> > @@ -430,9 +424,6 @@ int pci_cleanup_aer_error_status_regs(struct pci_dev *dev)
> >   	if (!pos)
> >   		return -EIO;
> >   
> > -	if (pcie_aer_get_firmware_first(dev))
> > -		return -EIO;
> > -
> >   	port_type = pci_pcie_type(dev);
> >   	if (port_type == PCI_EXP_TYPE_ROOT_PORT) {
> >   		pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, &status);
> > @@ -455,7 +446,8 @@ void pci_aer_init(struct pci_dev *dev)
> >   	if (dev->aer_cap)
> >   		dev->aer_stats = kzalloc(sizeof(struct aer_stats), GFP_KERNEL);
> >   
> > -	pci_cleanup_aer_error_status_regs(dev);
> > +	if (!pcie_aer_get_firmware_first(dev))
> > +		pci_cleanup_aer_error_status_regs(dev);
> This effectively moves the "if (pcie_aer_get_firmware_first())" check
> from pci_cleanup_aer_error_status_regs() into one of the callers.  But
> there are two other callers: pci_aer_init() and pci_restore_state().
> Do they need the change, or do you want to cleanup the AER error
> registers there, but not here?
Good catch. I have added this check to pci_aer_init(). But it needs to 
be added to
pci_restore_state() as well. Instead of moving the checks to the caller, 
If you agree,
I could change the API to pci_cleanup_aer_error_status_regs(struct 
pci_dev *dev, bool skip_ff_check) and
let the caller decide whether they want skip the check or not.
> >   }
> >   
> >   void pci_aer_exit(struct pci_dev *dev)
> > --
> > 2.21.0
--
Sathyanarayanan Kuppuswamy
Linux kernel developer