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

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

On Mon, 10 Jun 2024 17:22:22 +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>
Hi

Thanks for quick turn around.  A few follow up comments inline.

Jonathan

> ---
>  drivers/cxl/pci.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..0a09d1250f1d 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -786,6 +786,78 @@ 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;
> +
> +	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;
> +
> +	return readl(dport->regs.rcd_pcie_cap + offset);

I'd sanity check if dport->regs.rcd_pcie_cap == NULL
Maybe we have broken hardware which is an RCD but this cap is missing.
If that happens, return -EINVAL or similar here.

 
> +}
> +
> +static ssize_t rcd_link_cap_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%x\n",
> +					  get_rcd_pcie_caps(dev, PCI_EXP_LNKCAP));

Alignment issue.  Tabs need to be same length as 8 spaces for kernel code.


Also you cast in all other cases which seems odd.

> +}
> +static DEVICE_ATTR_RO(rcd_link_cap);
> +
> +static ssize_t rcd_link_ctrl_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%x\n",
> +					  (u16)get_rcd_pcie_caps(dev, PCI_EXP_LNKCTL));
> +}
> +static DEVICE_ATTR_RO(rcd_link_ctrl);
> +
> +static ssize_t rcd_link_status_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%x\n",
> +					  (u16)get_rcd_pcie_caps(dev, PCI_EXP_LNKSTA));
> +}
> +static DEVICE_ATTR_RO(rcd_link_status);