Re: [PATCH v4 4/6] drm/amdgpu: handle gpu_recovery per device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: broken gpu_recovery init and ordering in
  amdgpu_device_init()
- [Medium] drm/amdgpu: sysfs reset_mask leak when gpu_recovery is
  disabled at runtime
--

commit da82b226214b96b7716dea3ad2863a26a7b52c07
Author: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>

drm/amdgpu: handle gpu_recovery per device

This commit moves the gpu_recovery setting from a global module parameter
check into struct amdgpu_device and makes the amdgpu_gpu_recover debugfs
file writable so auto-recovery can be toggled per device.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9c6d51445f820..f284e433b72bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3769,6 +3769,45 @@ amdgpu_device_should_register_switcheroo(struct amdgpu_device *adev, bool px)
>  		       apple_gmux_detect(NULL, NULL)));
>  }
>  
> +static void amdgpu_device_init_gpu_recovery(struct amdgpu_device *adev)
> +{
> +	adev->gpu_recovery = 0;
> +
> +	if (amdgpu_gpu_recovery == 0)
> +		return;
> +
> +	/* Skip soft reset check in fatal error mode */
> +	if (!amdgpu_ras_is_poison_mode_supported(adev) ||
> +	    amdgpu_sriov_vf(adev)) {
> +		adev->gpu_recovery = adev->gpu_recovery > 0 ?
> +			adev->gpu_recovery :-1;
> +	} else if (amdgpu_gpu_recovery == -1) {
> +		switch (adev->asic_type) {
> +#ifdef CONFIG_DRM_AMDGPU_SI
> +		case CHIP_VERDE:
> +		case CHIP_TAHITI:
> +		case CHIP_PITCAIRN:
> +		case CHIP_OLAND:
> +		case CHIP_HAINAN:
> +#endif
> +#ifdef CONFIG_DRM_AMDGPU_CIK
> +		case CHIP_KAVERI:
> +		case CHIP_KABINI:
> +		case CHIP_MULLINS:
> +#endif
> +		case CHIP_CARRIZO:
> +		case CHIP_STONEY:
> +		case CHIP_CYAN_SKILLFISH:
> +			return;
> +		default:
> +			adev->gpu_recovery = 1;
> +			break;
> +		}
> +	} else {
> +		adev->gpu_recovery = amdgpu_gpu_recovery;
> +	}
> +}
> +
>  /**
>   * amdgpu_device_init - initialize the driver
>   *
> @@ -4117,6 +4156,8 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  		}
>  	}
>  
> +	amdgpu_device_init_gpu_recovery(adev);
> +
>  fence_driver_init:
>  	/* Fence driver */
>  	r = amdgpu_fence_driver_sw_init(adev);

[Severity: High]
Does amdgpu_device_init_gpu_recovery() always set adev->gpu_recovery to
(unsigned int)-1 whenever amdgpu_gpu_recovery != 0?

adev->gpu_recovery (declared as unsigned int in struct amdgpu_device) is set
to 0 at the top of the function, and then the first branch tests
adev->gpu_recovery > 0 instead of amdgpu_gpu_recovery > 0.  That overwrites
positive module parameter values such as amdgpu.gpu_recovery=4 or 5 with -1,
so the adev->gpu_recovery == 4 || adev->gpu_recovery == 5 check in
soc15_asic_reset_method() can never select Mode-2 reset.

In addition, amdgpu_device_init() calls amdgpu_device_init_gpu_recovery()
before amdgpu_device_ip_init() -> amdgpu_ras_init() allocates the RAS context
and sets con->poison_supported.  At this point
amdgpu_ras_is_poison_mode_supported(adev) always returns false (and legacy
SI/CIK/Carrizo/Stoney/Cyan Skillfish ASICs do not support RAS poison mode
either), making the else-if per-ASIC exclusion switch and the final else
branch unreachable dead code.  Placing the call here also leaves
adev->gpu_recovery at 0 during the earlier reset-on-init amdgpu_asic_reset()
call and skips it altogether when amdgpu_emu_mode == 1 jumps to
fence_driver_init.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 0ae6e990c3102..28f9048f806cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2299,7 +2299,7 @@ static int amdgpu_gfx_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  {
>  	int r = 0;
>  
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return r;
>  
>  	if (adev->gfx.num_gfx_rings) {
> @@ -2319,7 +2319,7 @@ static int amdgpu_gfx_sysfs_reset_mask_init(struct amdgpu_device *adev)
>  
>  static void amdgpu_gfx_sysfs_reset_mask_fini(struct amdgpu_device *adev)
>  {
> -	if (!amdgpu_gpu_recovery)
> +	if (!amdgpu_device_should_recover_gpu(adev))
>  		return;
>  
>  	if (adev->gfx.num_gfx_rings)

[Severity: Medium]
Can disabling gpu_recovery at runtime via debugfs leak the reset_mask sysfs
files on teardown?

If GPU recovery is enabled during sw_init, amdgpu_gfx_sysfs_reset_mask_init()
and amdgpu_sdma_sysfs_reset_mask_init() create gfx_reset_mask,
compute_reset_mask, and sdma_reset_mask on adev->dev.  If a user then writes
0 to amdgpu_gpu_recover in debugfs before driver unbind or module unload,
amdgpu_device_should_recover_gpu(adev) is false in
amdgpu_gfx_sysfs_reset_mask_fini() and amdgpu_sdma_sysfs_reset_mask_fini(),
so device_remove_file() is skipped.  The dangling sysfs attributes remain on
the PCI device, causing a subsequent re-bind to fail with -EEXIST in
device_create_file() and allowing reads of those attributes to access freed
adev memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109171333.4329-1-pierre-eric.pelloux-prayer@amd.com?part=4