[PATCH] drm/amdgpu: Fix uninitialized return value

[Felix Kuehling]

On 2023-11-28 8:18, Christian König wrote:
> 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.

Isn't that the opposite of defensive programming? If you write your 
kernel code in Rust, all your local variables will be implicitly 
initialized. In C you have to do it yourself. And the compiler is 
notoriously inconsistent warning about uninitialized variables.

Regards,
   Felix


>
> 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);
>>>
>