Re: [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 12, 2024 at 05:05:57PM +0900, Kobayashi,Daisuke wrote:
> This patch implements a process to output the link status information 
> of the CXL1.1 device to sysfs. The values of the registers related to 
> the link status are outputted into three separate files.
> 
> In CXL1.1, the link status of the device is included in the RCRB mapped to
> the memory mapped register area. This function accesses the address where
> the device's RCRB is mapped.
> 
> 

Spurious blank line in the commit log.

Perhaps include the names of the sysfs files?  And a hint of what they
mean?

I think it's also conventional for the patch to add entries to
Documentation/ABI/...  to show how to use the new files.

> +static u8 cxl_rcrb_get_pcie_cap_offset(void __iomem *addr){

Opening brace would typically be on the next line.

> +	u8 offset;
> +	u32 cap_hdr;
> +
> +	offset = readb(addr + PCI_CAPABILITY_LIST);
> +	cap_hdr = readl(addr + offset);
> +	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
> +		offset = (cap_hdr >> 8) & 0x000000ff;
> +		if (offset == 0) // End of capability list
> +			return 0;
> +		cap_hdr = readl(addr + offset);
> +	}
> +	return offset;

Possibly mimic the name and structure of pci_find_capability(), in
particular, the loop structure of __pci_find_next_cap_ttl().

> +

Spurious blank line.

> +}
> +
> +static u32 cxl_rcrb_to_linkcap(struct device *dev, resource_size_t rcrb)
> +{
> +	void __iomem *addr;
> +	u8 offset;
> +	u32 linkcap;
> +
> +	if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> +		return 0;
> +
> +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> +		return 0;
> +
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr)
> +		goto out;
> +
> +	offset = cxl_rcrb_get_pcie_cap_offset(addr);
> +	if (offset)
> +		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
> +	else
> +		goto out;
> +
> +	linkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> +	iounmap(addr);
> +out:
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	return linkcap;
> +}

> +static u16 cxl_rcrb_to_linkctr(struct device *dev, resource_size_t rcrb)

Why name this "linkctr" when other references here use "linkctrl"?

> +{
> +	void __iomem *addr;
> +	u8 offset;
> +	u16 linkctrl;
> +
> +	if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> +		return 0;
> +
> +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> +		return 0;
> +
> +	addr = ioremap(rcrb, SZ_4K);
> +	if (!addr)
> +		goto out;
> +
> +	offset = cxl_rcrb_get_pcie_cap_offset(addr);
> +	if (offset)
> +		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
> +	else
> +		goto out;
> +
> +	linkctrl = readw(addr + offset + PCI_EXP_LNKCTL);
> +	iounmap(addr);
> +out:
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	return linkctrl;

There's a lot of duplicated boilerplate here between
cxl_rcrb_to_linkcap(), cxl_rcrb_to_linkctr(),
cxl_rcrb_to_linkstatus().

It also seems like a lot of repeated work to search for the PCIe Cap,
ioremap, tear down, etc., for each file, every time it is read.  I
assume most readers will be interested in all three items at the same
time.

> +static umode_t cxl_rcd_visible(struct kobject *kobj,
> +					  struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	if (is_cxl_restricted(pdev))

Not related to *this* patch, but I can't connect the dots between the
"is_cxl_restricted()" name, the meaning of "restricted", and the "CXL
memory expander class code" mentioned in the is_cxl_restricted()
function comment.  It doesn't check the "class code".  It's not
obvious why this applies to RCiEPs but not other endpoints.  No doubt
all obvious to the CXL-initiated, which I am not.

Bjorn