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:

quoted

 {
-	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!