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