[PATCH 0/3] PCI: Disable parity checking if broken_parity is set

[PATCH 0/3] PCI: Disable parity checking if broken_parity is set

[Heiner Kallweit]

If we know that a device has broken parity checking, then disable it.
This avoids quirks like in r8169 where on the first parity error
interrupt parity checking will be disabled if broken_parity_status
is set. Make pci_quirk_broken_parity() public so that it can be used
by platform code, e.g. for Thecus N2100.

Heiner Kallweit (3):
  PCI: Disable parity checking if broken_parity_status is set
  ARM: iop32x: improve N2100 PCI broken parity quirk
  r8169: simplify broken parity handling now that PCI core takes care

 arch/arm/mach-iop32x/n2100.c              |  8 +++-----
 drivers/net/ethernet/realtek/r8169_main.c | 14 --------------
 drivers/pci/quirks.c                      | 17 +++++++++++------
 include/linux/pci.h                       |  2 ++
 4 files changed, 16 insertions(+), 25 deletions(-)

-- 
2.30.0

[PATCH 1/3] PCI: Disable parity checking if broken_parity_status is set

[Heiner Kallweit]

If we know that a device has broken parity checking, then disable it.
This avoids quirks like in r8169 where on the first parity error
interrupt parity checking will be disabled if broken_parity_status
is set. Make pci_quirk_broken_parity() public so that it can be used
by platform code, e.g. for Thecus N2100.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/quirks.c | 17 +++++++++++------
 include/linux/pci.h  |  2 ++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3b..ab54e26b8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -205,17 +205,22 @@ static void quirk_mmio_always_on(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 				PCI_CLASS_BRIDGE_HOST, 8, quirk_mmio_always_on);
 
+void pci_quirk_broken_parity(struct pci_dev *dev)
+{
+	u16 cmd;
+
+	dev->broken_parity_status = 1;	/* This device gives false positives */
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	pci_write_config_word(dev, PCI_COMMAND, cmd & ~PCI_COMMAND_PARITY);
+}
+
 /*
  * 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.
  */
-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_quirk_broken_parity);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_DEVICE_ID_MELLANOX_TAVOR_BRIDGE, pci_quirk_broken_parity);
 
 /*
  * Deal with broken BIOSes that neglect to enable passive release,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b32126d26..161dcc474 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1916,6 +1916,8 @@ enum pci_fixup_pass {
 	pci_fixup_suspend_late,	/* pci_device_suspend_late() */
 };
 
+void pci_quirk_broken_parity(struct pci_dev *dev);
+
 #ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS
 #define __DECLARE_PCI_FIXUP_SECTION(sec, name, vendor, device, class,	\
 				    class_shift, hook)			\
-- 
2.30.0

Re: [PATCH 1/3] PCI: Disable parity checking if broken_parity_status is set

[Leon Romanovsky]

On Tue, Jan 05, 2021 at 10:41:26AM +0100, Heiner Kallweit wrote:
> If we know that a device has broken parity checking, then disable it.
> This avoids quirks like in r8169 where on the first parity error
> interrupt parity checking will be disabled if broken_parity_status
> is set. Make pci_quirk_broken_parity() public so that it can be used
> by platform code, e.g. for Thecus N2100.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/quirks.c | 17 +++++++++++------
>  include/linux/pci.h  |  2 ++
>  2 files changed, 13 insertions(+), 6 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

[PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Heiner Kallweit]

Simplify the quirk by using new PCI core function
pci_quirk_broken_parity(). In addition make the quirk
more specific, use device id 0x8169 instead of PCI_ANY_ID.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 arch/arm/mach-iop32x/n2100.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
index 78b9a5ee4..24c3eec46 100644
--- a/arch/arm/mach-iop32x/n2100.c
+++ b/arch/arm/mach-iop32x/n2100.c
@@ -122,12 +122,10 @@ static struct hw_pci n2100_pci __initdata = {
  */
 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;
+	if (machine_is_n2100())
+		pci_quirk_broken_parity(dev);
 }
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, PCI_ANY_ID, n2100_fixup_r8169);
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169, n2100_fixup_r8169);
 
 static int __init n2100_pci_init(void)
 {
-- 
2.30.0

Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Bjorn Helgaas]

