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

[Alex_Gagniuc]

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);
>>>>>
>