On 4/24/19 12:19 PM, Alex Williamson wrote:
On Wed, 24 Apr 2019 16:45:45 +0000
<Alex_Gagniuc@xxxxxxxxxxxx> wrote:
On 4/23/2019 5:42 PM, Alex Williamson wrote:
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 7e12d0163863..233cd4b5b6e8 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2403,6 +2403,19 @@ void pcie_report_downtraining(struct pci_dev *dev)
I don't think you want to change pcie_report_downtraining(). You're
advertising to "report" something, by nomenclature, but then go around
and also call a notification callback. This is also used during probe,
and you've now just killed your chance to notice you've booted with a
degraded link.
If what you want to do is silence the bandwidth notification, you want
to modify the threaded interrupt that calls this.
During probe, ie. discovery, a device wouldn't have a driver attached,
so we'd fall through to simply printing the link status. Nothing
lost afaict. The "report" verb doesn't have a subject here, report to
whom? Therefore I thought it reasonable that a driver ask that it be
reported to them via a callback. I don't see that as such a stretch of
the interface.
That's just stretching the logic, and IMO makes the intent harder to
understand. The argument relies on the state of the PCI device and
logic, which is not obvious to the casual observer. If you want to
bypass the bandwidth notification, then bypass the notification.
if (PCI_FUNC(dev->devfn) != 0 || dev->is_virtfn)
return;
+ /*
+ * If driver handles link_change event, defer to driver. PCIe drivers
+ * can call pcie_print_link_status() to print current link info.
+ */
+ device_lock(&dev->dev);
+ if (dev->driver && dev->driver->err_handler &&
+ dev->driver->err_handler->link_change) {
+ dev->driver->err_handler->link_change(dev);
+ device_unlock(&dev->dev);
+ return;
+ }
+ device_unlock(&dev->dev);
Can we write this such that there is a single lock()/unlock() pair?
Not without introducing a tracking variable, ex.
[snip bad code]
That's not markedly better imo, but if it's preferred I can send a v2.
How about:
if (!invoke_link_changed_handler(pdev))
very_useful_downtraining_message(pdev);
Alex
Alex
