From: Russell King - ARM Linux admin <linux@armlinux.org.uk> To: Bjorn Helgaas <helgaas@kernel.org> Cc: Heiner Kallweit <hkallweit1@gmail.com>, Bjorn Helgaas <bhelgaas@google.com>, Realtek linux nic maintainers <nic_swsd@realtek.com>, David Miller <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org> Subject: Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk Date: Wed, 6 Jan 2021 00:50:53 +0000 [thread overview] Message-ID: <20210106005053.GG1551@shell.armlinux.org.uk> (raw) In-Reply-To: <20210106002833.GA1286114@bjorn-Precision-5520> 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!
WARNING: multiple messages have this Message-ID (diff)
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> To: Bjorn Helgaas <helgaas@kernel.org> Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, Bjorn Helgaas <bhelgaas@google.com>, Jakub Kicinski <kuba@kernel.org>, David Miller <davem@davemloft.net>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Heiner Kallweit <hkallweit1@gmail.com> Subject: Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk Date: Wed, 6 Jan 2021 00:50:53 +0000 [thread overview] Message-ID: <20210106005053.GG1551@shell.armlinux.org.uk> (raw) In-Reply-To: <20210106002833.GA1286114@bjorn-Precision-5520> 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! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-01-06 0:52 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-05 9:40 [PATCH 0/3] PCI: Disable parity checking if broken_parity is set Heiner Kallweit 2021-01-05 9:40 ` Heiner Kallweit 2021-01-05 9:41 ` [PATCH 1/3] PCI: Disable parity checking if broken_parity_status " Heiner Kallweit 2021-01-05 9:41 ` Heiner Kallweit 2021-01-05 10:00 ` Leon Romanovsky 2021-01-05 10:00 ` Leon Romanovsky 2021-01-05 9:42 ` [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk Heiner Kallweit 2021-01-05 9:42 ` Heiner Kallweit 2021-01-06 0:28 ` Bjorn Helgaas 2021-01-06 0:28 ` Bjorn Helgaas 2021-01-06 0:44 ` Heiner Kallweit 2021-01-06 0:44 ` Heiner Kallweit 2021-01-06 0:52 ` Russell King - ARM Linux admin 2021-01-06 0:52 ` Russell King - ARM Linux admin 2021-01-06 0:57 ` Heiner Kallweit 2021-01-06 0:57 ` Heiner Kallweit 2021-01-06 0:50 ` Russell King - ARM Linux admin [this message] 2021-01-06 0:50 ` Russell King - ARM Linux admin 2021-01-05 9:44 ` [PATCH 3/3] r8169: simplify broken parity handling now that PCI core takes care Heiner Kallweit 2021-01-05 9:44 ` Heiner Kallweit
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210106005053.GG1551@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=bhelgaas@google.com \ --cc=davem@davemloft.net \ --cc=helgaas@kernel.org \ --cc=hkallweit1@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nic_swsd@realtek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.