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

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

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