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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux