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

[PATCH v3 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.

v2[2] -> v3:
- Fix unnecessary initialization and wrong types (Bjohn).
- Create a helper function for getting a PCIe capability offset (Bjohn).
- Move platform-specific implementation to the lib directory in pciutils (Martin).

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

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

 drivers/cxl/acpi.c |   4 -
 drivers/cxl/pci.c  | 193 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 193 insertions(+), 4 deletions(-)

  Add function to display cxl1.1 device link status

 lib/access.c | 29 +++++++++++++++++++++
 lib/pci.h    |  2 ++
 ls-caps.c    | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)
-- 
2.43.0

[PATCH v3 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 | 193 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 193 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4fd1f207c84e..8f66f80a7bdc 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -781,6 +781,195 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
 	return 0;
 }
 
+static u8 cxl_rcrb_get_pcie_cap_offset(void __iomem *addr){
+	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;
+
+}
+
+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 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;
+	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;
+}
+
+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;
+	u16 linksta;
+
+	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;
+
+	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)
 {
 	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
@@ -806,6 +995,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 +1159,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

[PATCH v3 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

[PATCH v3 3/3] Add function to display cxl1.1 device link status

[Kobayashi,Daisuke]

From: KobayashiDaisuke <kobayashi.da-06@fujitsu.com>

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: "KobayashiDaisuke" <kobayashi.da-06@fujitsu.com>
---
 lib/access.c | 29 +++++++++++++++++++++
 lib/pci.h    |  2 ++
 ls-caps.c    | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)

diff --git a/lib/access.c b/lib/access.c
index 7d66123..bc75a84 100644
--- a/lib/access.c
+++ b/lib/access.c
@@ -12,6 +12,7 @@
 #include <stdlib.h>
 #include <stdarg.h>
 #include <string.h>
+#include <fcntl.h>
 
 #include "internal.h"
 
@@ -268,3 +269,31 @@ pci_get_string_property(struct pci_dev *d, u32 prop)
 
   return NULL;
 }
+
+#define OBJNAMELEN 1024
+#define OBJBUFSIZE 64
+int
+get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result)
+{
+#ifdef PCI_HAVE_PM_LINUX_SYSFS
+  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;
+#else
+  return -1;
+#endif
+}
diff --git a/lib/pci.h b/lib/pci.h
index 2322bf7..fb25069 100644
--- a/lib/pci.h
+++ b/lib/pci.h
@@ -187,6 +187,8 @@ int pci_write_long(struct pci_dev *, int pos, u32 data) PCI_ABI;
 int pci_read_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI;
 int pci_write_block(struct pci_dev *, int pos, u8 *buf, int len) PCI_ABI;
 
+int get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result) PCI_ABI;
+
 /*
  * Most device properties take some effort to obtain, so libpci does not
  * initialize them during default bus scan. Instead, you have to call
diff --git a/ls-caps.c b/ls-caps.c
index 1b63262..12c9f34 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,74 @@ static void cap_express_slot2(struct device *d UNUSED, int where UNUSED)
   /* No capabilities that require this field in PCIe rev2.0 spec. */
 }
 
+#define OBJBUFSIZE 64
+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 +1515,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: [PATCH v3 0/3] Display cxl1.1 device link status

[Daisuke Kobayashi (Fujitsu)]

Kindly ping

Any comment is welcome.

On Tue, March 12, 2024, Kobayashi,Daisuke wrote:
> -----Original Message-----
> From: Kobayashi,Daisuke <kobayashi.da-06@fujitsu.com>
> Sent: Tuesday, March 12, 2024 5:06 PM
> To: Kobayashi, Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com>;
> linux-cxl@vger.kernel.org
> Cc: Gotou, Yasunori/五島 康文 <y-goto@fujitsu.com>;
> linux-pci@vger.kernel.org; mj@ucw.cz; dan.j.williams@intel.com; Kobayashi,
> Daisuke/小林 大介 <kobayashi.da-06@fujitsu.com>
> Subject: [PATCH v3 0/3] Display cxl1.1 device link status
> 
> 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.
> 
> v2[2] -> v3:
> - Fix unnecessary initialization and wrong types (Bjohn).
> - Create a helper function for getting a PCIe capability offset (Bjohn).
> - Move platform-specific implementation to the lib directory in pciutils
> (Martin).
> 
> [1]
> https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06
> @fujitsu.com/
> [2]
> https://lore.kernel.org/linux-cxl/20240227083313.87699-1-kobayashi.da-06@
> fujitsu.com/
> 
> Kobayashi,Daisuke (3):
>   Add sysfs attribute for CXL 1.1 device link status
>   Remove conditional branch that is not suitable for cxl1.1 devices
> 
>  drivers/cxl/acpi.c |   4 -
>  drivers/cxl/pci.c  | 193
> +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 193 insertions(+), 4 deletions(-)
> 
>   Add function to display cxl1.1 device link status
> 
>  lib/access.c | 29 +++++++++++++++++++++
>  lib/pci.h    |  2 ++
>  ls-caps.c    | 73
> ++++++++++++++++++++++++++++++++++++++++++++++++
> ++++
>  3 files changed, 104 insertions(+)
> --
> 2.43.0

