Re: [PATCH v3 2/2] drm/xe/xe_device_sysfs: Remove sysfs unconditionally while clean sysfs entries

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

please use correct subject prefix

On 9/15/2025 11:47 PM, Zongyao Bai wrote:
> xe_device_sysfs_fini() and late_bind_remove_files():

above line could be dropped,
below lines don't need to be indented 
>   Adjust the remove order of sysfs entries to follow LIFO.
>   Remove configuration and status checking,
>   call the sysfs removal unconditionally.
> 
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Stuart Summers <stuart.summers@intel.com>
> Cc: Shuicheng Lin <shuicheng.lin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Zongyao Bai <zongyao.bai@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device_sysfs.c | 32 ++++++----------------------
>  1 file changed, 6 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c b/drivers/gpu/drm/xe/xe_device_sysfs.c
> index e5e3c56ea53c..693bfda1cfa6 100644
> --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> @@ -184,25 +184,8 @@ static int late_bind_create_files(struct device *dev)
>  
>  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;
> -
> -	if (REG_FIELD_GET(V1_FAN_SUPPORTED, cap))
> -		sysfs_remove_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
> -
> -	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);
> +	sysfs_remove_file(&dev->kobj, &dev_attr_lb_voltage_regulator_version.attr);
> +	sysfs_remove_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);
>  }
>  
>  /**
> @@ -287,14 +270,11 @@ static const struct attribute *auto_link_downgrade_attrs[] = {
>  static void xe_device_sysfs_fini(void *arg)
>  {
>  	struct xe_device *xe = arg;
> +	struct device *dev = xe->drm.dev;

if you only need "dev" here then maybe pass it as "arg" instead of "xe" ?

>  
> -	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);
> -	}
> +    late_bind_remove_files(dev);

as you remove files unconditionally, does it make sense to keep this helper function?
maybe just move here:

  +	sysfs_remove_file(&dev->kobj, &dev_attr_lb_voltage_regulator_version.attr);
  +	sysfs_remove_file(&dev->kobj, &dev_attr_lb_fan_control_version.attr);

> +    sysfs_remove_files(&dev->kobj, auto_link_downgrade_attrs);
> +    sysfs_remove_file(&dev->kobj, &dev_attr_vram_d3cold_threshold.attr);
>  }
>  
>  int xe_device_sysfs_init(struct xe_device *xe)