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

[Alex G.]

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.

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.

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