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

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

On 9/15/2025 5:17 PM, Raag Jadav 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%
> 
> I find the summary to be sufficient but upto you.

sure, can strip this (if there will be v2)

> 
>> 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")
>> 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,
>> +};
>> +
>>  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,
>> +	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;
> 
> Should we keep the original log so we don't have to guesswork our way around
> random pcode errors?

hmm, I was assuming that PCODE_LATE_BINDING is also used elsewhere
so it will be reported, but it looks that it's not

but OTOH, is it a right place (sysfs init, or now is_visible)
to report any missing firmware cap?

maybe there should be xe_lb_init() somewhere, where we can report any
unexpected problems, and then in xe_sysfs_init() we will just use
already retrieved and confirmed cap?

but, still the aim of this series was different

> 
> PS: I've found patience algo to generate much readable diffs but ofcourse
> it's a personal preference :)

it was patience algorithm already ;)

> 
> Raag