Re: [PATCH v3 0/3] Display cxl1.1 device link status

[Dan Williams]

Kobayashi,Daisuke wrote:
> Hello.
> 
> This patch series adds a feature that displays the link status
> of the CXL1.1 device.

Good write up! One minor feedback going forward is to drop usage of
"This patch" in a description as that is self evident.

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

One common way to rewrite a "This patch..." sentence is in "imperative"
tense, so for example:

"Solve these issues with sysfs attributes to expose the status
registers hidden in the RCRB."

...i.e. write the commit log as if the patch is commanding the code to
change. You can find some more notes about this here:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

...but really, this cover letter is higher quality than most, so thanks
for that!

> 
> 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.
> 
> v2[2] -> v3:
> - Fix unnecessary initialization and wrong types (Bjohn).
> - Create a helper function for getting a PCIe capability offset (Bjohn).
> - Move platform-specific implementation to the lib directory in pciutils (Martin).
> 
> [1]
> https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06@fujitsu.com/
> [2]
> https://lore.kernel.org/linux-cxl/20240227083313.87699-1-kobayashi.da-06@fujitsu.com/
> 
> Kobayashi,Daisuke (3):
>   Add sysfs attribute for CXL 1.1 device link status
>   Remove conditional branch that is not suitable for cxl1.1 devices
> 
>  drivers/cxl/acpi.c |   4 -
>  drivers/cxl/pci.c  | 193 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 193 insertions(+), 4 deletions(-)
> 
>   Add function to display cxl1.1 device link status
> 
>  lib/access.c | 29 +++++++++++++++++++++
>  lib/pci.h    |  2 ++
>  ls-caps.c    | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)

The typical way I handle cases where I am updating the kernel side and
the user tooling side of a problem is to post the kernel changes and
then separately post the user changes with a lore link to the kernel
submission.

For PCI utils I believe you will need to send a separate pull request
via github once the kernel changes are accepted.

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

[Dan Williams]

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.

Per the comments on the cover letter I would rewrite this as:

---
In CXL1.1, the link status of the device is included in the RCRB mapped to
the memory mapped register area. Critically, that arrangement makes the link
status and control registers invisible to existing PCI user tooling.

Export those registers via sysfs with the expectation that PCI user
tooling will alternatively look for these sysfs files when attempting to
access these registers on CXL 1.1 endpoints.
---

> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/pci.c | 193 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 193 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..8f66f80a7bdc 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -781,6 +781,195 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	return 0;
>  }
>  
> +static u8 cxl_rcrb_get_pcie_cap_offset(void __iomem *addr){
> +	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;

The location is static, so there should be no need to lookup the
location every time the sysfs attribute is accessed. I also think the
values are static unless the link is reset. So my expectation is that
these register values can just be read once and cached.

Likely the best place to do this is inside __rcrb_to_component(). That
routine already has the RCRB mapped and can be refactored to collect the
the link status registers. Something like, rename __rcrb_to_component()
to __rcrb_to_regs() and then have it fill in an updated cxl_rcrb_info():

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 534e25e2f0a4..16c7472877b7 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -651,7 +651,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
 
 struct cxl_rcrb_info {
        resource_size_t base;
+       resource_size_t component_reg;
+       resource_size_t rcd_component_reg;
        u16 aer_cap;
+       u16 rcd_lnkctrl;
+       u16 rcd_lnkstatus;
+       u32 rcd_lnkcap;
 };
 
 /**

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

Why is this a WARN_ON_ONCE()? In other words the caller should know
ahead of time whether it has a valid RCRB base or not.

...oh, I see this is copying cxl_rcrb_to_aer(). I think that
WARN_ON_ONCE() in that function is bogus as well.


> +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> +		return 0;

This is awkward because it may collide with usages of the RCRB, so that
is another reason to cache the values.

> +
> +	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 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);

This and the other ones should be using "%#x\n" so that the format of
the number base is included.

> +}
> +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;
> +	u16 linkctrl;
> +
> +	if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> +		return 0;
> +
> +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> +		return 0;

...the other benefit of centralizing this code is that we do not end up
with multiple copies of similar, but slightly different code.

> +
> +	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;
> +}
> +
> +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;
> +	u16 linksta;
> +
> +	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;
> +
> +	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)
>  {
>  	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
> @@ -806,6 +995,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);

No need to manually call device_create_file() when the attribute group
is already registered below. I am surprised you did not get duplicate
sysfs file warnings when registering these files twice.

>  	pci_set_drvdata(pdev, cxlds);
>  
>  	cxlds->rcd = is_cxl_restricted(pdev);
> @@ -967,6 +1159,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
> 
> 

Re: [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1 devices

[Dan Williams]

Kobayashi,Daisuke wrote:
> 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.

No, in Table 9-21 in CXL 3.1:

Base: If CXL Version = 0000 0000h, this represents the base address of
the RCH Downstream Port RCRB

Length: If CXL Version = 0000 0000h, this field must be set to 8 KB
(2000h)

The size of the CHBS structure is always much less than 8KB.

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

[Dan Williams]

Kobayashi,Daisuke wrote:
> From: KobayashiDaisuke <kobayashi.da-06@fujitsu.com>
> 
> 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: "KobayashiDaisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  lib/access.c | 29 +++++++++++++++++++++
>  lib/pci.h    |  2 ++
>  ls-caps.c    | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)

Including a userspace patch in a kernel patch submission breaks kernel
patch management tools like b4 shazam:

---
$ b4 shazam 20240312080559.14904-1-kobayashi.da-06@fujitsu.com
Grabbing thread from lore.kernel.org/all/20240312080559.14904-1-kobayashi.da-06@fujitsu.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 8 messages in the thread
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status
  ✓ [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1 devices
  ✓ [PATCH v3 3/3] Add function to display cxl1.1 device link status
  ---
  ✓ Signed: DKIM/fujitsu.com
---
Total patches: 3
---
Applying: Add sysfs attribute for CXL 1.1 device link status
Applying: Remove conditional branch that is not suitable for cxl1.1 devices
Applying: Add function to display cxl1.1 device link status
Patch failed at 0003 Add function to display cxl1.1 device link status
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
/home/dwillia2/git/linux/.git/rebase-apply/patch:117: trailing whitespace.
    w = (u16)strtoul(buf, NULL, 16);    
/home/dwillia2/git/linux/.git/rebase-apply/patch:130: trailing whitespace.
    w = (u16)strtoul(buf, NULL, 16);    
/home/dwillia2/git/linux/.git/rebase-apply/patch:158: trailing whitespace.
   
error: lib/access.c: does not exist in index
error: lib/pci.h: does not exist in index
error: ls-caps.c: does not exist in index
hint: Use 'git am --show-current-patch=diff' to see the failed patch
---

...and this patch should wait until the kernel change is accepted before
being submitted directly to the PCI utils project which does not watch
this linux-cxl mailing list.

RE: [PATCH v3 0/3] Display cxl1.1 device link status

[Daisuke Kobayashi (Fujitsu)]

Dan Williams wrote:
> Kobayashi,Daisuke wrote:
> > Hello.
> >
> > This patch series adds a feature that displays the link status
> > of the CXL1.1 device.
> 
> Good write up! One minor feedback going forward is to drop usage of
> "This patch" in a description as that is self evident.
> 
> > 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.
> 
> One common way to rewrite a "This patch..." sentence is in "imperative"
> tense, so for example:
> 
> "Solve these issues with sysfs attributes to expose the status
> registers hidden in the RCRB."
> 
> ...i.e. write the commit log as if the patch is commanding the code to
> change. You can find some more notes about this here:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-
> submission-notes
> 
> ...but really, this cover letter is higher quality than most, so thanks
> for that!
> 

Thank you for your kind feedback.
In the updated patch, I will revise the text and submit it.

> > Kobayashi,Daisuke (3):
> >   Add sysfs attribute for CXL 1.1 device link status
> >   Remove conditional branch that is not suitable for cxl1.1 devices
> >
> >  drivers/cxl/acpi.c |   4 -
> >  drivers/cxl/pci.c  | 193
> +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 193 insertions(+), 4 deletions(-)
> >
> >   Add function to display cxl1.1 device link status
> >
> >  lib/access.c | 29 +++++++++++++++++++++
> >  lib/pci.h    |  2 ++
> >  ls-caps.c    | 73
> ++++++++++++++++++++++++++++++++++++++++++++++++
> ++++
> >  3 files changed, 104 insertions(+)
> 
> The typical way I handle cases where I am updating the kernel side and
> the user tooling side of a problem is to post the kernel changes and
> then separately post the user changes with a lore link to the kernel
> submission.
> 
> For PCI utils I believe you will need to send a separate pull request
> via github once the kernel changes are accepted.

I understand that it’s better to post separately. 
After the changes to the kernel are accepted, I will submit a patch to pciutils.

RE: [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1 devices

[Daisuke Kobayashi (Fujitsu)]

Dan Williams wrote:
> Kobayashi,Daisuke wrote:
> > 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.
> 
> No, in Table 9-21 in CXL 3.1:
> 
> Base: If CXL Version = 0000 0000h, this represents the base address of the
> RCH Downstream Port RCRB
> 
> Length: If CXL Version = 0000 0000h, this field must be set to 8 KB
> (2000h)
> 
> The size of the CHBS structure is always much less than 8KB.

Thank you for your feedback. 
After revisiting the specifications, I realized that my understanding was incorrect. 
Therefore, in the next patch, I will revert this change.

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

[Daisuke Kobayashi (Fujitsu)]

Dan Williams wrote:
> Kobayashi,Daisuke wrote:
> > From: KobayashiDaisuke <kobayashi.da-06@fujitsu.com>
> >
> > 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: "KobayashiDaisuke" <kobayashi.da-06@fujitsu.com>
> > ---
> >  lib/access.c | 29 +++++++++++++++++++++
> >  lib/pci.h    |  2 ++
> >  ls-caps.c    | 73
> ++++++++++++++++++++++++++++++++++++++++++++++++
> ++++
> >  3 files changed, 104 insertions(+)
> 
> Including a userspace patch in a kernel patch submission breaks kernel patch
> management tools like b4 shazam:
> 
> ---
> $ b4 shazam 20240312080559.14904-1-kobayashi.da-06@fujitsu.com
> Grabbing thread from
> lore.kernel.org/all/20240312080559.14904-1-kobayashi.da-06@fujitsu.com/t.
> mbox.gz
> Checking for newer revisions
> Grabbing search results from lore.kernel.org Analyzing 8 messages in the
> thread Checking attestation on all messages, may take a moment...
> ---
>   ✓ [PATCH v3 1/3] Add sysfs attribute for CXL 1.1 device link status
>   ✓ [PATCH v3 2/3] Remove conditional branch that is not suitable for cxl1.1
> devices
>   ✓ [PATCH v3 3/3] Add function to display cxl1.1 device link status
>   ---
>   ✓ Signed: DKIM/fujitsu.com
> ---
> Total patches: 3
> ---
> Applying: Add sysfs attribute for CXL 1.1 device link status
> Applying: Remove conditional branch that is not suitable for cxl1.1 devices
> Applying: Add function to display cxl1.1 device link status Patch failed at 0003
> Add function to display cxl1.1 device link status When you have resolved this
> problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> /home/dwillia2/git/linux/.git/rebase-apply/patch:117: trailing whitespace.
>     w = (u16)strtoul(buf, NULL, 16);
> /home/dwillia2/git/linux/.git/rebase-apply/patch:130: trailing whitespace.
>     w = (u16)strtoul(buf, NULL, 16);
> /home/dwillia2/git/linux/.git/rebase-apply/patch:158: trailing whitespace.
> 
> error: lib/access.c: does not exist in index
> error: lib/pci.h: does not exist in index
> error: ls-caps.c: does not exist in index
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> ---
> 
> ...and this patch should wait until the kernel change is accepted before being
> submitted directly to the PCI utils project which does not watch this linux-cxl
> mailing list.

Thank you! I will submit the patch for pciutils separately.

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

[Daisuke Kobayashi (Fujitsu)]

Dan Williams wrote:
> 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.
> 
> Per the comments on the cover letter I would rewrite this as:
> 
> ---
> In CXL1.1, the link status of the device is included in the RCRB mapped to the
> memory mapped register area. Critically, that arrangement makes the link
> status and control registers invisible to existing PCI user tooling.
> 
> Export those registers via sysfs with the expectation that PCI user tooling will
> alternatively look for these sysfs files when attempting to access these
> registers on CXL 1.1 endpoints.
> ---
> 

This message will be updated in the next patch.
Thank you for your helpful feedback.

> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > ---
> >  drivers/cxl/pci.c | 193
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 193 insertions(+)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> > 4fd1f207c84e..8f66f80a7bdc 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -781,6 +781,195 @@ static int cxl_event_config(struct pci_host_bridge
> *host_bridge,
> >  	return 0;
> >  }
> >
> > +static u8 cxl_rcrb_get_pcie_cap_offset(void __iomem *addr){
> > +	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;
> 
> The location is static, so there should be no need to lookup the location every
> time the sysfs attribute is accessed. I also think the values are static unless the
> link is reset. So my expectation is that these register values can just be read
> once and cached.
> 
> Likely the best place to do this is inside __rcrb_to_component(). That routine
> already has the RCRB mapped and can be refactored to collect the the link
> status registers. Something like, rename __rcrb_to_component() to
> __rcrb_to_regs() and then have it fill in an updated cxl_rcrb_info():
> 

Add processing to__rcrb_to_component() to change the implementation
to cache these values. As you say, I think these values are static, 
so it's not efficient to access the RCRB every time you access the sysfs attribute.

> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index
> 534e25e2f0a4..16c7472877b7 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -651,7 +651,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const
> struct device *dport_dev)
> 
>  struct cxl_rcrb_info {
>         resource_size_t base;
> +       resource_size_t component_reg;
> +       resource_size_t rcd_component_reg;
>         u16 aer_cap;
> +       u16 rcd_lnkctrl;
> +       u16 rcd_lnkstatus;
> +       u32 rcd_lnkcap;
>  };
> 
>  /**
> 
> > +
> > +}
> > +
> > +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;
> 
> Why is this a WARN_ON_ONCE()? In other words the caller should know ahead
> of time whether it has a valid RCRB base or not.
> 
> ...oh, I see this is copying cxl_rcrb_to_aer(). I think that
> WARN_ON_ONCE() in that function is bogus as well.
> 
> 

Yes, as you mentioned, I have copied cxl_rcrb_to_aer().
However, it seems to be an improper implementation.
In the next patch, I will modify the implementation to cache the value, 
and consequently, this part of the code will be removed.

> > +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> > +		return 0;
> 
> This is awkward because it may collide with usages of the RCRB, so that is
> another reason to cache the values.
> 
> > +
> > +	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 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);
> 
> This and the other ones should be using "%#x\n" so that the format of the
> number base is included.
> 

I will fix them. Thank you.

> > +}
> > +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;
> > +	u16 linkctrl;
> > +
> > +	if (WARN_ON_ONCE(rcrb == CXL_RESOURCE_NONE))
> > +		return 0;
> > +
> > +	if (!request_mem_region(rcrb, SZ_4K, dev_name(dev)))
> > +		return 0;
> 
> ...the other benefit of centralizing this code is that we do not end up with
> multiple copies of similar, but slightly different code.
> 

Are you saying that caching values simplifies the show function?
Then I think you're right. I will change that the value should be cached
in the same way as the component register.

> > +
> > +	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;
> > +}
> > +
> > +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;
> > +	u16 linksta;
> > +
> > +	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;
> > +
> > +	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)  {
> >  	struct pci_host_bridge *host_bridge =
> > pci_find_host_bridge(pdev->bus); @@ -806,6 +995,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);
> 
> No need to manually call device_create_file() when the attribute group is
> already registered below. I am surprised you did not get duplicate sysfs file
> warnings when registering these files twice.
> 

Thank you for pointing it out. Remove these calls.

> >  	pci_set_drvdata(pdev, cxlds);
> >
> >  	cxlds->rcd = is_cxl_restricted(pdev); @@ -967,6 +1159,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
> >
> >
> 

Re: [PATCH v3 3/3] 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.

> diff --git a/lib/access.c b/lib/access.c
> index 7d66123..bc75a84 100644
> --- a/lib/access.c
> +++ b/lib/access.c
[...]
> @@ -268,3 +269,31 @@ pci_get_string_property(struct pci_dev *d, u32 prop)
>  
>    return NULL;
>  }
> +
> +#define OBJNAMELEN 1024
> +#define OBJBUFSIZE 64
> +int
> +get_rcd_sysfs_obj_file(struct pci_dev *d, char *object, char *result)
> +{
> +#ifdef PCI_HAVE_PM_LINUX_SYSFS
> +  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;
> +#else
> +  return -1;
> +#endif
> +}

This really is not the right place to read from sysfs. The libpci should provide
a backend-indepenent interface for reading this information and the sysfs
back-end (lib/sysfs.c) should provide one implementation of this interface.

I think that we can easily extend pci_fill_info() and add another PCI_FILL_xxx
flag for CXL RCD properties, which will be available in struct pci_dev (beware
that new fields have to be added _after_ all public fields to keep ABI compatibility).

> @@ -1445,6 +1515,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)

Does it make sense to look up CXL RCD information for all PCIe devices of type
PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL
capability?

				Have a nice fortnight
-- 
Martin `MJ' Mareš                        <mj@ucw.cz>   http://mj.ucw.cz/
United Computer Wizards, Prague, Czech Republic, Europe, Earth, Universe

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

[Dan Williams]

Martin Mareš wrote:
[..]
> 
> This really is not the right place to read from sysfs. The libpci should provide
> a backend-indepenent interface for reading this information and the sysfs
> back-end (lib/sysfs.c) should provide one implementation of this interface.
> 
> I think that we can easily extend pci_fill_info() and add another PCI_FILL_xxx
> flag for CXL RCD properties, which will be available in struct pci_dev (beware
> that new fields have to be added _after_ all public fields to keep ABI compatibility).
> 
> > @@ -1445,6 +1515,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)
> 
> Does it make sense to look up CXL RCD information for all PCIe devices of type
> PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL
> capability?

I think so, would this fit more naturally in pci_scan_caps() with a new
scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't
the trouble that this needs a DVSEC scan for CXL to know it needs to go
back and fill in details that normally in appear in the base PCIe
capability scan?

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

[Martin Mareš]

Hello!

> > Does it make sense to look up CXL RCD information for all PCIe devices of type
> > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL
> > capability?
> 
> I think so, would this fit more naturally in pci_scan_caps() with a new
> scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't
> the trouble that this needs a DVSEC scan for CXL to know it needs to go
> back and fill in details that normally in appear in the base PCIe
> capability scan?

I would prefer to display all CXL stuff together (i.e., when showing the
DVSEC caps). Is there any reason not to?

					Martin

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

[Dan Williams]

Martin Mareš wrote:
> Hello!
> 
> > > Does it make sense to look up CXL RCD information for all PCIe devices of type
> > > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices with the CXL
> > > capability?
> > 
> > I think so, would this fit more naturally in pci_scan_caps() with a new
> > scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However, isn't
> > the trouble that this needs a DVSEC scan for CXL to know it needs to go
> > back and fill in details that normally in appear in the base PCIe
> > capability scan?
> 
> I would prefer to display all CXL stuff together (i.e., when showing the
> DVSEC caps). Is there any reason not to?

Together with the DVSEC caps display makes sense to me as well.

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

[Daisuke Kobayashi (Fujitsu)]

Martin Mareš wrote:
[..]
> 
> This really is not the right place to read from sysfs. The libpci 
> should provide a backend-indepenent interface for reading this 
> information and the sysfs back-end (lib/sysfs.c) should provide one implementation of this interface.
> 
> I think that we can easily extend pci_fill_info() and add another 
> PCI_FILL_xxx flag for CXL RCD properties, which will be available in 
> struct pci_dev (beware that new fields have to be added _after_ all public fields to keep ABI compatibility).
>

Thank you for your feedback.
I understand that lib/sysfs.c is the appropriate location. 
I also understand that to extend it, I need to add code in pci_fill_info() 
to read the sysfs file and add a new flag at the bottom of PCI_FILL_xx. 
Thank you for the clarification. I will update this in the next patch and 
submit it separately from the driver implementation.

Dan Williams wrote:
> Martin Mareš wrote:
> > Hello!
> >
> > > > Does it make sense to look up CXL RCD information for all PCIe devices
> of type
> > > > PCI_EXP_TYPE_ROOT_INT_EP? Shouldn't it be done only for devices
> with the CXL
> > > > capability?
> > >
> > > I think so, would this fit more naturally in pci_scan_caps() with a new
> > > scan for DVSEC caps ("PCI_EXT_CAP_ID_DVSEC" in Linux). However,
> isn't
> > > the trouble that this needs a DVSEC scan for CXL to know it needs to go
> > > back and fill in details that normally in appear in the base PCIe
> > > capability scan?
> >
> > I would prefer to display all CXL stuff together (i.e., when showing the
> > DVSEC caps). Is there any reason not to?
> 
> Together with the DVSEC caps display makes sense to me as well.

Here is my understanding of this point. Please let me know if there is a more appropriate approach.

Based on my understanding, the information we are trying to display 
with this feature is included in the PCIe Capability. Therefore, I believe 
it is appropriate to display it within the cap_express() function. By checking the DVSEC, 
we can determine whether this device is a CXL device or not. However, 
I couldn't find a proper way to check the DVSEC within the cap_express() function.
As a result, I explored other options and set the PCI_EXP_TYPE_ROOT_INT_EP
as a branching condition.(cxl3.0 specification 7.3.1.2, 9.10)
Furthermore, since rcd is identified as PCI_EXP_TYPE_ROOT_INT_EP by
PCI_EXP_FLAGS_TYPE, cap_express_link() is not called.

Alternatively, I might be able to create a new rcd variable for pci_fill_info()
in struct pci_dev and make it a branch condition.

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

[Daisuke Kobayashi (Fujitsu)]

> Dan Williams wrote:
> > Kobayashi,Daisuke wrote:
> > > +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)  {
> > >  	struct pci_host_bridge *host_bridge =
> > > pci_find_host_bridge(pdev->bus); @@ -806,6 +995,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);
> >
> > No need to manually call device_create_file() when the attribute group is
> > already registered below. I am surprised you did not get duplicate sysfs file
> > warnings when registering these files twice.
> >
> 
> Thank you for pointing it out. Remove these calls.
> 
If you are aware of the cause, I would appreciate your insight. 
In my environment, when I removed this device_create_file(), 
the file was not generated in sysfs. Therefore, I have not been 
able to remove this manual procedure at the moment. Is there a 
possibility that simply registering with 
struct pci_driver.driver.groups will not generate a sysfs file?

> > >  	pci_set_drvdata(pdev, cxlds);
> > >
> > >  	cxlds->rcd = is_cxl_restricted(pdev); @@ -967,6 +1159,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
> > >
> > >
> >
> 

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

[Daisuke Kobayashi (Fujitsu)]

Kobayashi,Daisuke wrote:
> > Dan Williams wrote:
> > > Kobayashi,Daisuke wrote:
> > > > +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)  {
> > > >  	struct pci_host_bridge *host_bridge =
> > > > pci_find_host_bridge(pdev->bus); @@ -806,6 +995,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);
> > >
> > > No need to manually call device_create_file() when the attribute group is
> > > already registered below. I am surprised you did not get duplicate sysfs file
> > > warnings when registering these files twice.
> > >
> >
> > Thank you for pointing it out. Remove these calls.
> >
> If you are aware of the cause, I would appreciate your insight.
> In my environment, when I removed this device_create_file(),
> the file was not generated in sysfs. Therefore, I have not been
> able to remove this manual procedure at the moment. Is there a
> possibility that simply registering with
> struct pci_driver.driver.groups will not generate a sysfs file?
> 

I would like to report on some additional findings.
The process of registering cxl_rcd_groups to struct pci_driver.driver.dev_groups seems
to not generate a file in sysfs when looking at the contents of the module_pci_driver() macro.

For this feature, I think it would be best to output the values to a directory
of /sys/bus/pci/devices/<pci-addr>/. To output to this directory, the attribute would
need to be registered to pci_dev.dev.
My current understanding is that the best way to do this would be to register the attribute
with device_add_groups(&pdev->dev, cxl_rcd_groups) on probe and remove the files with
device_remove_groups(&pdev->dev, cxl_rcd_groups) on remove.
I have attached the code below. Is my usage of probe/remove correct? If so,
I will update and resubmit the patch with the following code.

 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
        struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
@@ -812,6 +885,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;
+       rc = device_add_groups(&pdev->dev, cxl_rcd_groups);
+       if (rc)
+               dev_warn(&pdev->dev, "Couldn't make rcd_groups (%d)\n", rc);
        pci_set_drvdata(pdev, cxlds);
 
        cxlds->rcd = is_cxl_restricted(pdev);
@@ -964,10 +1040,18 @@ static const struct pci_error_handlers cxl_error_handlers = {
        .cor_error_detected     = cxl_cor_error_detected,
 };
 
+static void cxl_pci_remove(struct pci_dev *pdev)
+{
+       if (is_cxl_restricted(pdev))
+               device_remove_groups(&pdev->dev, cxl_rcd_groups);
+}
+
 static struct pci_driver cxl_pci_driver = {
        .name                   = KBUILD_MODNAME,
        .id_table               = cxl_mem_pci_tbl,
        .probe                  = cxl_pci_probe,
+       .remove                 = cxl_pci_remove,
        .err_handler            = &cxl_error_handlers,
        .driver = {
                .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
--

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

[Jonathan Cameron]

On Wed, 3 Apr 2024 09:40:27 +0000
"Daisuke Kobayashi (Fujitsu)" <kobayashi.da-06@fujitsu.com> wrote:

> > Dan Williams wrote:  
> > > Kobayashi,Daisuke wrote:  
> > > > +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)  {
> > > >  	struct pci_host_bridge *host_bridge =
> > > > pci_find_host_bridge(pdev->bus); @@ -806,6 +995,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);  
> > >
> > > No need to manually call device_create_file() when the attribute group is
> > > already registered below. I am surprised you did not get duplicate sysfs file
> > > warnings when registering these files twice.
> > >  
> > 
> > Thank you for pointing it out. Remove these calls.
> >   
> If you are aware of the cause, I would appreciate your insight. 
> In my environment, when I removed this device_create_file(), 
> the file was not generated in sysfs. Therefore, I have not been 
> able to remove this manual procedure at the moment. Is there a 
> possibility that simply registering with 
> struct pci_driver.driver.groups will not generate a sysfs file?
> 
> > > >  	pci_set_drvdata(pdev, cxlds);
> > > >
> > > >  	cxlds->rcd = is_cxl_restricted(pdev); @@ -967,6 +1159,7 @@ static
> > > > struct pci_driver cxl_pci_driver = {
> > > >  	.err_handler		= &cxl_error_handlers,
> > > >  	.driver	= {
> > > >  		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
> > > > +		.dev_groups	= cxl_rcd_groups,
Odd though it may seem, try setting
.dev_groups in the outer structure not the inner one.

		.err_handler = &....
		.dev_groups = cxl_rcd_groups,
		.driver = {
			...
		},
Similar to:
https://elixir.bootlin.com/linux/latest/source/drivers/crypto/ccp/sp-pci.c#L592

For reasons I don't follow, __pci_register_driver() overrides the internal one.
Some ancient bit of code migration that never finished?
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-driver.c#L1447

I did some digging.
https://lore.kernel.org/all/20190731124349.4474-2-gregkh@linuxfoundation.org/

So this got added to the driver core fairly recently (only 4 years ago ;)

The the dev_groups was added to pci in
https://lore.kernel.org/all/20210512142648.666476-8-andrey.grodzovsky@amd.com/

I'm not sure why the bounce via pci_driver is needed though.
Greg, looks like this came from usb originally, can you recall the reasoning?

> > > >  	},
> > > >  };
> > > >
> > > > --
> > > > 2.43.0
> > > >
> > > >  
> > >  
> >   
> 
> 

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

[Dan Williams]

Daisuke Kobayashi (Fujitsu) wrote:
[..]
> > Thank you for pointing it out. Remove these calls.
> > 
> If you are aware of the cause, I would appreciate your insight. 
> In my environment, when I removed this device_create_file(), 
> the file was not generated in sysfs. Therefore, I have not been 
> able to remove this manual procedure at the moment. Is there a 
> possibility that simply registering with 
> struct pci_driver.driver.groups will not generate a sysfs file?

Be careful, are you assigning cxl_rcd_groups to
"pci_driver.driver.groups", or "pci_driver.driver.dev_groups"?
"dev_groups" adds them to the PCI device object, "groups" adds them to
the *driver* object (/sys/bus/pci/drivers/cxl_pci/$files).

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

[Dan Williams]

Daisuke Kobayashi (Fujitsu) wrote:
[..]
> I would like to report on some additional findings.
> The process of registering cxl_rcd_groups to struct pci_driver.driver.dev_groups seems
> to not generate a file in sysfs when looking at the contents of the module_pci_driver() macro.

Oh, apologies, and thanks for taking a deeper look. The failure is
because my suggested example led you astray. I suggested this:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..eec04f103aa8 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -971,6 +971,7 @@ static struct pci_driver cxl_pci_driver = {
        .err_handler            = &cxl_error_handlers,
        .driver = {
                .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
+               .dev_groups     = cxl_rcd_groups,
        },
 }; 


...the correct place to put it is here:

@@ -969,6 +969,7 @@ static struct pci_driver cxl_pci_driver = {
        .id_table               = cxl_mem_pci_tbl,
        .probe                  = cxl_pci_probe,
        .err_handler            = &cxl_error_handlers,
+       .dev_groups             = cxl_rcd_groups,
        .driver = {
                .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
        },


...otherwise __pci_register_driver() will overwrite it. This is a subtle
bug given probe_type is directly initialized in .driver.

> For this feature, I think it would be best to output the values to a directory
> of /sys/bus/pci/devices/<pci-addr>/. To output to this directory, the attribute would
> need to be registered to pci_dev.dev.
> My current understanding is that the best way to do this would be to register the attribute
> with device_add_groups(&pdev->dev, cxl_rcd_groups) on probe and remove the files with

No, the dynamic sysfs registration APIs should be avoided when possible.
The above fix is what you need.

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

[Daisuke Kobayashi (Fujitsu)]

Dan Williams wrote:
> Daisuke Kobayashi (Fujitsu) wrote:
> [..]
> > I would like to report on some additional findings.
> > The process of registering cxl_rcd_groups to struct
> > pci_driver.driver.dev_groups seems to not generate a file in sysfs when
> looking at the contents of the module_pci_driver() macro.
> 
> Oh, apologies, and thanks for taking a deeper look. The failure is because my
> suggested example led you astray. I suggested this:
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
> 2ff361e756d6..eec04f103aa8 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -971,6 +971,7 @@ static struct pci_driver cxl_pci_driver = {
>         .err_handler            = &cxl_error_handlers,
>         .driver = {
>                 .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
> +               .dev_groups     = cxl_rcd_groups,
>         },
>  };
> 
> 
> ...the correct place to put it is here:
> 
> @@ -969,6 +969,7 @@ static struct pci_driver cxl_pci_driver = {
>         .id_table               = cxl_mem_pci_tbl,
>         .probe                  = cxl_pci_probe,
>         .err_handler            = &cxl_error_handlers,
> +       .dev_groups             = cxl_rcd_groups,
>         .driver = {
>                 .probe_type     = PROBE_PREFER_ASYNCHRONOUS,
>         },
> 
> 
> ...otherwise __pci_register_driver() will overwrite it. This is a subtle bug given
> probe_type is directly initialized in .driver.
> 
> > For this feature, I think it would be best to output the values to a
> > directory of /sys/bus/pci/devices/<pci-addr>/. To output to this
> > directory, the attribute would need to be registered to pci_dev.dev.
> > My current understanding is that the best way to do this would be to
> > register the attribute with device_add_groups(&pdev->dev,
> > cxl_rcd_groups) on probe and remove the files with
> 
> No, the dynamic sysfs registration APIs should be avoided when possible.
> The above fix is what you need.

Thank you. With your guidance and the hint in the previous email, 
I now understand the idea of "groups" and "dev_groups". 
I see that, by registering "cxl_rcd_groups" with "pci_driver.dev_groups", 
files are created in device's sysfs when the device is attached. 
I will update the patch to reflect this.

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

[Bjorn Helgaas]

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

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

[Bjorn Helgaas]

On Tue, Apr 09, 2024 at 09:59:35AM -0500, Bjorn Helgaas wrote:
> 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.
> > 
> > 

Sorry for commenting on v3 when you already posted v4.