Re: [PATCH v4 05/15] drm/amdgpu: validate hw_fini before function call

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 17.10.24 um 12:06 schrieb Sunil Khatri:
> Before making a function call to hw_fini, validate
> the function pointer like we do in sw_init.
>
> Signed-off-by: Sunil Khatri <sunil.khatri@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 ++++++++++++++--------
>   1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index cf84b50f2355..96cecd4a636b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3287,11 +3287,17 @@ static void amdgpu_device_smu_fini_early(struct amdgpu_device *adev)
>   		if (!adev->ip_blocks[i].status.hw)
>   			continue;
>   		if (adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_SMC) {
> -			r = adev->ip_blocks[i].version->funcs->hw_fini(&adev->ip_blocks[i]);
> -			/* XXX handle errors */
> -			if (r) {
> -				DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> -					  adev->ip_blocks[i].version->funcs->name, r);
> +			if (!adev->ip_blocks[i].version->funcs->hw_fini) {
> +				DRM_ERROR("hw_fini of IP block <%s> not defined\n",
> +						adev->ip_blocks[i].version->funcs->name);
> +			} else {
> +				r = adev->ip_blocks[i].version->funcs->hw_fini(
> +					&adev->ip_blocks[i]);
> +				/* XXX handle errors */
> +				if (r) {
> +					DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> +						adev->ip_blocks[i].version->funcs->name, r);
> +				}
>   			}
>   			adev->ip_blocks[i].status.hw = false;
>   			break;
> @@ -3325,12 +3331,17 @@ static int amdgpu_device_ip_fini_early(struct amdgpu_device *adev)
>   	for (i = adev->num_ip_blocks - 1; i >= 0; i--) {
>   		if (!adev->ip_blocks[i].status.hw)
>   			continue;
> -
> -		r = adev->ip_blocks[i].version->funcs->hw_fini(&adev->ip_blocks[i]);
> -		/* XXX handle errors */
> -		if (r) {
> -			DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> -				  adev->ip_blocks[i].version->funcs->name, r);
> +		if (!adev->ip_blocks[i].version->funcs->hw_fini) {
> +			DRM_ERROR("hw_fini of IP block <%s> not defined\n",
> +				  adev->ip_blocks[i].version->funcs->name);
> +		} else {
> +			r = adev->ip_blocks[i].version->funcs->hw_fini(
> +				&adev->ip_blocks[i]);
> +			/* XXX handle errors */
> +			if (r) {
> +				DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
> +					  adev->ip_blocks[i].version->funcs->name, r);
> +			}

I strongly suggest to make this a separate function, e.g. something like 
amdgpu_ip_block_hw_fini() and then call it twice.

Regards,
Christian.

>   		}
>   
>   		adev->ip_blocks[i].status.hw = false;