RE: [PATCH v5 1/3] CXL, PCIE: Add cxl_rcrb_addr file to dport_dev

Dan Williams
2023-09-26T22:53:45Z

Ben Cheatham wrote:
> Add cxl_rcrb_addr to the dport_dev (normally represented by a pcie
> device) for CXL RCH root ports. The file will print the RCRB base
> MMIO address of the root port when read and will be used by
> users looking to inject CXL EINJ error types for RCH hosts.

RCRB is an implementation detail of RCH topologies, I don't see why
userspace needs this information, maybe it becomes clearer in the follow
on patches, but I would hope this detail could be hidden.

> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
> Documentation/ABI/testing/sysfs-bus-cxl | 9 ++++
> drivers/cxl/acpi.c | 2 +
> drivers/cxl/core/port.c | 58 +++++++++++++++++++++++++
> drivers/cxl/cxl.h | 2 +
> 4 files changed, 71 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 087f762ebfd5..85621da69296 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -177,6 +177,15 @@ Description:
> integer reflects the hardware port unique-id used in the
> hardware decoder target list.
>
> +What: /sys/bus/cxl/devices/portX/dportY/cxl_rcrb_addr
> +What: /sys/devices/pciX/cxl_rcrb_addr
> +Date: August, 2023
> +KernelVersion: v6.6
> +Contact: linux-cxl@vger.kernel.org
> +Description:
> + (RO) The 'cxl_rcrb_addr' device file gives the MMIO base address
> + of the RCRB of the corresponding CXL 1.1 downstream port. Only
> + present for CXL 1.1 dports.
>
> What: /sys/bus/cxl/devices/decoderX.Y
> Date: June, 2021
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index d1c559879dcc..3e2ca946bf47 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -676,6 +676,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
> if (IS_ERR(root_port))
> return PTR_ERR(root_port);
>
> + set_cxl_root(root_port);
> +

The cxl_root is not a singleton and the way to determine this linkage is
by walking up the port hierarchy. See find_cxl_root().

> rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
> add_host_bridge_dport);
> if (rc < 0)
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 724be8448eb4..c3914e73f67e 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -875,6 +875,14 @@ struct cxl_port *find_cxl_root(struct cxl_port *port)
> }
> EXPORT_SYMBOL_NS_GPL(find_cxl_root, CXL);
>
> +static struct cxl_port *cxl_root;
> +
> +void set_cxl_root(struct cxl_port *root_port)
> +{
> + cxl_root = root_port;
> +}
> +EXPORT_SYMBOL_NS_GPL(set_cxl_root, CXL);
> +
> static struct cxl_dport *find_dport(struct cxl_port *port, int id)
> {
> struct cxl_dport *dport;
> @@ -930,11 +938,56 @@ static void cond_cxl_root_unlock(struct cxl_port *port)
> device_unlock(&port->dev);
> }
>
> +static ssize_t cxl_rcrb_addr_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cxl_dport *dport;
> +
> + if (!cxl_root)
> + return -ENODEV;
> +
> + dport = cxl_find_dport_by_dev(cxl_root, dev);
> + if (!dport)
> + return -ENODEV;
> +
> + return sysfs_emit(buf, "0x%llx\n", (u64) dport->rcrb.base);
> +}
> +DEVICE_ATTR_RO(cxl_rcrb_addr);
> +
> +static umode_t cxl_rcrb_addr_is_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct cxl_dport *dport;
> +
> + if (!IS_ENABLED(CONFIG_ACPI_APEI_EINJ) || !cxl_root)
> + return 0;
> +
> + dport = cxl_find_dport_by_dev(cxl_root, dev);
> + if (!dport || !dport->rch || dport->rcrb.base == CXL_RESOURCE_NONE)
> + return 0;
> +
> + return a->mode;
> +}
> +
> +static struct attribute *cxl_rcrb_addr_attrs[] = {
> + &dev_attr_cxl_rcrb_addr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group cxl_rcrb_addr_group = {
> + .attrs = cxl_rcrb_addr_attrs,
> + .is_visible = cxl_rcrb_addr_is_visible,
> +};
> +
> static void cxl_dport_remove(void *data)
> {
> struct cxl_dport *dport = data;
> struct cxl_port *port = dport->port;
>
> + if (dport->rch)
> + sysfs_remove_group(&dport->dport_dev->kobj, &cxl_rcrb_addr_group);
> +
> xa_erase(&port->dports, (unsigned long) dport->dport_dev);
> put_device(dport->dport_dev);
> }
> @@ -1021,6 +1074,11 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> if (rc)
> return ERR_PTR(rc);
>
> + rc = sysfs_create_group(&dport_dev->kobj, &cxl_rcrb_addr_group);
> + if (rc)
> + dev_dbg(dport_dev, "Couldn't create cxl_rcrb_addr group: %d\n",
> + rc);

Please no dynamic sysfs attribute registration. If this attribute is
needed it should be static.