[RFC PATCH v2 0/3] Display cxl1.1 device link status

[RFC PATCH v2 0/3] Display cxl1.1 device link status

[Kobayashi,Daisuke]

Hello.

This patch series adds a feature that displays the link status
of the CXL1.1 device.

CXL devices are extensions of PCIe. Therefore, from CXL2.0 onwards,
the link status can be output in the same way as traditional PCIe.
However, unlike devices from CXL2.0 onwards, CXL1.1 requires a
different method to obtain the link status from traditional PCIe.
This is because the link status of the CXL1.1 device is not mapped
in the configuration space (as per cxl3.0 specification 8.1).
Instead, the configuration space containing the link status is mapped
to the memory mapped register region (as per cxl3.0 specification 8.2,
Table 8-18). Therefore, the current lspci has a problem where it does
not display the link status of the CXL1.1 device. 
This patch solves these issues.

The procedure is as follows:
First, obtain the RCRB address within the cxl driver, then access 
the configuration space. Next, output the link status information from
the configuration space to sysfs. Finally, read sysfs within lspci to 
display the link status information.

Changes
v1[1] -> v2:
The following are the main changes made based on the feedback from Dan Williams.
- Modified to perform rcrb access within the CXL driver.
- Added new attributes to the sysfs of the PCI device.
- Output the link status information to the sysfs of the PCI device.
- Retrieve information from sysfs as the source when displaying information in lspci.


[1]
https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06@fujitsu.com/


Kobayashi,Daisuke (3):
  Add sysfs attribute for CXL 1.1 device link status
  Removed conditional branches that are not suitable for cxl1.1 devices

 drivers/cxl/acpi.c |   4 -
 drivers/cxl/pci.c  | 206 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 206 insertions(+), 4 deletions(-)
  
  Add function to display cxl1.1 device link status

 ls-caps.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)
-- 
2.43.0

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

[Kobayashi,Daisuke]

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.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/cxl/pci.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4fd1f207c84e..8302249819d0 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -780,6 +780,203 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
 
 	return 0;
 }
+static u32 cxl_rcrb_to_linkcap(struct device *dev, resource_size_t rcrb)
+{
+	void __iomem *addr;
+	u8 offset = 0;
+	u32 cap_hdr;
+	u32 linkcap = 0;
+
+	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);
+	}
+
+	if (offset)
+		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
+
+	linkcap = readl(addr + offset + PCI_EXP_LNKCAP);
+	iounmap(addr);
+out:
+	release_mem_region(rcrb, SZ_4K);
+
+	return linkcap;
+}
+
+static ssize_t rcd_link_cap_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+	u32 linkcap;
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+	linkcap = cxl_rcrb_to_linkcap(dev, dport->rcrb.base + SZ_4K);
+	return sysfs_emit(buf, "%x\n", linkcap);
+}
+static DEVICE_ATTR_RO(rcd_link_cap);
+
+static u16 cxl_rcrb_to_linkctr(struct device *dev, resource_size_t rcrb)
+{
+	void __iomem *addr;
+	u8 offset = 0;
+	u16 linkctrl = 0;
+	u32 cap_hdr;
+
+	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);
+	}
+
+	if (offset)
+		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
+	linkctrl = readw(addr + offset + PCI_EXP_LNKCTL);
+
+	iounmap(addr);
+out:
+	release_mem_region(rcrb, SZ_4K);
+
+	return linkctrl;
+}
+
+static ssize_t rcd_link_ctrl_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+	u16 linkctrl;
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+
+	linkctrl = cxl_rcrb_to_linkctr(dev, dport->rcrb.base + SZ_4K);
+
+	return sysfs_emit(buf, "%x\n", linkctrl);
+}
+static DEVICE_ATTR_RO(rcd_link_ctrl);
+
+static u16 cxl_rcrb_to_linkstatus(struct device *dev, resource_size_t rcrb)
+{
+	void __iomem *addr;
+	u8 offset = 0;
+	u16 linksta = 0;
+	u32 cap_hdr;
+
+	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);
+	}
+
+	if (offset)
+		dev_dbg(dev, "found PCIe capability (0x%x)\n", offset);
+
+	linksta = readw(addr + offset + PCI_EXP_LNKSTA);
+	iounmap(addr);
+out:
+	release_mem_region(rcrb, SZ_4K);
+
+	return linksta;
+}
+
+static ssize_t rcd_link_status_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+	u16 linkstatus;
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+
+	linkstatus = cxl_rcrb_to_linkstatus(dev, dport->rcrb.base + SZ_4K);
+
+	return sysfs_emit(buf, "%x\n", linkstatus);
+}
+static DEVICE_ATTR_RO(rcd_link_status);
+
+static struct attribute *cxl_rcd_attrs[] = {
+		&dev_attr_rcd_link_cap.attr,
+		&dev_attr_rcd_link_ctrl.attr,
+		&dev_attr_rcd_link_status.attr,
+		NULL
+};
+
+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 a->mode;
+
+	return 0;
+}
+
+static struct attribute_group cxl_rcd_group = {
+		.attrs = cxl_rcd_attrs,
+		.is_visible = cxl_rcd_visible,
+};
+
+__ATTRIBUTE_GROUPS(cxl_rcd);
 
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *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);
 	pci_set_drvdata(pdev, cxlds);
 
 	cxlds->rcd = is_cxl_restricted(pdev);
