Re: [PATCH] drm/amdgpu: Fix uninitialized return value

["Lazar, Lijo" <lijo.lazar@amd.com>]

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@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@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.

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@amd.com>
>>
>>> Regards,
>>> Christian.
>>>
>>>>        if (con) {
>>>>                ret = amdgpu_mca_smu_set_debug_mode(adev, enable);
>