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

["Khatri, Sunil" <sunil.khatri@amd.com>]

On 10/17/2024 5:22 PM, Christian König wrote:
> 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@amd.com>
>> ---
>>   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.

Sure. Noted.

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