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
