Re: [PATCH v8 2/2] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu,  6 Jun 2024 16:48:14 +0900
"Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com> wrote:

> Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> 
> In CXL1.1, the link status of the device is included in the RCRB mapped to
> the memory mapped register area. Critically, that arrangement makes the
> link status and control registers invisible to existing PCI user tooling.
> 
> Export those registers via sysfs with the expectation that PCI user
> tooling will alternatively look for these sysfs files when attempting to
> access to these CXL 1.1 endpoints registers.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/pci.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..e157959dffc2 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -786,6 +786,96 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	return 0;
>  }
>  
> +static u32 get_rcd_pcie_caps(struct device *dev, u16 offset)
> +{
> +	struct cxl_dev_state *cxlds = dev_get_drvdata(dev);
> +	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> +	struct device *endpoint_parent;
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +	resource_size_t rcrb;
> +	void __iomem *addr;
> +	u32 ret;
> +
> +	port = cxl_mem_find_port(cxlmd, &dport);
> +	if (!port)
> +		return 0;
> +
> +	endpoint_parent = port->uport_dev;
> +	if (!endpoint_parent)
> +		return 0;
> +
> +	guard(device)(endpoint_parent);
> +	if (!endpoint_parent->driver)
> +		return 0;
> +
> +	rcrb = dport->rcrb.base;
> +	if (!request_mem_region(rcrb, SZ_4K, "CXL RCRB"))

I was assuming we would find the address of the capability only once
and map the relevant cap in for all the time the driver is
bound, as is done for aer_cap.

I'd be worried that this might race with other accesses to rcrb
in the future.

I suspect best option is to keep the capability always mapped
in similar fashion to cxl_dport_map_rch_aer() does it for
the aer registers.



> +		return 0;
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr) {
> +		dev_err(dev, "Failed to map region %pr\n", addr);
> +		release_mem_region(rcrb, SZ_4K);
> +		return 0;
> +	}
> +
> +	ret = readl(addr + dport->rcrb.rcd_pcie_cap + offset);
> +	release_mem_region(rcrb, SZ_4K);
> +	return ret;
> +}
> +
> +static ssize_t rcd_link_cap_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	u32 linkcap = get_rcd_pcie_caps(dev, PCI_EXP_LNKCAP);
> +
> +	return sysfs_emit(buf, "%x\n", linkcap);
> +}
> +static DEVICE_ATTR_RO(rcd_link_cap);
> +
> +static ssize_t rcd_link_ctrl_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	u16 linkctl = get_rcd_pcie_caps(dev, PCI_EXP_LNKCTL);
> +
> +	return sysfs_emit(buf, "%x\n", linkctl);
> +}
> +static DEVICE_ATTR_RO(rcd_link_ctrl);
> +
> +static ssize_t rcd_link_status_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	u16 linksta = get_rcd_pcie_caps(dev, PCI_EXP_LNKSTA);
> +
> +	return sysfs_emit(buf, "%x\n", linksta);

Local variable doesn't add to readability as the PCI_EXP_LNKSTA
makes it pretty obvious what is being queried.

	return sysfs_emit(buf, "%x\n",
			  get_rcd_pcie_caps(dev, PCI_EXP_LNKSTA));

> +}
> +static DEVICE_ATTR_RO(rcd_link_status);
> +
> +static struct attribute *cxl_rcd_attrs[] = {
> +		&dev_attr_rcd_link_cap.attr,
> +		&dev_attr_rcd_link_ctrl.attr,

Indent one less tab.

> +		&dev_attr_rcd_link_status.attr,
> +		NULL
> +};