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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Feb 27, 2024 at 05:33:11PM +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.

> +static u32 cxl_rcrb_to_linkcap(struct device *dev, resource_size_t rcrb)
> +{
> +	void __iomem *addr;
> +	u8 offset = 0;

Unnecessary initialization.  Also, readw() returns u16.

> +	u32 cap_hdr;
> +	u32 linkcap = 0;

Ditto.

> +
> +	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 = readw(addr + PCI_CAPABILITY_LIST);
> +	offset = offset & 0x00ff;
> +	cap_hdr = readl(addr + offset);
> +	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
> +		offset = (cap_hdr >> 8) & 0x000000ff;
> +		if (!offset)
> +			break;
> +		cap_hdr = readl(addr + offset);
> +	}

Hmmm, it's a shame we have to reimplement pci_find_capability() here.
I see the problem though -- pci_find_capability() does config reads
and this is in MMIO space, not config space.

But you need this several times, so maybe a helper function would
still be useful so you don't have to repeat the code.

> +	if (offset)
> +		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);

Testing "offset" acknowledges the possibility that it may be NULL, and
in that case, the readl() below is a NULL pointer dereference.

> +	linkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> +	iounmap(addr);
> +out:
> +	release_mem_region(rcrb, SZ_4K);
> +
> +	return linkcap;
> +}

> @@ -806,6 +1003,9 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(mds))
>  		return PTR_ERR(mds);
>  	cxlds = &mds->cxlds;
> +	device_create_file(&pdev->dev, &dev_attr_rcd_link_cap);
> +	device_create_file(&pdev->dev, &dev_attr_rcd_link_ctrl);
> +	device_create_file(&pdev->dev, &dev_attr_rcd_link_status);

Is there a removal issue here?  What if "pdev" is removed?  Or what if
this module is unloaded?  Do these sysfs files get cleaned up
automagically somehow?

Bjorn