[PATCH] drm/amdgpu: fix uninitialized return value

[PATCH] drm/amdgpu: fix uninitialized return value

[ts8060]

The amdgpu_display_suspend_helper() function uses an uninitialized
variable as its return value, causing a failure to suspend/resume on a
Radeon R7 240/340 GPU, with the following error in dmesg:
[drm:amdgpu_device_ip_suspend_phase1 [amdgpu]] *ERROR* suspend of IP block <dce_v6_0> failed 118489088

Initialize the variable to 0.

Signed-off-by: Tim Saunders <ts8060@my.bristol.ac.uk>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 8a1fb8b6606e..27626d8a0647 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1544,7 +1544,7 @@ int amdgpu_display_suspend_helper(struct amdgpu_device *adev)
        struct drm_crtc *crtc;
        struct drm_connector *connector;
        struct drm_connector_list_iter iter;
-       int r;
+       int r = 0;

        /* turn off display hw */
        drm_modeset_lock_all(dev);
--
2.31.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

On Tue, May 11, 2021 at 1:25 PM ts8060 <ts8060@my.bristol.ac.uk> wrote:
>
> The amdgpu_display_suspend_helper() function uses an uninitialized
> variable as its return value, causing a failure to suspend/resume on a
> Radeon R7 240/340 GPU, with the following error in dmesg:
> [drm:amdgpu_device_ip_suspend_phase1 [amdgpu]] *ERROR* suspend of IP block <dce_v6_0> failed 118489088
>
> Initialize the variable to 0.
>
> Signed-off-by: Tim Saunders <ts8060@my.bristol.ac.uk>

Already fixed here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4b12ee6f426e5e36396501a58f3a1af5b92a7e06

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 8a1fb8b6606e..27626d8a0647 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1544,7 +1544,7 @@ int amdgpu_display_suspend_helper(struct amdgpu_device *adev)
>         struct drm_crtc *crtc;
>         struct drm_connector *connector;
>         struct drm_connector_list_iter iter;
> -       int r;
> +       int r = 0;
>
>         /* turn off display hw */
>         drm_modeset_lock_all(dev);
> --
> 2.31.1
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Re: [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@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.

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

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

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

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

[Christian König]

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.

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

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

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

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Regards,
> Christian.
>
> >
> >       if (con) {
> >               ret = amdgpu_mca_smu_set_debug_mode(adev, enable);
>

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

[Christian König]

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.

Regards,
Christian.

>   
>   	if (con) {
>   		ret = amdgpu_mca_smu_set_debug_mode(adev, enable);

[PATCH] drm/amdgpu: Fix uninitialized return value

[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;
 
 	if (con) {
 		ret = amdgpu_mca_smu_set_debug_mode(adev, enable);
-- 
2.25.1