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

[Tal Gilboa]

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.

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