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

[Ben Cheatham <benjamin.cheatham@amd.com>]

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.