On 9/16/2023 08:36, Lukas Wunner wrote:
On Sat, Sep 16, 2023 at 08:09:19AM -0500, Mario Limonciello wrote:
On 9/15/2023 23:48, Lukas Wunner wrote:
On Fri, Sep 15, 2023 at 07:04:11AM -0500, Mario Limonciello wrote:
On 9/15/2023 02:08, Lukas Wunner wrote:
On Thu, Sep 14, 2023 at 09:33:54PM -0500, Mario Limonciello wrote:
+static bool child_has_amd_usb4(struct pci_dev *pdev)
+{
+ struct pci_dev *child = NULL;
+
+ while ((child = pci_get_class(PCI_CLASS_SERIAL_USB_USB4, child))) {
+ if (child->vendor != PCI_VENDOR_ID_AMD)
+ continue;
+ if (pcie_find_root_port(child) != pdev)
+ continue;
+ return true;
+ }
+
+ return false;
+}
What's the purpose of the pcie_find_root_port() check? PCI is a hierarchy,
not a graph, so a device cannot have any other Root Port but the one below
which you're searching.
If the purpose is to check that the port is a Root Port (if the PCI IDs
you're using in the DECLARE_PCI_FIXUP_* clauses match non-Root Ports),
check for pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT. (No need to
check for that in every loop iteration obviously, just check once in
the fixup.)
Thanks,
Lukas
The reason to look for it the way that I did was that there are multiple
root ports with the exact same PCI ID.
The problem only occurs on the root port that happens to have an AMD USB4
controller connected.
Yes but what's the purpose of the pcie_find_root_port(child) check
quoted above?
You're right that if you look at this system alone that the check isn't
strictly necessary. It's to future proof the quirk. If a discrete USB4
controller was connected to the system it would be connected to a different
root port (the one that is used for PCI tunneling).
AMD doesn't have any of these devices, but if some day one was created it
could trip this codepath.
If you feel it's better to remove the check unless such a device is created
sure I can drop it.
PCIe ports used for Thunderbolt tunneling are Downstream Ports or
Upstream Ports (depending on which of the two ends of the tunnel
you're looking at).
Let me explain the topology from an actual system with PCIe tunneling
active and a dock connected downstream.
-[0000:00]-+-00.0
+-04.1-[33-62]----00.0-[34-36]--+-02.0-[35]----00.0
| \-04.0-[36]--
+-08.3-[64]--+-00.0
| +-00.3
| +-00.4
| +-00.5
| \-00.6
This is the root port that tunneled devices will connect to:
$ sudo lspci -s 0000:00:04.1 -v | grep "Capabilities: \[58\]"
Capabilities: [58] Express Root Port (Slot+), MSI 00
This is the root port that the USB4 controllers are connected to that is
problematic:
$ sudo lspci -s 0000:00:08.3 -v | grep "Capabilities: \[58\]"
Capabilities: [58] Express Root Port (Slot-), MSI 00
Here's a downstream connected dock from tunneling:
33:00.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)
34:02.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)
34:04.0 PCI bridge [0604]: Intel Corporation JHL7540 Thunderbolt 3
Bridge [Titan Ridge DD 2018] [8086:15ef] (rev 06)
Here's the USB4 controllers connected to 08.3:
64:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:161f]
64:00.3 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15d6]
64:00.4 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:15d7]
64:00.5 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:162e]
64:00.6 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device
[1022:162f]
If an AMD USB4 controller happened to be connected, it would have shown
up connected to 04.1 as it's root port.
The "pcie_find_root_port(child) != pdev" check is always false:
You're searching for a USB controller below a Root Port and
check whether the Root Port in the USB controller's ancestry
is the Root Port below which you're searching. That's a
tautology.
The search doesn't start at pdev, it starts at NULL.
struct pci_dev *child = NULL;
I'm guessing what you really mean is:
if (pci_upstream_bridge(child)) != pdev)
continue;
That's exactly what I had before and Rafael suggested [1]
to change it to pcie_find_root_port().
[1]
https://lore.kernel.org/linux-usb/20230913040832.114610-1-mario.limonciello@xxxxxxx/T/#m66acb79a13d314b5e868993b1611266a968cf064
Thanks,
Lukas
