Messages in this thread | | | From | <> | Subject | Re: [PATCH v3] PCI: Check for PCIe downtraining conditions | Date | Mon, 16 Jul 2018 22:28:35 +0000 |
| |
On 7/16/2018 4:17 PM, Bjorn Helgaas wrote: > [+cc maintainers of drivers that already use pcie_print_link_status() > and GPU folks]
Thanks for finding them!
[snip] >> identifying this from userspace is neither intuitive, nor straigh >> forward. > > s/straigh/straight/ > In this context, I think "straightforward" should be closed up > (without the space).
That's a straightforward edit. Thanks for the feedback!
>> The easiest way to detect this is with pcie_print_link_status(), >> since the bottleneck is usually the link that is downtrained. It's not >> a perfect solution, but it works extremely well in most cases. > > This is an interesting idea. I have two concerns: > > Some drivers already do this on their own, and we probably don't want > duplicate output for those devices. In most cases (ixgbe and mlx* are > exceptions), the drivers do this unconditionally so we *could* remove > it from the driver if we add it to the core. The dmesg order would > change, and the message wouldn't be associated with the driver as it > now is.
Oh, there are only 8 users of that. Even I could patch up the drivers to remove the call, assuming we reach agreement about this change.
> Also, I think some of the GPU devices might come up at a lower speed, > then download firmware, then reset the device so it comes up at a > higher speed. I think this patch will make us complain about about > the low initial speed, which might confuse users.
I spoke to one of the PCIe spec writers. It's allowable for a device to downtrain speed or width. It would also be extremely dumb to downtrain with the intent to re-train at a higher speed later, but it's possible devices do dumb stuff like that. That's why it's an informational message, instead of a warning.
Another case: Some devices (lower-end GPUs) use silicon (and marketing) that advertises x16, but they're only routed for x8. I'm okay with seeing an informational message in this case. In fact, I didn't know that my Quadro card for three years is only wired for x8 until I was testing this patch.
> So I'm not sure whether it's better to do this in the core for all > devices, or if we should just add it to the high-performance drivers > that really care.
You're thinking "do I really need that bandwidth" because I'm using a function called "_bandwidth_". The point of the change is very far from that: it is to help in system troubleshooting by detecting downtraining conditions.
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com> [snip] >> + /* Look from the device up to avoid downstream ports with no devices. */ >> + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT) && >> + (pci_pcie_type(dev) != PCI_EXP_TYPE_LEG_END) && >> + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM)) >> + return; > > Do we care about Upstream Ports here?
YES! Switches. e.g. an x16 switch with 4x downstream ports could downtrain at 8x and 4x, and we'd never catch it.
> I suspect that ultimately we > only care about the bandwidth to Endpoints, and if an Endpoint is > constrained by a slow link farther up the tree, > pcie_print_link_status() is supposed to identify that slow link.
See above.
> I would find this test easier to read as > > if (!(type == PCI_EXP_TYPE_ENDPOINT || type == PCI_EXP_TYPE_LEG_END)) > return; > > But maybe I'm the only one that finds the conjunction of inequalities > hard to read. No big deal either way. > >> + /* Multi-function PCIe share the same link/status. */ >> + if ((PCI_FUNC(dev->devfn) != 0) || dev->is_virtfn) >> + return; >> + >> + pcie_print_link_status(dev); >> +} >> + >> static void pci_init_capabilities(struct pci_dev *dev) >> { >> /* Enhanced Allocation */ >> @@ -2181,6 +2200,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); >> + >> if (pci_probe_reset_function(dev) == 0) >> dev->reset_fn = 1; >> } >> -- >> 2.14.4 >> >
| |