> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, February 28, 2024 1:44 AM
> To: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@xxxxxxxxxxx>
> Cc: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@xxxxxxxxxxx>;
> linux-cxl@xxxxxxxxxxxxxxx; Gotou, Yasunori/五島 康文 <y-goto@xxxxxxxxxxx>;
> linux-pci@xxxxxxxxxxxxxxx; mj@xxxxxx; dan.j.williams@xxxxxxxxx
> Subject: Re: [RFC PATCH v2 1/3] Add sysfs attribute for CXL 1.1 device link
> status
>
> 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.
>
Thank you, I will fix them.
> > +
> > + 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.
>
I'll take your suggestion and create a helper function.
> > + 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.
>
I will check them.
> > + 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
Thank you, I overlooked my consideration of the removal issue.
I will check current code and add a cleanup process.
