[PATCH v4 0/3] PCI: Disable parity checking

[PATCH v4 0/3] PCI: Disable parity checking

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

I think this is essentially the same as Heiner's v3 posting, with these
changes:

  - Added a pci_disable_parity() interface in pci.c instead of making a
    public pci_quirk_broken_parity() because quirks.c is only compiled when
    CONFIG_PCI_QUIRKS=y.

  - Removed the setting of dev->broken_parity_status because it's really
    only used by EDAC error reporting, and if we disable parity error
    reporting, we shouldn't get there.  This change will be visible in the
    sysfs "broken_parity_status" file, but I doubt that's important.

I dropped Leon's reviewed-by because I fiddled with the code.  Similarly I
haven't added your signed-off-by, Heiner, because I don't want you blamed
for my errors.  But if this looks OK to you I'll add it.

v1: https://lore.kernel.org/r/a6f09e1b-4076-59d1-a4e3-05c5955bfff2@gmail.com
v2: https://lore.kernel.org/r/bbc33d9b-af7c-8910-cdb3-fa3e3b2e3266@gmail.com
- reduce scope of N2100 change to using the new PCI core quirk
v3: https://lore.kernel.org/r/992c800e-2e12-16b0-4845-6311b295d932@gmail.com/
- improve commit message of patch 2

v4:
- add pci_disable_parity() (not conditional on CONFIG_PCI_QUIRKS)
- remove setting of dev->broken_parity_status


Bjorn Helgaas (1):
  PCI: Add pci_disable_parity()

Heiner Kallweit (2):
  IB/mthca: Disable parity reporting
  ARM: iop32x: disable N2100 PCI parity reporting

 arch/arm/mach-iop32x/n2100.c              |  8 ++++----
 drivers/net/ethernet/realtek/r8169_main.c | 14 --------------
 drivers/pci/pci.c                         | 17 +++++++++++++++++
 drivers/pci/quirks.c                      | 13 ++++---------
 include/linux/pci.h                       |  1 +
 5 files changed, 26 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] PCI: Add pci_disable_parity()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add pci_disable_parity() to disable reporting of parity errors for a
device by clearing PCI_COMMAND_PARITY.

The device will still set PCI_STATUS_DETECTED_PARITY when it detects
a parity error or receives a Poisoned TLP, but it will not set
PCI_STATUS_PARITY, which means it will not assert PERR#
(conventional PCI) or report Poisoned TLPs (PCIe).

Based-on: https://lore.kernel.org/linux-arm-kernel/d375987c-ea4f-dd98-4ef8-99b2fbfe7c33@gmail.com/
Based-on-patch-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   | 17 +++++++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 16a17215f633..b1845e5e5c8f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4453,6 +4453,23 @@ void pci_clear_mwi(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pci_clear_mwi);
 
+/**
+ * pci_disable_parity - disable parity checking for device
+ * @dev: the PCI device to operate on
+ *
+ * Disable parity checking for device @dev
+ */
+void pci_disable_parity(struct pci_dev *dev)
+{
+	u16 cmd;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	if (cmd & PCI_COMMAND_PARITY) {
+		cmd &= ~PCI_COMMAND_PARITY;
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+	}
+}
+
 /**
  * pci_intx - enables/disables PCI INTx for device dev
  * @pdev: the PCI device to operate on
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..4eaa773115da 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1201,6 +1201,7 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int __must_check pcim_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
+void pci_disable_parity(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
 bool pci_check_and_mask_intx(struct pci_dev *dev);
 bool pci_check_and_unmask_intx(struct pci_dev *dev);
-- 
2.25.1

[PATCH v4 2/3] IB/mthca: Disable parity reporting

[Bjorn Helgaas]

From: Heiner Kallweit <hkallweit1@gmail.com>

For Mellanox Tavor devices, we previously set dev->broken_parity_status,
which does not change the device's behavior; it merely prevents the EDAC
PCI error reporting from warning about Master Data Parity Error, Signaled
System Error, or Detected Parity Error for this device.

Instead, disable Parity Error Response so the device doesn't report
parity errors in the first place.

[bhelgaas: split out pci_disable_parity(), commit log, keep quirk static]
Link: https://lore.kernel.org/r/d375987c-ea4f-dd98-4ef8-99b2fbfe7c33@gmail.com
---
 drivers/pci/quirks.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..6aa9df411604 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -206,16 +206,11 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 				PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on);
 
 /*
- * The Mellanox Tavor device gives false positive parity errors.  Mark this
- * device with a broken_parity_status to allow PCI scanning code to "skip"
- * this now blacklisted device.
+ * The Mellanox Tavor device gives false positive parity errors.  Disable
+ * parity error reporting.
  */