On Tue, Jan 05, 2021 at 10:42:31AM +0100, Heiner Kallweit wrote:
> Simplify the quirk by using new PCI core function
> pci_quirk_broken_parity(). In addition make the quirk
> more specific, use device id 0x8169 instead of PCI_ANY_ID.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  arch/arm/mach-iop32x/n2100.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
> index 78b9a5ee4..24c3eec46 100644
> --- a/arch/arm/mach-iop32x/n2100.c
> +++ b/arch/arm/mach-iop32x/n2100.c
> @@ -122,12 +122,10 @@ static struct hw_pci n2100_pci __initdata = {
>   */
>  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;
> +	if (machine_is_n2100())
> +		pci_quirk_broken_parity(dev);

Whatever "machine_is_n2100()" is (I can't find the definition), it is
surely not equivalent to "00:01.0 || 00:02.0".  That change probably
should be a separate patch with some explanation.

If this makes the quirk safe to use in a generic kernel, that sounds
like a good thing.

I guess a parity problem could be the result of a defect in either the
device (e.g., 0x8169), which would be an issue in *all* platforms, or
a platform-specific issue in the way it's wired up.  I assume it's the
latter because the quirk is not in drivers/pci/quirks.c.

Why is it safe to restrict this to device ID 0x8169?  If this is
platform issue, it might affect any device in the slot.

>  }
> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, PCI_ANY_ID, n2100_fixup_r8169);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169, n2100_fixup_r8169);
>  
>  static int __init n2100_pci_init(void)
>  {
> -- 
> 2.30.0
> 
> 

Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Heiner Kallweit]

On 06.01.2021 01:28, Bjorn Helgaas wrote:
> On Tue, Jan 05, 2021 at 10:42:31AM +0100, Heiner Kallweit wrote:
>> Simplify the quirk by using new PCI core function
>> pci_quirk_broken_parity(). In addition make the quirk
>> more specific, use device id 0x8169 instead of PCI_ANY_ID.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  arch/arm/mach-iop32x/n2100.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
>> index 78b9a5ee4..24c3eec46 100644
>> --- a/arch/arm/mach-iop32x/n2100.c
>> +++ b/arch/arm/mach-iop32x/n2100.c
>> @@ -122,12 +122,10 @@ static struct hw_pci n2100_pci __initdata = {
>>   */
>>  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;
>> +	if (machine_is_n2100())
>> +		pci_quirk_broken_parity(dev);
> 
> Whatever "machine_is_n2100()" is (I can't find the definition), it is
> surely not equivalent to "00:01.0 || 00:02.0".  That change probably
> should be a separate patch with some explanation.
> 
The machine_is_xxx() checks are dynamically created,
see arch/arm/tools/gen-mach-types.
Slots 1 and 2 are the two network cards, both are Realtek RTL8169.
The quirk (after this patch) applies for Realtek RTL8169 devices only,
therefore we don't need the slot checks any longer.
Actually the slot checks haven't been needed even before, because
only in slots 1 and 2 are Realtek devices.

The machine type check is there to protect from (theoretical) cases
where the n2100 code (incl. the RTL8169 quirk) may be compiled in,
but the kernel is used on another machine.

> If this makes the quirk safe to use in a generic kernel, that sounds
> like a good thing.
> 
> I guess a parity problem could be the result of a defect in either the
> device (e.g., 0x8169), which would be an issue in *all* platforms, or
> a platform-specific issue in the way it's wired up.  I assume it's the
> latter because the quirk is not in drivers/pci/quirks.c.
> 
I haven't seen any other report about RTL8169 parity problems.
Therefore I also think it's platform-specific.

> Why is it safe to restrict this to device ID 0x8169?  If this is
> platform issue, it might affect any device in the slot.
> 
So far the quirk was applied for all Realtek devices.
The parity problem is limited to the two RTL8169 network cards, and there
are no other Realtek PCI devices in the system. Supposedly PCI_ANY_ID
was just used because it was less work than looking for the device id.
Functionally it's the same on this system.

>>  }
>> -DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, PCI_ANY_ID, n2100_fixup_r8169);
>> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REALTEK, 0x8169, n2100_fixup_r8169);
>>  
>>  static int __init n2100_pci_init(void)
>>  {
>> -- 
>> 2.30.0
>>
>>

Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Russell King - ARM Linux admin]

On Wed, Jan 06, 2021 at 01:44:03AM +0100, Heiner Kallweit wrote:
> The machine type check is there to protect from (theoretical) cases
> where the n2100 code (incl. the RTL8169 quirk) may be compiled in,
> but the kernel is used on another machine.

