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