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

[Dan Williams <dan.j.williams@intel.com>]

Daisuke Kobayashi (Fujitsu) wrote:
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Wednesday, February 28, 2024 1:44 AM
> > To: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com>
> > Cc: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com>;
> > linux-cxl@vger.kernel.org; Gotou, Yasunori/五島 康文 <y-goto@fujitsu.com>;
> > linux-pci@vger.kernel.org; mj@ucw.cz; dan.j.williams@intel.com
> > 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.

There is already a cxl_rcrb_to_aer() in the CXL core, I would recommend
just converting that function to one that take a capability id.

[..]
> > > @@ -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.
> 

No, the proper way to register sysfs attributes already includes cleanup
tied to the device lifetime. Since these can only show up once the driver
is loaded they are so-called driver "dev_groups" attributes. The way to
declare them is something like this (UNTESTED!):

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..c8535078c74f 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -964,6 +964,30 @@ static const struct pci_error_handlers cxl_error_handlers = {
        .cor_error_detected     = cxl_cor_error_detected,
 };
 
+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))
+               return 0;
+       return a->mode;
+}
+
+static struct attribute *cxl_rcd_attrs[] = {
+       &dev_attr_rcd_link_cap,
+       &dev_attr_rcd_link_ctrl,
+       &dev_attr_rcd_link_status,
+       NULL,
+};
+
+static struct attribute_group cxl_rcd_group = {
+       .attrs = cxl_rcd_attrs,
+       .is_visible = cxl_rcd_visible,
+};
+
+__ATTRIBUTE_GROUPS(cxl_rcd);
+
 static struct pci_driver cxl_pci_driver = {
        .name                   = KBUILD_MODNAME,
        .id_table               = cxl_mem_pci_tbl,
@@ -971,6 +995,7 @@ static struct pci_driver cxl_pci_driver = {
        .err_handler            = &cxl_error_handlers,
        .driver = {
                .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
+               .dev_groups     = &cxl_rcd_groups,
        },
 };