[PATCH] drm/amdgpu: Fix uninitialized return value

[Christian König]

Am 28.11.23 um 10:49 schrieb Lazar, Lijo:
>
> On 11/28/2023 3:07 PM, Christian König wrote:
>> Am 27.11.23 um 22:55 schrieb Alex Deucher:
>>> On Mon, Nov 27, 2023 at 2:22 PM Christian König
>>> <ckoenig.leichtzumerken at gmail.com> wrote:
>>>> Am 27.11.23 um 19:29 schrieb Lijo Lazar:
>>>>> The return value is uniinitialized if ras context is NULL.
>>>>>
>>>>> Fixes: 0f4c8faa043c (drm/amdgpu: Move mca debug mode decision to ras)
>>>>>
>>>>> Signed-off-by: Lijo Lazar <lijo.lazar at amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> index 1a8668a63e67..f6b47ebce9d6 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>>>> @@ -3410,7 +3410,7 @@ int amdgpu_ras_reset_gpu(struct 
>>>>> amdgpu_device *adev)
>>>>>    int amdgpu_ras_set_mca_debug_mode(struct amdgpu_device *adev, 
>>>>> bool enable)
>>>>>    {
>>>>>        struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>>>>> -     int ret;
>>>>> +     int ret = 0;
>>>> That's usually considered very bad coding style and complained 
>>>> about by
>>>> automated checkers.
>>>>
>>>> Instead explicitly set the return value in the code paths not actually
>>>> setting it.
>>> In this case, the function is so short, I think it makes things less
>>> readable to do that.
>>
>> Yeah, indeed but that doesn't help us with the coding style checkers.
>
> Are these checkers for real? I see many instances of variable 
> initialization including in core kernel code (ex: workqueue) code.

Yes, I've got multiple complains about that already.

What people basically seem to do is to search for patterns like "int ret 
= 0;... ret = whatever();.. return ret;" with cocci.

This then results in a note that an initialization isn't necessary and 
should be avoided.

Same for things like return after else, e.g. when you have something 
like this "if (...) { ret = whatever(); if (ret) return ret; } else { 
... ret = 0;} return ret;".

Regards,
Christian.

>
> Thanks
>
> Lijo
>
>>
>> We could do something like this instead:
>>
>> if (!con)
>>     return 0;
>>
>> ret = amdgpu_mca_smu_set_debug_mode(adev, enable);
>> ...
>>
>> Regards,
>> Christian.
>>
>>>
>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>        if (con) {
>>>>>                ret = amdgpu_mca_smu_set_debug_mode(adev, enable);
>>