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

[Bjorn Helgaas <helgaas@kernel.org>]

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