Daisuke Kobayashi (Fujitsu) wrote:
>
>
> > -----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.
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,
},
};
