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