@@ -967,6 +1167,7 @@ static struct pci_driver cxl_pci_driver = {
 	.err_handler		= &cxl_error_handlers,
 	.driver	= {
 		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
+		.dev_groups	= cxl_rcd_groups,
 	},
 };
 
-- 
2.43.0

[RFC PATCH v2 2/3] Remove conditional branch that is not suitable for cxl1.1 devices

[Kobayashi,Daisuke]

This patch removes conditional branching that does not comply with the specifications.

In the existing code, there is a conditional branch that compares "chbs->length" 
with "CXL_RCRB_SIZE". However, according to the specifications, "chbs->length" 
indicates the length of the CHBS structure in the cedt, not the size of the 
configuration space. Therefore, since this condition does not comply with
the specifications(cxl3.0 specification 9.17.1), remove it.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/cxl/acpi.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index dcf2b39e1048..cf15c5130428 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -477,10 +477,6 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 	if (!chbs->base)
 		return 0;
 
-	if (chbs->cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11 &&
-	    chbs->length != CXL_RCRB_SIZE)
-		return 0;
-
 	ctx->base = chbs->base;
 
 	return 0;
-- 
2.43.0

[RFC PATCH v2 3/3] lspci: Add function to display cxl1.1 device link status

[Kobayashi,Daisuke]

This patch adds a function to output the link status of the CXL1.1 device
when it is connected.

In CXL1.1, the link status of the device is included in the RCRB mapped to
the memory mapped register area. The value of that register is outputted
to sysfs, and based on that, displays the link status information.


Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 ls-caps.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/ls-caps.c b/ls-caps.c
index 1b63262..5c60321 100644
--- a/ls-caps.c
+++ b/ls-caps.c
@@ -10,6 +10,8 @@
 
 #include <stdio.h>
 #include <string.h>
+#include <fcntl.h>
+#include <stdlib.h>
 
 #include "lspci.h"
 
@@ -1381,6 +1383,97 @@ static void cap_express_slot2(struct device *d UNUSED, int where UNUSED)
   /* No capabilities that require this field in PCIe rev2.0 spec. */
 }
 
