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

[Lukas Wunner <lukas@wunner.de>]

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

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.

I'm guessing what you really mean is:

		if (pci_upstream_bridge(child)) != pdev)
			continue;

Thanks,

Lukas