That is far from a theoretical case. The ARM port has always supported
multiple machines in a single kernel. They just had to be "compatible"
in other words, the same SoC. All the platforms supported by
arch/arm/mach-iop32x can be built as a single kernel image and run on
any of those platforms.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Heiner Kallweit]

On 06.01.2021 01:52, Russell King - ARM Linux admin wrote:
> On Wed, Jan 06, 2021 at 01:44:03AM +0100, Heiner Kallweit wrote:
>> The machine type check is there to protect from (theoretical) cases
>> where the n2100 code (incl. the RTL8169 quirk) may be compiled in,
>> but the kernel is used on another machine.
> 
> That is far from a theoretical case. The ARM port has always supported
> multiple machines in a single kernel. They just had to be "compatible"
> in other words, the same SoC. All the platforms supported by
> arch/arm/mach-iop32x can be built as a single kernel image and run on
> any of those platforms.
> 
Good to know, then we indeed need the machine check. IOW, based on
what you state we could even now have the following situation:
N2100 support is compiled in, and the kernel is used on another machine
that by chance also has Realtek RTL8169 in PCI slots 1 or 2.
Then the PCI quirk would be applied, even though the machine doesn't
have the parity issue.

Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

[Russell King - ARM Linux admin]

On Tue, Jan 05, 2021 at 06:28:33PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 05, 2021 at 10:42:31AM +0100, Heiner Kallweit wrote:
> >  {
> > -	if (dev->bus->number == 0 &&
> > -	    (dev->devfn == PCI_DEVFN(1, 0) ||
> > -	     dev->devfn == PCI_DEVFN(2, 0)))
> > -		dev->broken_parity_status = 1;
> > +	if (machine_is_n2100())
> > +		pci_quirk_broken_parity(dev);
> 
> Whatever "machine_is_n2100()" is (I can't find the definition), it is
> surely not equivalent to "00:01.0 || 00:02.0".  That change probably
> should be a separate patch with some explanation.

It isn't equivalent. It says "if this machine is N2100" which is a
completely different thing from matching the bus/devfn numbers.

You won't find a definition for machine_is_n2100() in the kernel;
it is generated from the machine table by scripts, along with lots
of similar definitions for each machine type:

/* The type of machine we're running on */
extern unsigned int __machine_arch_type;

#ifdef CONFIG_MACH_N2100
# ifdef machine_arch_type
#  undef machine_arch_type
#  define machine_arch_type     __machine_arch_type
# else
#  define machine_arch_type     MACH_TYPE_N2100
# endif
# define machine_is_n2100()     (machine_arch_type == MACH_TYPE_N2100)
#else
# define machine_is_n2100()     (0)
#endif

The upshot of the above is that machine_is_n2100() is constant zero
if the platform is not configured (thereby allowing the compiler to
eliminate the code.) If it is the _only_ platform selected, then it
evaluates to an always-true expression. Otherwise, it becomes a
run-time evaluated conditional.

We may have better ways to do this in modern kernels, but this was
invented decades ago, and works with zero runtime overhead.

> If this makes the quirk safe to use in a generic kernel, that sounds
> like a good thing.
> 
> I guess a parity problem could be the result of a defect in either the
> device (e.g., 0x8169), which would be an issue in *all* platforms, or
> a platform-specific issue in the way it's wired up.  I assume it's the
> latter because the quirk is not in drivers/pci/quirks.c.
> 
> Why is it safe to restrict this to device ID 0x8169?  If this is
> platform issue, it might affect any device in the slot.

You assume the platform has multiple PCI slots - it doesn't. It's an
embedded platform (it's sold as a NAS) and it has a single mini-PCI
slot for a WiFi card. Without that populated, lspci -n looks like
this:

00:01.0 0200: 10ec:8169 (rev 10)
00:02.0 0200: 10ec:8169 (rev 10)
00:03.0 0180: 1095:3512 (rev 01)
00:04.0 0c03: 1106:3038 (rev 61)
00:04.1 0c03: 1106:3038 (rev 61)
00:04.2 0c03: 1106:3104 (rev 63)

Where all those devices are soldered to the board.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 3/3] r8169: simplify broken parity handling now that PCI core takes care

[Heiner Kallweit]

Meanwhile the PCI core disables parity checking for a device that has
broken_parity_status set. Therefore we don't need the quirk any longer
to disable parity checking on the first parity error interrupt.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a8284feb4..1e26b47f5 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4329,20 +4329,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.30.0