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@xxxxxxxxx>
[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
>>
>
