Re: [PATCH 1/6] amdgpu: add flags to track sysfs initialization status

["Chen, Xiaogang" <xiaogang.chen@xxxxxxx>]

On 1/1/2025 11:36 PM, Jiang Liu wrote:
> Add flags to track sysfs initialization status, so we can correctly
> clean them up on error recover paths.
>
> Signed-off-by: Jiang Liu <gerry@xxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  3 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 34 +++++++++++++++++-----
>   2 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 22c7e9669162..e4b13e729770 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1157,6 +1157,9 @@ struct amdgpu_device {
>   	bool                            in_runpm;
>   	bool                            has_pr3;
>   
> +	bool				sysfs_en;
> +	bool				fru_sysfs_en;
> +	bool				reg_state_sysfs_en;
>   	bool                            ucode_sysfs_en;
>   
>   	struct amdgpu_fru_info		*fru_info;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index d1bb9e85b6d7..3244966b0c39 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4533,8 +4533,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   		adev->ucode_sysfs_en = true;
>   
>   	r = sysfs_create_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	if (r)
> +	if (r) {
>   		dev_err(adev->dev, "Could not create amdgpu device attr\n");
> +		adev->sysfs_en = false;
> +	} else {
> +		adev->sysfs_en = true;
> +	}
> +
not need use {}
>   #ifdef HAVE_PCI_DRIVER_DEV_GROUPS
>   	r = devm_device_add_group(adev->dev, &amdgpu_board_attrs_group);
>   	if (r)
> @@ -4542,8 +4547,21 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   			"Could not create amdgpu board attributes\n");
>   #endif
>   
> -	amdgpu_fru_sysfs_init(adev);
> -	amdgpu_reg_state_sysfs_init(adev);
> +	r = amdgpu_fru_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu fru attr\n");
> +		adev->fru_sysfs_en = false;
> +	} else {
> +		adev->fru_sysfs_en = true;
> +	}
not need use {}
> +
> +	r = amdgpu_reg_state_sysfs_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "Could not create amdgpu reg state attr\n");
> +		adev->reg_state_sysfs_en = false;
> +	} else {
> +		adev->reg_state_sysfs_en = true;
> +	}
same as above
>   
>   	if (IS_ENABLED(CONFIG_PERF_EVENTS))
>   		r = amdgpu_pmu_init(adev);
> @@ -4666,10 +4684,12 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>   		amdgpu_pm_sysfs_fini(adev);
>   	if (adev->ucode_sysfs_en)
>   		amdgpu_ucode_sysfs_fini(adev);
> -	sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> -	amdgpu_fru_sysfs_fini(adev);
> -
> -	amdgpu_reg_state_sysfs_fini(adev);
> +	if (adev->sysfs_en)
> +		sysfs_remove_files(&adev->dev->kobj, amdgpu_dev_attributes);
> +	if (adev->fru_sysfs_en)
> +		amdgpu_fru_sysfs_fini(adev);
> +	if (adev->reg_state_sysfs_en)
> +		amdgpu_reg_state_sysfs_fini(adev);

If creations of these sys entries fail does kernel send warnings or just 
ignore that when delete these entries? Seems kernel keeps silence for 
this case.

Regards

Xiaogang


>   
>   	/* disable ras feature must before hw fini */
>   	amdgpu_ras_pre_fini(adev);