diff mbox series

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

Message ID 20230925200127.504256-2-Benjamin.Cheatham@amd.com (mailing list archive)
State Superseded, archived
Headers show
Series CXL, ACPI, APEI, EINJ: Update EINJ for CXL 1.1 error types | expand

Commit Message

Ben Cheatham Sept. 25, 2023, 8:01 p.m. UTC
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.

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

Comments

Jonathan Cameron Sept. 26, 2023, 10:50 a.m. UTC | #1
On Mon, 25 Sep 2023 15:01:25 -0500
Ben Cheatham <Benjamin.Cheatham@amd.com> 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.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
Hi Ben,

I'm still not totally convinced that injecting the group via the link
onto the PCI device is necessarily a good idea, but if Bjorn is fine with
that I don't mind too much.

There is a question on whether this should also be added to the
sysfs-bus-pci docs given it turns up on a device where people might look there
first.

So with that in mind
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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);
> +
>  	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);
> +
>  	return dport;
>  }
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 76d92561af29..4d5bce4bae7e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -690,6 +690,8 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>  				   resource_size_t component_reg_phys,
>  				   struct cxl_dport *parent_dport);
>  struct cxl_port *find_cxl_root(struct cxl_port *port);
> +void set_cxl_root(struct cxl_port *root_port);
> +
>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
>  void cxl_bus_rescan(void);
>  void cxl_bus_drain(void);
Ben Cheatham Sept. 26, 2023, 4 p.m. UTC | #2
Hi Jonathan, thanks for the review! Responses inline.

On 9/26/23 5:50 AM, Jonathan Cameron wrote:
> On Mon, 25 Sep 2023 15:01:25 -0500
> Ben Cheatham <Benjamin.Cheatham@amd.com> 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.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Hi Ben,
> 
> I'm still not totally convinced that injecting the group via the link
> onto the PCI device is necessarily a good idea, but if Bjorn is fine with
> that I don't mind too much.
> 

I'm not 100% convinced it's great either, but it was the cleanest implementation
I could come up with at the time. I'll try and see if I can come up with something
better if I get some extra time in the near future, otherwise I'll leave it as is
if Bjorn approves.

> There is a question on whether this should also be added to the
> sysfs-bus-pci docs given it turns up on a device where people might look there
> first.
> 

I agree, I'll move it to sysfs-bus-pci. I didn't move it this revision because the device
the file was on (pciX) wasn't under sys/bus/pci on my test system.

> So with that in mind
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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);
>> +
>>  	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);
>> +
>>  	return dport;
>>  }
>>  
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 76d92561af29..4d5bce4bae7e 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -690,6 +690,8 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>>  				   resource_size_t component_reg_phys,
>>  				   struct cxl_dport *parent_dport);
>>  struct cxl_port *find_cxl_root(struct cxl_port *port);
>> +void set_cxl_root(struct cxl_port *root_port);
>> +
>>  int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
>>  void cxl_bus_rescan(void);
>>  void cxl_bus_drain(void);
>
Bjorn Helgaas Sept. 26, 2023, 8:23 p.m. UTC | #3
On Mon, Sep 25, 2023 at 03:01:25PM -0500, 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.

I guess this is talking about a sysfs file?  If so, maybe mention that
explicitly in the subject and commit log.

I don't know how you decided to capitalize CXL initialisms and not
"pcie", but I usually use "PCIe".  

> +static struct cxl_port *cxl_root;
> +
> +void set_cxl_root(struct cxl_port *root_port)
> +{
> +	cxl_root = root_port;
> +}

Is there always at most one cxl_root?  Seems worth a one-line comment
at the static data item, since in the world of devices, data is
usually in a per-device struct, not in a single static item.

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

Is there any way to create this with an attribute group that the sysfs
infrastructure adds automatically?  I'm not suggesting you have a race
condition here, but using the sysfs infrastructure avoids a lot of
potential problems.

Bjorn
Dan Williams Sept. 26, 2023, 9:15 p.m. UTC | #4
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.
Ben Cheatham Sept. 27, 2023, 3:30 p.m. UTC | #5
Hi Bjorn, thanks for taking a look!

On 9/26/23 3:23 PM, Bjorn Helgaas wrote:
> On Mon, Sep 25, 2023 at 03:01:25PM -0500, 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.
> 
> I guess this is talking about a sysfs file?  If so, maybe mention that
> explicitly in the subject and commit log.
> 
> I don't know how you decided to capitalize CXL initialisms and not
> "pcie", but I usually use "PCIe".  
> 

Sorry about that, it's a typo. I should've written PCIE/PCIe.

>> +static struct cxl_port *cxl_root;
>> +
>> +void set_cxl_root(struct cxl_port *root_port)
>> +{
>> +	cxl_root = root_port;
>> +}
> 
> Is there always at most one cxl_root?  Seems worth a one-line comment
> at the static data item, since in the world of devices, data is
> usually in a per-device struct, not in a single static item.
> 

I thought that the root was a singleton, but per Dan's comment it seems
that isn't the case.

>> @@ -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);
> 
> Is there any way to create this with an attribute group that the sysfs
> infrastructure adds automatically?  I'm not suggesting you have a race
> condition here, but using the sysfs infrastructure avoids a lot of
> potential problems.
> 

I would need to look into it more. I did it this way because I wasn't sure
there was a way to set it up statically since the dport_dev can match a
couple of different device types. After looking at Dan's comments I will
probably move away from making this file altogether and move the functionality
under sysfs/debug/cxl.

> Bjorn
Ben Cheatham Sept. 27, 2023, 3:31 p.m. UTC | #6
Thanks for the review Dan, responses inline.

On 9/26/23 4:15 PM, Dan Williams wrote:
> 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.
> 

It doesn't, I'll rename the file (if it stays at all).

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

Ok, I was under the impression it was and couldn't find anything definitive
in the CXL spec about it. The reason I did that was that I couldn't get
access to the CXL port hierarchy from the EINJ model and needed an
access point.

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

Understood. The file probably won't stay in v6, but I'll keep everything
static.
diff mbox series

Patch

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);
+
 	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);
+
 	return dport;
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 76d92561af29..4d5bce4bae7e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -690,6 +690,8 @@  struct cxl_port *devm_cxl_add_port(struct device *host,
 				   resource_size_t component_reg_phys,
 				   struct cxl_dport *parent_dport);
 struct cxl_port *find_cxl_root(struct cxl_port *port);
+void set_cxl_root(struct cxl_port *root_port);
+
 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
 void cxl_bus_rescan(void);
 void cxl_bus_drain(void);