diff mbox series

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

Message ID 20221109104059.766720-7-rrichter@amd.com
State New
Headers
Series cxl: Add support for Restricted CXL hosts (RCD mode) |

Commit Message

Robert Richter Nov. 9, 2022, 10:40 a.m. UTC 
  The link capabilities of a PCI device are read when enumerating its
dports. This is done by reading the PCI config space. If that fails
port enumeration ignores that error. However, reading the PCI config
space should reliably work.

To reduce some complexity to the code flow when factoring out parts of
the code in match_add_dports() for later reuse, change this to throw
an error.

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bjorn Helgaas Nov. 9, 2022, 11:09 p.m. UTC | #1 
On Wed, Nov 09, 2022 at 11:40:56AM +0100, Robert Richter wrote:
> The link capabilities of a PCI device are read when enumerating its
> dports. This is done by reading the PCI config space. If that fails
> port enumeration ignores that error. However, reading the PCI config
> space should reliably work.
> 
> To reduce some complexity to the code flow when factoring out parts of
> the code in match_add_dports() for later reuse, change this to throw
> an error.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0dbbe8d39b07..8271b8abde7a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -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.

> -		return 0;
> +		return -ENXIO;
>  
>  	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
>  	if (rc)
> -- 
> 2.30.2
>
Robert Richter Nov. 11, 2022, 11:56 a.m. UTC | #2 
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
Robert Richter Nov. 11, 2022, 12:07 p.m. UTC | #3 
On 11.11.22 12:56:56, Robert Richter wrote:
> 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)*/

Correct is CXL DVSEC 0 here.

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

-Robert
Dan Williams Nov. 16, 2022, 7:36 p.m. UTC | #4 
Robert Richter wrote:
> The link capabilities of a PCI device are read when enumerating its
> dports. This is done by reading the PCI config space. If that fails
> port enumeration ignores that error. However, reading the PCI config
> space should reliably work.
> 
> To reduce some complexity to the code flow when factoring out parts of
> the code in match_add_dports() for later reuse, change this to throw
> an error.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0dbbe8d39b07..8271b8abde7a 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -47,7 +47,7 @@ static int match_add_dports(struct pci_dev *pdev, void *data)

match_add_dports() never comes into play in the RCH topology case. There
are no switch ports to handle and CXL host-bridges are only ever dports
in the RCH case.

I will post the cxl_test enabling for an RCH topology so we can compare
notes there.
diff mbox series

Patch

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0dbbe8d39b07..8271b8abde7a 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -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))
-		return 0;
+		return -ENXIO;
 
 	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
 	if (rc)