Re: [PATCH v19 2/2] PCI: Add a quirk for AMD PCIe root ports w/ USB4 controllers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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



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.

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux