ixgbe: Implement PCIe AER support

From: PJ Waskiewicz <[email protected]>

This patch implements the PCIe Advanced Error Reporting callbacks in ixgbe. The 82598 hardware supports AER, so we enable it.

Signed-off-by Peter P Waskiewicz Jr <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

---

pci_enable_pcie_error_reporting should be checking for pci_read/pci_write config errors and does not.

Your driver should be checking for pci_enable_pcie_error_reporting failing.

Quoted 8 lines · On Mon, 01 Dec 2008 16:41:15 -0800 Jeff Kirsher <[email protected]>…

From: PJ Waskiewicz <[email protected]>

This patch implements the PCIe Advanced Error Reporting callbacks in ixgbe. The 82598 hardware supports AER, so we enable it.

Signed-off-by Peter P Waskiewicz Jr <[email protected]>

Signed-off-by Jeff Kirsher <[email protected]>

---

Once again: pcie-aer is not checking for mmconfig working! If you turn it on, you may get screaming errors.

pci_enable_pcie_error_reporting should be checking for pci_read/pci_write config errors and does not.

Your driver should be checking for pci_enable_pcie_error_reporting failing.

This patch does more error checking in the Advanced Error Reporting code. Since AER needs to access PCI registers > 255, it won't work without MMCONFIG and other quirks may stop it as well. The code must check this by looking at return values from pci_read/write_config_XXX calls.

I don't have any hardware that uses AER routines but discovered this in earlier versions of the sky2 driver that tried to use
pci AER routines. Ended up just giving up and using other ways to access PCI config space on sky2 since there were too many platform glitches.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/pci/pcie/aer/aerdrv_core.c +28 -17

@@ -31,80 +31,92 @@ module_param(forceload, bool, 0);
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
 	u16 reg16 = 0;
-	int pos;
+	int pos, err;
+	u32 status;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos)
 		return -EIO;
 
+	err = pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
+	if (err)
+		return err;
+
 	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
 	if (!pos)
 		return -EIO;
 
-	pci_read_config_word(dev, pos+PCI_EXP_DEVCTL, &reg16);
+	err = pci_read_config_word(dev, pos+PCI_EXP_DEVCTL, &reg16);
+	if (err)
+		return err;
+
 	reg16 = reg16 |
 		PCI_EXP_DEVCTL_CERE |
 		PCI_EXP_DEVCTL_NFERE |
 		PCI_EXP_DEVCTL_FERE |
 		PCI_EXP_DEVCTL_URRE;
-	pci_write_config_word(dev, pos+PCI_EXP_DEVCTL,
-			reg16);
-	return 0;
+	return pci_write_config_word(dev, pos+PCI_EXP_DEVCTL, reg16);
 }
 
 int pci_disable_pcie_error_reporting(struct pci_dev *dev)
 {
 	u16 reg16 = 0;
-	int pos;
+	int pos, err;
 
 	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
 	if (!pos)
 		return -EIO;
 
-	pci_read_config_word(dev, pos+PCI_EXP_DEVCTL, &reg16);
+	err = pci_read_config_word(dev, pos+PCI_EXP_DEVCTL, &reg16);
+	if (err)
+		return err;
+
 	reg16 = reg16 & ~(PCI_EXP_DEVCTL_CERE |
 			PCI_EXP_DEVCTL_NFERE |
 			PCI_EXP_DEVCTL_FERE |
 			PCI_EXP_DEVCTL_URRE);
-	pci_write_config_word(dev, pos+PCI_EXP_DEVCTL,
-			reg16);
-	return 0;
+	return pci_write_config_word(dev, pos+PCI_EXP_DEVCTL, reg16);
 }
 
 int pci_cleanup_aer_uncorrect_error_status(struct pci_dev *dev)
 {
-	int pos;
+	int pos, err;
 	u32 status, mask;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos)
 		return -EIO;
 
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
-	pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
+	err = pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, &status);
+	if (err)
+		return err;
+
+	err = pci_read_config_dword(dev, pos + PCI_ERR_UNCOR_SEVER, &mask);
+	if (err)
+		return err;
+
 	if (dev->error_state == pci_channel_io_normal)
 		status &= ~mask; /* Clear corresponding nonfatal bits */
 	else
 		status &= mask; /* Clear corresponding fatal bits */
-	pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
-
-	return 0;
+	return pci_write_config_dword(dev, pos + PCI_ERR_UNCOR_STATUS, status);
 }
 
 #if 0
 int pci_cleanup_aer_correct_error_status(struct pci_dev *dev)
 {
-	int pos;
+	int pos, err;
 	u32 status;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
 	if (!pos)
 		return -EIO;
 

This patch does more error checking in the Advanced Error Reporting code. Since AER needs to access PCI registers > 255, it won't work without MMCONFIG and other quirks may stop it as well. The code must check this by looking at return values from pci_read/write_config_XXX calls.

I don't have any hardware that uses AER routines but discovered this in earlier versions of the sky2 driver that tried to use pci AER routines. Ended up just giving up and using other ways to access PCI
config space on sky2 since there were too many platform glitches.

Signed-off-by Stephen Hemminger <[email protected]>

drivers/pci/pcie/aer/aerdrv_core.c +2 -1

@@ -31,80 +31,92 @@ module_param(forceload, bool, 0);
 int pci_enable_pcie_error_reporting(struct pci_dev *dev)
 {
 	u16 reg16 = 0;
-	int pos;
+	int pos, err;
+	u32 status;
 

Quoted 10 lines · Stephen Hemminger

This patch does more error checking in the Advanced Error Reporting code. Since AER needs to access PCI registers > 255, it won't work without MMCONFIG and other quirks may stop it as well. The code must check this by looking at return values from pci_read/write_config_XXX calls.

I don't have any hardware that uses AER routines but discovered this in earlier versions of the sky2 driver that tried to use
pci AER routines. Ended up just giving up and using other ways to access PCI config space on sky2 since there were too many platform glitches.

When experimenting with sky2 driver, was pci_find_ext_capability() returning non-zero although further ext-space accesses were failing?

pci_find_ext_capability would succeed but all access to registers >= 256 would fail if MMCONFIG failed.

P.s: you can look back into LKML for Linus discussion about why MMCONFIG is broken anyway.

Quoted 2 lines · Stephen Hemminger

pci_find_ext_capability would succeed but all access to registers >= 256 would fail if MMCONFIG failed.

Your sentence seems self-contradicting to me. pci_find_ext_capability() (!= pci_find_capability()) only tries to access registers >= 256, so I don't see how it would succeed if *all* those accesses are failing.

Quoted 3 lines

P.s: you can look back into LKML for Linus discussion about why MMCONFIG is broken anyway.

I was an active participant in one of those discussions (although it is possible I missed a later one), so I am quite aware about the limitations of MMCONFIG. But pci_find_ext_capability() looked to me a good filter to check about those limitations().

This is not just about pciaer, for instance, it might be that a lot of the SR-IOV code submitted recently assumes that if the corresponding extended-capability can be detected, little checking needs to be done afterwards.

From: Stephen Hemminger <[email protected]>
Date: Mon, 1 Dec 2008 17:47:54 -0800

Quoted 4 lines

pci_enable_pcie_error_reporting should be checking for pci_read/pci_write config errors and does not.

Your driver should be checking for pci_enable_pcie_error_reporting failing.

Agreed.