Re: [PATCH v3 6/9] cxl/pci: Do not ignore PCI config read errors in match_add_dports()

[Robert Richter <rrichter@amd.com>]

Bjorn,

thank you for your review, I will address all your comments you made
also for other patches.

On 09.11.22 17:09:56, Bjorn Helgaas wrote:
> On Wed, Nov 09, 2022 at 11:40:56AM +0100, Robert Richter wrote:

> > @@ -47,7 +47,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)
> >  		return 0;
> >  	if (pci_read_config_dword(pdev, pci_pcie_cap(pdev) + PCI_EXP_LNKCAP,
> >  				  &lnkcap))
> 
> You didn't change this, but I recommend using
> pcie_capability_read_dword() when reading the PCIe Capability.  It
> takes care of some annoying corner cases like devices that don't
> implement Link Cap and the different versions of the PCIe Capability.

This is a good hint, I looked into pcie_capability_read_dword() and
found the function is checking the pci_pcie_type(). For CXL VH it is
ok as the type is an endpoint, but for the RCD case it is an RCiEP.

Two issues arise here, there are those options:

1) Device implements CXL UP RCRB (3.0, 8.2.1.2)

The link capability must be read from PCIe caps of the UP RCRB instead
of the RCiEP. The implementation of pci_dev_add_dport() and
restricted_host_enumerate_dport() in a later patch of this series
("cxl/pci: Extend devm_cxl_port_enumerate_dports() to support
restricted hosts (RCH)") is not sufficient and must be changed to use
RCRB to determine the port id if it is an RCD.

2) Device does not implement CXL UP RCRB (3.0, 9.11.8)

The RCRB reads all FFs and CXL DVSEC 7 is accessible through the
device's config space now to avoid register remappings. Since there is
no RCD UP type 0 config space any longer, I would assume the UP's PCIe
caps with the link cap would be also made available through the
endpoint, but now it is an RCiEP. This violates the PCIe base spec
which does not allow to implement the link caps (PCIe base 6.0, 7.5.3
PCI Express Capability Structure, Link Capabilities). This makes sense
as an RCiEP is not connected to a root or downstream port and thus
does not have an upstream port. Strictly looking into the wording of
the PCI base spec, Link caps are not required for RCiEP and thus is
not prohibitedi and could be implemented optionnally. But CXL 3.0 spec
is more explicit here: "[The Link Capabilities] for an RCiEP [...]
shall not be implemented by the Device 0, Function 0" (CXL 3.0,
8.2.1.2).

I think, the spec's intention in 9.11.8 is to reduce remappings and
the device should just pass through the Link caps as there is a hidden
upstream port the RCiEP connected to it. Anyway, a CXL spec
clarification is needed here and pcie_capability_read_dword() needs to
be adjusted then for the RCiEP/RCD case.

Which raises another question to extend struct pci_dev the following:

#ifdef CXL_PCI
	u16	cxl_dev_cap;	/* CXL DVSEC 3, 8.1.3.1 DVSEC CXL Capability (Offset 0Ah)*/
	u16	cxl_port_cap;	/* CXL DVSEC 7, 8.2.1.3.1 DVSEC Flex Bus Port Capability (Offset 0Ah) */
#endif

Note: At least one cap is mandatory for all kind of CXL devices, see
CXL 3.0, Table 8-2.

There could be a helper then for a CXL check:

static inline bool dev_is_cxl(struct pci_dev *dev)
{
	return dev->cxl_dev_cap || dev->cxl_port_cap;
}

Thanks,

-Robert