Re: [PATCH v14 1/2] cxl/core/regs: Add rcd_pcie_cap initialization

[Dan Williams <dan.j.williams@intel.com>]

Kobayashi,Daisuke wrote:
> Add rcd_pcie_cap and its initialization to cache the offset of cxl1.1 
> device link status information. By caching it, avoid the walking 
> memory map area to find the offset when output the register value.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/core/core.h |  6 ++++
>  drivers/cxl/core/regs.c | 61 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h       |  9 ++++++
>  drivers/cxl/pci.c       |  8 ++++--
>  4 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 3b64fb1b9ed0..69006bde7ad5 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -74,6 +74,12 @@ resource_size_t __rcrb_to_component(struct device *dev,
>  				    struct cxl_rcrb_info *ri,
>  				    enum cxl_rcrb which);
>  u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
> +resource_size_t cxl_rcrb_to_linkcap(struct device *dev, struct cxl_dport *dport);

Why is this function public? If it only has a caller within regs.c it
can be marked static.

> +#define PCI_RCRB_CAP_LIST_ID_MASK	GENMASK(7, 0)
> +#define PCI_RCRB_CAP_HDR_ID_MASK	GENMASK(7, 0)
> +#define PCI_RCRB_CAP_HDR_NEXT_MASK	GENMASK(15, 8)
> +#define PCI_CAP_EXP_SIZEOF		0x3c
>  
>  extern struct rw_semaphore cxl_dpa_rwsem;
>  extern struct rw_semaphore cxl_region_rwsem;
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 372786f80955..8a7cef1cf87d 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -505,6 +505,67 @@ u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb)
>  	return offset;
>  }
>  
> +resource_size_t cxl_rcrb_to_linkcap(struct device *dev, struct cxl_dport *dport)
> +{
> +	resource_size_t rcrb = dport->rcrb.base;
> +	void __iomem *addr;
> +	u32 cap_hdr;
> +	u16 offset;
> +
> +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))
> +		return CXL_RESOURCE_NONE;
> +
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr) {
> +		dev_err(dev, "Failed to map region %pr\n", addr);
> +		release_mem_region(rcrb, SZ_4K);
> +		return CXL_RESOURCE_NONE;
> +	}
> +
> +	offset = FIELD_GET(PCI_RCRB_CAP_LIST_ID_MASK, readw(addr + PCI_CAPABILITY_LIST));
> +	cap_hdr = readl(addr + offset);
> +	while ((FIELD_GET(PCI_RCRB_CAP_HDR_ID_MASK, cap_hdr)) != PCI_CAP_ID_EXP) {
> +		offset = FIELD_GET(PCI_RCRB_CAP_HDR_NEXT_MASK, cap_hdr);
> +		if (offset == 0 || offset > SZ_4K) {
> +			offset = 0;
> +			break;
> +		}
> +		cap_hdr = readl(addr + offset);

I would feel better if this duplicated __pci_find_next_cap_ttl() as
__rcrb_find_next_cap_ttl() and built an rcrb_find_capability() helper on
top of that.

> +	}
> +
> +	iounmap(addr);
> +	release_mem_region(rcrb, SZ_4K);
> +	if (!offset)
> +		return CXL_RESOURCE_NONE;
> +
> +	return offset;
> +}
> +
> +int cxl_dport_map_rcd_linkcap(struct pci_dev *pdev)
> +{
> +	void __iomem *dport_pcie_cap = NULL;
> +	resource_size_t rcd_pcie_offset;
> +	struct cxl_rcrb_info *ri;
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +
> +	port = cxl_pci_find_port(pdev, &dport);
> +	if (!port)
> +		return -EPROBE_DEFER;

There is a missing put_device() for this find. It also seems silly to
find the port again after already finding it for
cxl_rcrb_get_comp_regs().

> +
> +	ri = &dport->rcrb;
> +	rcd_pcie_offset = cxl_rcrb_to_linkcap(&pdev->dev, dport);
> +	if (rcd_pcie_offset > 0)

This looks broken if cxl_rcrb_to_linkcap() returns CXL_RESOURCE_NONE.

Also, lets s/rcd_pcie_offset/pos/ since @pos is already the common
variable name for tracking PCI config register offsets.

> +		dport_pcie_cap = devm_cxl_iomap_block(&pdev->dev,
> +						      ri->base + rcd_pcie_offset,
> +						      PCI_CAP_EXP_SIZEOF);
> +
> +	dport->regs.rcd_pcie_cap = dport_pcie_cap;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_dport_map_rcd_linkcap, CXL);
> +
>  resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri,
>  				    enum cxl_rcrb which)
>  {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 003feebab79b..7e37ad6d84d6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -230,6 +230,14 @@ struct cxl_regs {
>  	struct_group_tagged(cxl_rch_regs, rch_regs,
>  		void __iomem *dport_aer;
>  	);
> +
> +	/*
> +	 * RCD upstream port specific PCIe cap register
> +	 * @pcie_cap: CXL 3.0 8.2.1.2 RCD Upstream Port RCRB
> +	 */
> +	struct_group_tagged(cxl_rcd_regs, rcd_regs,
> +		void __iomem *rcd_pcie_cap;
> +	);
>  };
>  
>  struct cxl_reg_map {
> @@ -299,6 +307,7 @@ int cxl_setup_regs(struct cxl_register_map *map);
>  struct cxl_dport;
>  resource_size_t cxl_rcd_component_reg_phys(struct device *dev,
>  					   struct cxl_dport *dport);
> +int cxl_dport_map_rcd_linkcap(struct pci_dev *pdev);
>  
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..bbc55732d6c1 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -512,11 +512,15 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	 * is an RCH and try to extract the Component Registers from
>  	 * an RCRB.
>  	 */
> -	if (rc && type == CXL_REGLOC_RBI_COMPONENT && is_cxl_restricted(pdev))
> +	if (rc && type == CXL_REGLOC_RBI_COMPONENT && is_cxl_restricted(pdev)) {
>  		rc = cxl_rcrb_get_comp_regs(pdev, map);
> +		if (rc)
> +			return rc;
>  
> -	if (rc)
> +		cxl_dport_map_rcd_linkcap(pdev);

There is a chance that the port gets removed between finding it for
cxl_rcrb_get_comp_regs() and finding it again for
cxl_dport_map_rcd_linkcap(), so this misses the need to return
EPROBE_DEFER if that latter race is lost. However, is also a chance that
race can be lost *internal* to cxl_rcrb_get_comp_regs().

The only guarantee of cxl_pci_find_port() is that it returns a 'struct
cxl_port' instance that will not be freed until put_device(&port->dev),
but it makes no guarantee that the dport information is stable over that
lifetime. I think this needs a leading fix patch that finds and locks
the port and then passes that port to cxl_rcrb_get_comp_regs() and
cxl_dport_map_rcd_linkcap().

Otherwise I think you could crash the kernel with a test that repeatedly
reloads the cxl_acpi module while also reloading cxl_pci.

Ugh, I was going to say copy what cxl_mem_probe() does around locking
endpoint_parent before attaching further ports, but that also appears to
not handle the same race. I.e. I think cxl_mem_probe() needs a fix to do
this as well. I will copy you on a proposed patch for that.