+#define OBJNAMELEN 1024
+#define OBJBUFSIZE 64
+static int
+get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result)
+{
+  char namebuf[OBJNAMELEN];
+  int n = snprintf(namebuf, OBJNAMELEN, "%s/devices/%04x:%02x:%02x.%d/%s",
+		   pci_get_param(d->access, "sysfs.path"),
+       d->domain, d->bus, d->dev, d->func, object);
+  if (n < 0 || n >= OBJNAMELEN){
+    d->access->error("Failed to get filename");
+    return -1;
+    }
+  int fd = open(namebuf, O_RDONLY);
+  if(fd < 0)
+    return -1;
+  n = read(fd, result, OBJBUFSIZE);
+  if (n < 0 || n >= OBJBUFSIZE){
+    d->access->error("Failed to read the file");
+    return -1;
+  }
+  return 0;
+}
+
+static void cap_express_link_rcd(struct device *d){
+  u32 t, aspm, cap_speed, cap_width, sta_speed, sta_width;
+  u16 w;
+  struct pci_dev *pdev = d->dev;
+  char buf[OBJBUFSIZE];
+
+  if(get_rcd_sysfs_obj_file(pdev, "rcd_link_cap", buf))
+    return;
+  t = (u32)strtoul(buf, NULL, 16);
+
+  aspm = (t & PCI_EXP_LNKCAP_ASPM) >> 10;
+  cap_speed = t & PCI_EXP_LNKCAP_SPEED;
+  cap_width = (t & PCI_EXP_LNKCAP_WIDTH) >> 4;
+  printf("\t\tLnkCap:\tPort #%d, Speed %s, Width x%d, ASPM %s",
+	t >> 24,
+	link_speed(cap_speed), cap_width,
+	aspm_support(aspm));
+  if (aspm)
+    {
+      printf(", Exit Latency ");
+      if (aspm & 1)
+	printf("L0s %s", latency_l0s((t & PCI_EXP_LNKCAP_L0S) >> 12));
+      if (aspm & 2)
+	printf("%sL1 %s", (aspm & 1) ? ", " : "",
+	    latency_l1((t & PCI_EXP_LNKCAP_L1) >> 15));
+    }
+  printf("\n");
+  printf("\t\t\tClockPM%c Surprise%c LLActRep%c BwNot%c ASPMOptComp%c\n",
+	FLAG(t, PCI_EXP_LNKCAP_CLOCKPM),
+	FLAG(t, PCI_EXP_LNKCAP_SURPRISE),
+	FLAG(t, PCI_EXP_LNKCAP_DLLA),
+	FLAG(t, PCI_EXP_LNKCAP_LBNC),
+	FLAG(t, PCI_EXP_LNKCAP_AOC));
+
+  if(!get_rcd_sysfs_obj_file(pdev, "rcd_link_ctrl", buf)){
+    w = (u16)strtoul(buf, NULL, 16);    
+    printf("\t\tLnkCtl:\tASPM %s;", aspm_enabled(w & PCI_EXP_LNKCTL_ASPM));
+    printf(" Disabled%c CommClk%c\n\t\t\tExtSynch%c ClockPM%c AutWidDis%c BWInt%c AutBWInt%c\n",
+    FLAG(w, PCI_EXP_LNKCTL_DISABLE),
+    FLAG(w, PCI_EXP_LNKCTL_CLOCK),
+    FLAG(w, PCI_EXP_LNKCTL_XSYNCH),
+    FLAG(w, PCI_EXP_LNKCTL_CLOCKPM),
+    FLAG(w, PCI_EXP_LNKCTL_HWAUTWD),
+    FLAG(w, PCI_EXP_LNKCTL_BWMIE),
+    FLAG(w, PCI_EXP_LNKCTL_AUTBWIE));
+  }
+
+  if(!get_rcd_sysfs_obj_file(pdev, "rcd_link_status", buf)){
+    w = (u16)strtoul(buf, NULL, 16);    
+    sta_speed = w & PCI_EXP_LNKSTA_SPEED;
+    sta_width = (w & PCI_EXP_LNKSTA_WIDTH) >> 4;
+    printf("\t\tLnkSta:\tSpeed %s%s, Width x%d%s\n",
+    link_speed(sta_speed),
+    link_compare(PCI_EXP_TYPE_ROOT_INT_EP, sta_speed, cap_speed),
+    sta_width,
+    link_compare(PCI_EXP_TYPE_ROOT_INT_EP, sta_width, cap_width));
+    printf("\t\t\tTrErr%c Train%c SlotClk%c DLActive%c BWMgmt%c ABWMgmt%c\n",
+    FLAG(w, PCI_EXP_LNKSTA_TR_ERR),
+    FLAG(w, PCI_EXP_LNKSTA_TRAIN),
+    FLAG(w, PCI_EXP_LNKSTA_SL_CLK),
+    FLAG(w, PCI_EXP_LNKSTA_DL_ACT),
+    FLAG(w, PCI_EXP_LNKSTA_BWMGMT),
+    FLAG(w, PCI_EXP_LNKSTA_AUTBW));
+  }
+  return;
+}
+
 static int
 cap_express(struct device *d, int where, int cap)
 {
@@ -1445,6 +1538,9 @@ cap_express(struct device *d, int where, int cap)
   cap_express_dev(d, where, type);
   if (link)
     cap_express_link(d, where, type);
+  else if (type == PCI_EXP_TYPE_ROOT_INT_EP)
+    cap_express_link_rcd(d);
+   
   if (slot)
     cap_express_slot(d, where);
   if (type == PCI_EXP_TYPE_ROOT_PORT || type == PCI_EXP_TYPE_ROOT_EC)
-- 
2.43.0

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

[Bjorn Helgaas]

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

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

[Daisuke Kobayashi (Fujitsu)]

> -----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.

> > +	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.

Re: [RFC PATCH v2 3/3] lspci: Add function to display cxl1.1 device link status

[Martin Mareš]

Hello!

> This patch adds a function to output the link status of the CXL1.1 device
> when it is connected.
> 
> In CXL1.1, the link status of the device is included in the RCRB mapped to
> the memory mapped register area. The value of that register is outputted
> to sysfs, and based on that, displays the link status information.

I like using sysfs to access the RCRB, but since it is specific to Linux,
you cannot do it in ls-caps.c (everything in this file is platform-independent).

The right way is to extend libpci and its interface to platform-specific
back-ends to provide this functionality. However, I am not sure that we should
add special functions just for this purpose. I will think of something more
general and let you know soon.

Otherwise, please follow the coding style of the rest of the file.

				Martin

RE: [RFC PATCH v2 3/3] lspci: Add function to display cxl1.1 device link status

[Daisuke Kobayashi (Fujitsu)]

> -----Original Message-----
> From: Martin Mareš <mj@ucw.cz>
> Sent: Wednesday, February 28, 2024 5:35 PM
> 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; dan.j.williams@intel.com
> Subject: Re: [RFC PATCH v2 3/3] lspci: Add function to display cxl1.1 device
> link status
> 
> Hello!
> 
> > This patch adds a function to output the link status of the CXL1.1
> > device when it is connected.
> >
> > In CXL1.1, the link status of the device is included in the RCRB
> > mapped to the memory mapped register area. The value of that register
> > is outputted to sysfs, and based on that, displays the link status information.
> 
> I like using sysfs to access the RCRB, but since it is specific to Linux, you
> cannot do it in ls-caps.c (everything in this file is platform-independent).
> 
> The right way is to extend libpci and its interface to platform-specific
> back-ends to provide this functionality. However, I am not sure that we should
> add special functions just for this purpose. I will think of something more
> general and let you know soon.
> 
> Otherwise, please follow the coding style of the rest of the file.
> 
> 				Martin

Thank you for your comment.
I understand that ls-caps.c requires a platform-independent implementation.
I will look for an appropriate place to implement that.
Is there a good place to add code to read files output to sysfs?
For example, is it practical to add a function to lib/sysfs.c that reads and uses a specific sysfs file?

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

[Dan Williams]

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,
        },
 };