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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Jan 2021 01:44:03 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/3] ARM: iop32x: improve N2100 PCI broken parity quirk

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@...il.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
>>
>>