linux-kernel - Re: [PATCH v5] PCI: Check for PCIe downtraining conditions

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:   Mon, 6 Aug 2018 18:39:48 +0000
From:   <Alex_Gagniuc@...lteam.com>
To:     <talgi@...lanox.com>, <mr.nuke.me@...il.com>,
        <jakub.kicinski@...ronome.com>
Cc:     <linux-pci@...r.kernel.org>, <bhelgaas@...gle.com>,
        <keith.busch@...el.com>, <Austin.Bolen@...l.com>,
        <Shyam.Iyer@...l.com>, <jeffrey.t.kirsher@...el.com>,
        <ariel.elior@...ium.com>, <michael.chan@...adcom.com>,
        <ganeshgr@...lsio.com>, <tariqt@...lanox.com>, <airlied@...il.com>,
        <alexander.deucher@....com>, <mike.marciniszyn@...el.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] PCI: Check for PCIe downtraining conditions

On 08/05/2018 02:06 AM, Tal Gilboa wrote:
> On 7/31/2018 6:10 PM, Alex G. wrote:
>> On 07/31/2018 01:40 AM, Tal Gilboa wrote:
>> [snip]
>>>>>> @@ -2240,6 +2258,9 @@ static void pci_init_capabilities(struct
>>>>>> pci_dev *dev)
>>>>>>        /* Advanced Error Reporting */
>>>>>>        pci_aer_init(dev);
>>>>>> +    /* Check link and detect downtrain errors */
>>>>>> +    pcie_check_upstream_link(dev);
>>>>
>>>> This is called for every PCIe device right? Won't there be a
>>>> duplicated print in case a device loads with lower PCIe bandwidth
>>>> than needed?
>>>
>>> Alex, can you comment on this please?
>>
>> Of course I can.
>>
>> There's one print at probe() time, which happens if bandwidth < max. I
>> would think that's fine. There is a way to duplicate it, and that is if
>> the driver also calls print_link_status(). A few driver maintainers who
>> call it have indicated they'd be fine with removing it from the driver,
>> and leaving it in the core PCI.
> 
> We would be fine with that as well. Please include the removal in your
> patches.

What's the proper procedure? Do I wait for confirmation from Bjorn 
before knocking on maintainer's doors, or do I William Wallace into 
their trees and demand they merge the removal (pending Bjorn's approval 
on the other side) ?

Alex

>>
>> Should the device come back at low speed, go away, then come back at
>> full speed we'd still only see one print from the first probe. Again, if
>> driver doesn't also call the function.
>> There's the corner case where the device come up at < max, goes away,
>> then comes back faster, but still < max. There will be two prints, but
>> they won't be true duplicates -- each one will indicate different BW.
> 
> This is fine.
> 
>>
>> Alex
>>
>>>>>> +
>>>>>>        if (pci_probe_reset_function(dev) == 0)
>>>>>>            dev->reset_fn = 1;
>>>>>>    }
>>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>>>> index abd5d5e17aee..15bfab8f7a1b 100644
>>>>>> --- a/include/linux/pci.h
>>>>>> +++ b/include/linux/pci.h
>>>>>> @@ -1088,6 +1088,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps);
>>>>>>    u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev
>>>>>> **limiting_dev,
>>>>>>                     enum pci_bus_speed *speed,
>>>>>>                     enum pcie_link_width *width);
>>>>>> +void __pcie_print_link_status(struct pci_dev *dev, bool verbose);
>>>>>>    void pcie_print_link_status(struct pci_dev *dev);
>>>>>>    int pcie_flr(struct pci_dev *dev);
>>>>>>    int __pci_reset_function_locked(struct pci_dev *dev);
>>>>>
>