Re: [PATCH 1/2] drm/xe/sysfs: Simplify and fix sysfs registration

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 9/15/2025 5:12 PM, Lucas De Marchi wrote:
> On Mon, Sep 15, 2025 at 04:06:13PM +0200, Michal Wajdeczko wrote:
>> Instead of manually maintaining each sysfs attribute define and use
>> attribute groups and register them using device managed function.
>> Then use is_visible() to filter-out unsupported attributes.
>>
>> This will result not only in less code and smaller footprint:
>>
>>  Function                                     old     new   delta
>>  late_bind_attr_is_visible                      -     183    +183
>>  ____versions                               80832   80896     +64
>>  vram_attr_group                                -      48     +48
>>  late_bind_attr_group                           -      48     +48
>>  auto_link_downgrade_attr_group                 -      48     +48
>>  late_bind_attrs                                -      24     +24
>>  vram_attrs                                     -      16     +16
>>  __pfx_late_bind_attr_is_visible                -      16     +16
>>  xe_device_sysfs_init.cold                     20      21      +1
>>  __pfx_xe_device_sysfs_fini                    16       -     -16
>>  xe_device_sysfs_fini.cold                     21       -     -21
>>  xe_device_sysfs_fini                         271       -    -271
>>  xe_device_sysfs_init                         421     135    -286
>>  Total: Before=2848898, After=2848752, chg -0.01%
>>
>> but will also fix some bad error handling that we had here.
>>
>> Fixes: 0e414bf7ad01 ("drm/xe: Expose PCIe link downgrade attributes")
>> Fixes: cdc36b66cd41 ("drm/xe: Expose fan control and voltage regulator version")
> 
> we should probably land the fix separately though:
> https://lore.kernel.org/intel-xe/7p7wqbghc2ebupbzljjaegakun3azczuby2haiqmmneavnpl44@joqtzfismsgv/
> 
> and suggestion to rather use attribute group, similarly to what you're
> doing here:
> https://lore.kernel.org/intel-xe/3tkvzv7krbeuofuew44zm3ibenlntqkfclueq3soazkagwkezg@wtdhq4yvengu/
> 
> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Raag Jadav <raag.jadav@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_device_sysfs.c | 96 +++++++++++-----------------
>> 1 file changed, 39 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/xe/xe_device_sysfs.c
>> index 6ee422594b56..5b0b98ac9b17 100644
>> --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
>> @@ -71,6 +71,15 @@ vram_d3cold_threshold_store(struct device *dev, struct device_attribute *attr,
>>
>> static DEVICE_ATTR_RW(vram_d3cold_threshold);
>>
>> +static struct attribute *vram_attrs[] = {
>> +    &dev_attr_vram_d3cold_threshold.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group vram_attr_group = {
>> +    .attrs = vram_attrs,
> 
> since we are already checking by the attribute inside the is_visible
> hook, couldn't we just add 1 group for "device attributes" and
> share a single function?

I tried that approach first, but checking for late_bind_attrs was
so different that decided to keep it separated from other attrs

after all, this is what the purpose of having multiple groups is, no?

> 
>> +};
>> +
>> static ssize_t
>> lb_fan_control_version_show(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> @@ -149,8 +158,16 @@ lb_voltage_regulator_version_show(struct device *dev, struct device_attribute *a
>> }
>> static DEVICE_ATTR_ADMIN_RO(lb_voltage_regulator_version);
>>
>> -static int late_bind_create_files(struct device *dev)
>> +static struct attribute *late_bind_attrs[] = {
>> +    &dev_attr_lb_fan_control_version.attr,
>> +    &dev_attr_lb_voltage_regulator_version.attr,
> 
> As above, I think we can just consider them "device attributes".

IMO we should _group_ related attributes into "attribute_group" and
either add them based on single prerequisite condition and/or provide
is_visible filter to control each attribute separately

now as we only have five attribute, it's tempting to put all into
one group, but I assume we will soon have more attributes, and trying
to manage them as single group may not scale, so it's better to show
how to split related attrs now

> 
> Lucas De Marchi
> 
>> +    NULL
>> +};
>> +
>> +static umode_t late_bind_attr_is_visible(struct kobject *kobj,
>> +                     struct attribute *attr, int n)
>> {
>> +    struct device *dev = kobj_to_dev(kobj);
>>     struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
>>     struct xe_tile *root = xe_device_get_root_tile(xe);
>>     u32 cap = 0;
>> @@ -160,51 +177,25 @@ static int late_bind_create_files(struct device *dev)
>>
>>     ret = xe_pcode_read(root, PCODE_MBOX(PCODE_LATE_BINDING, GET_CAPABILITY_STATUS, 0),
>>                 &cap, NULL);
>> -    if (ret) {
>> -        if (ret == -ENXIO) {
>> -            drm_dbg(&xe->drm, "Late binding not supported by firmware\n");
>> -            ret = 0;
>> -        }
>> -        goto out;
>> -    }
>> -
>> -    if (REG_FIELD_GET(V1_FAN_SUPPORTED, cap)) {
>> -        ret = sysfs_create_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
>> -        if (ret)
>> -            goto out;
>> -    }
>> -
>> -    if (REG_FIELD_GET(VR_PARAMS_SUPPORTED, cap))
>> -        ret = sysfs_create_file(&dev->kobj, &dev_attr_lb_voltage_regulator_version.attr);
>> -out:
>>     xe_pm_runtime_put(xe);
>> -
>> -    return ret;
>> -}
>> -
>> -static void late_bind_remove_files(struct device *dev)
>> -{
>> -    struct xe_device *xe = pdev_to_xe_device(to_pci_dev(dev));
>> -    struct xe_tile *root = xe_device_get_root_tile(xe);
>> -    u32 cap = 0;
>> -    int ret;
>> -
>> -    xe_pm_runtime_get(xe);
>> -
>> -    ret = xe_pcode_read(root, PCODE_MBOX(PCODE_LATE_BINDING, GET_CAPABILITY_STATUS, 0),
>> -                &cap, NULL);
>>     if (ret)
>> -        goto out;
>> +        return 0;
>>
>> -    if (REG_FIELD_GET(V1_FAN_SUPPORTED, cap))
>> -        sysfs_remove_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
>> +    if (attr == &dev_attr_lb_fan_control_version.attr &&
>> +        REG_FIELD_GET(V1_FAN_SUPPORTED, cap))
>> +        return attr->mode;
>> +    if (attr == &dev_attr_lb_voltage_regulator_version.attr &&
>> +        REG_FIELD_GET(VR_PARAMS_SUPPORTED, cap))
>> +        return attr->mode;
>>
>> -    if (REG_FIELD_GET(VR_PARAMS_SUPPORTED, cap))
>> -        sysfs_remove_file(&dev->kobj, &dev_attr_lb_voltage_regulator_version.attr);
>> -out:
>> -    xe_pm_runtime_put(xe);
>> +    return 0;
>> }
>>
>> +static const struct attribute_group late_bind_attr_group = {
>> +    .attrs = late_bind_attrs,
>> +    .is_visible = late_bind_attr_is_visible,
>> +};
>> +
>> /**
>>  * DOC: PCIe Gen5 Limitations
>>  *
>> @@ -278,24 +269,15 @@ auto_link_downgrade_status_show(struct device *dev, struct device_attribute *att
>> }
>> static DEVICE_ATTR_ADMIN_RO(auto_link_downgrade_status);
>>
>> -static const struct attribute *auto_link_downgrade_attrs[] = {
>> +static struct attribute *auto_link_downgrade_attrs[] = {
>>     &dev_attr_auto_link_downgrade_capable.attr,
>>     &dev_attr_auto_link_downgrade_status.attr,
>>     NULL
>> };
>>
>> -static void xe_device_sysfs_fini(void *arg)
>> -{
>> -    struct xe_device *xe = arg;
>> -
>> -    if (xe->d3cold.capable)
>> -        sysfs_remove_file(&xe->drm.dev->kobj, &dev_attr_vram_d3cold_threshold.attr);
>> -
>> -    if (xe->info.platform == XE_BATTLEMAGE) {
>> -        sysfs_remove_files(&xe->drm.dev->kobj, auto_link_downgrade_attrs);
>> -        late_bind_remove_files(xe->drm.dev);
>> -    }
>> -}
>> +static const struct attribute_group auto_link_downgrade_attr_group = {
>> +    .attrs = auto_link_downgrade_attrs,
>> +};
>>
>> int xe_device_sysfs_init(struct xe_device *xe)
>> {
>> @@ -303,20 +285,20 @@ int xe_device_sysfs_init(struct xe_device *xe)
>>     int ret;
>>
>>     if (xe->d3cold.capable) {
>> -        ret = sysfs_create_file(&dev->kobj, &dev_attr_vram_d3cold_threshold.attr);
>> +        ret = devm_device_add_group(dev, &vram_attr_group);
>>         if (ret)
>>             return ret;
>>     }
>>
>>     if (xe->info.platform == XE_BATTLEMAGE) {
>> -        ret = sysfs_create_files(&dev->kobj, auto_link_downgrade_attrs);
>> +        ret = devm_device_add_group(dev, &auto_link_downgrade_attr_group);
>>         if (ret)
>>             return ret;
>>
>> -        ret = late_bind_create_files(dev);
>> +        ret = devm_device_add_group(dev, &late_bind_attr_group);
>>         if (ret)
>>             return ret;
>>     }
>>
>> -    return devm_add_action_or_reset(dev, xe_device_sysfs_fini, xe);
>> +    return 0;
>> }
>> -- 
>> 2.47.1
>>