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