-static void quirk_mellanox_tavor(struct pci_dev *dev)
-{
-	dev->broken_parity_status = 1;	/* This device gives false positives */
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_TAVOR, quirk_mellanox_tavor);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_TAVOR_BRIDGE, quirk_mellanox_tavor);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_TAVOR, pci_disable_parity);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_TAVOR_BRIDGE, pci_disable_parity);
 
 /*
  * Deal with broken BIOSes that neglect to enable passive release,
-- 
2.25.1

[PATCH v4 3/3] ARM: iop32x: disable N2100 PCI parity reporting

[Bjorn Helgaas]

From: Heiner Kallweit <hkallweit1@gmail.com>

On the N2100, instead of just marking the r8169 chips as having
broken_parity_status, disable parity error reporting for them entirely.

This was the only relevant place that set broken_parity_status, so we no
longer need to check for it in the r8169 error interrupt handler.

[bhelgaas: squash into one patch, commit log]
Link: https://lore.kernel.org/r/0c0dcbf2-5f1e-954c-ebd7-e6ccfae5c60e@gmail.com
Link: https://lore.kernel.org/r/9e312679-a684-e9c7-2656-420723706451@gmail.com
---
 arch/arm/mach-iop32x/n2100.c              |  8 ++++----
 drivers/net/ethernet/realtek/r8169_main.c | 14 --------------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
index 78b9a5ee41c9..bf99e718f8b8 100644
--- a/arch/arm/mach-iop32x/n2100.c
+++ b/arch/arm/mach-iop32x/n2100.c
@@ -116,16 +116,16 @@ static struct hw_pci n2100_pci __initdata = {
 };
 
 /*
- * Both r8169 chips on the n2100 exhibit PCI parity problems.  Set
- * the ->broken_parity_status flag for both ports so that the r8169
- * driver knows it should ignore error interrupts.
+ * Both r8169 chips on the n2100 exhibit PCI parity problems.  Turn
+ * off parity reporting for both ports so we don't get error interrupts
+ * for them.
  */
 static void n2100_fixup_r8169(struct pci_dev *dev)
 {
 	if (dev->bus->number == 0 &&
 	    (dev->devfn == PCI_DEVFN(1, 0) ||
 	     dev->devfn == PCI_DEVFN(2, 0)))
-		dev->broken_parity_status = 1;
+		pci_disable_parity(dev);
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, PCI_ANY_ID, n2100_fixup_r8169);
 
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index f704da3f214c..a6aff0d993eb 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4358,20 +4358,6 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
 	if (net_ratelimit())
 		netdev_err(dev, "PCI error (cmd = 0x%04x, status_errs = 0x%04x)\n",
 			   pci_cmd, pci_status_errs);
-	/*
-	 * The recovery sequence below admits a very elaborated explanation:
-	 * - it seems to work;
-	 * - I did not see what else could be done;
-	 * - it makes iop3xx happy.
-	 *
-	 * Feel free to adjust to your needs.
-	 */
-	if (pdev->broken_parity_status)
-		pci_cmd &= ~PCI_COMMAND_PARITY;
-	else
-		pci_cmd |= PCI_COMMAND_SERR | PCI_COMMAND_PARITY;
-
-	pci_write_config_word(pdev, PCI_COMMAND, pci_cmd);
 
 	rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
 }
-- 
2.25.1

Re: [PATCH v4 0/3] PCI: Disable parity checking

[Heiner Kallweit]

On 30.03.2021 19:43, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> I think this is essentially the same as Heiner's v3 posting, with these
> changes:
> 
>   - Added a pci_disable_parity() interface in pci.c instead of making a
>     public pci_quirk_broken_parity() because quirks.c is only compiled when
>     CONFIG_PCI_QUIRKS=y.
> 
>   - Removed the setting of dev->broken_parity_status because it's really
>     only used by EDAC error reporting, and if we disable parity error
>     reporting, we shouldn't get there.  This change will be visible in the
>     sysfs "broken_parity_status" file, but I doubt that's important.
> 
> I dropped Leon's reviewed-by because I fiddled with the code.  Similarly I
> haven't added your signed-off-by, Heiner, because I don't want you blamed
> for my errors.  But if this looks OK to you I'll add it.
> 
> v1: https://lore.kernel.org/r/a6f09e1b-4076-59d1-a4e3-05c5955bfff2@gmail.com
> v2: https://lore.kernel.org/r/bbc33d9b-af7c-8910-cdb3-fa3e3b2e3266@gmail.com
> - reduce scope of N2100 change to using the new PCI core quirk
> v3: https://lore.kernel.org/r/992c800e-2e12-16b0-4845-6311b295d932@gmail.com/
> - improve commit message of patch 2
> 
> v4:
> - add pci_disable_parity() (not conditional on CONFIG_PCI_QUIRKS)
> - remove setting of dev->broken_parity_status
> 
> 
> Bjorn Helgaas (1):
>   PCI: Add pci_disable_parity()
> 
> Heiner Kallweit (2):
>   IB/mthca: Disable parity reporting
>   ARM: iop32x: disable N2100 PCI parity reporting
> 
>  arch/arm/mach-iop32x/n2100.c              |  8 ++++----
>  drivers/net/ethernet/realtek/r8169_main.c | 14 --------------
>  drivers/pci/pci.c                         | 17 +++++++++++++++++
>  drivers/pci/quirks.c                      | 13 ++++---------
>  include/linux/pci.h                       |  1 +
>  5 files changed, 26 insertions(+), 27 deletions(-)
> 

LGTM. Thanks!

Re: [PATCH v4 0/3] PCI: Disable parity checking

[Krzysztof Wilczyński]

Hi Bjorn,

> I think this is essentially the same as Heiner's v3 posting, with these
> changes:
> 
>   - Added a pci_disable_parity() interface in pci.c instead of making a
>     public pci_quirk_broken_parity() because quirks.c is only compiled when
>     CONFIG_PCI_QUIRKS=y.

Very nice idea to add pci_disable_parity(), looks very clean.

>   - Removed the setting of dev->broken_parity_status because it's really
>     only used by EDAC error reporting, and if we disable parity error
>     reporting, we shouldn't get there.  This change will be visible in the
>     sysfs "broken_parity_status" file, but I doubt that's important.
> 
> I dropped Leon's reviewed-by because I fiddled with the code.  Similarly I
> haven't added your signed-off-by, Heiner, because I don't want you blamed
> for my errors.  But if this looks OK to you I'll add it.
[...]


Thank you Bjorn and Heiner!

Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

Krzysztof