[AMD Official Use Only - General]
Hi Christian
Agree with you, returning an error is surely a better modification.
I will send v2 patch to fix this.
Regards,
Bob
-----Original Message-----
From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: 2024年4月23日 15:41
To: Zhou, Bob <Bob.Zhou@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Subject: Re: [PATCH 2/2] drm/amdgpu: fix uninitialized variable warning
In this case we should modify amdgpu_i2c_get_byte() to return an error and prevent writing the value back.
See zero is as random as any other value and initializing the variable here doesn't really help, it just makes your warning disappear.
Regards,
Christian.
Am 23.04.24 um 08:27 schrieb Zhou, Bob:
> [AMD Official Use Only - General]
>
> Thanks for your comments.
>
> I should clarify the issue. As you see the amdgpu_i2c_get_byte code:
> if (i2c_transfer(&i2c_bus->adapter, msgs, 2) == 2) {
> *val = in_buf[0];
> DRM_DEBUG("val = 0x%02x\n", *val);
> } else {
> DRM_DEBUG("i2c 0x%02x 0x%02x read failed\n", addr, *val);
> }
> If the read failure by amdgpu_i2c_get_byte(), the value will not be modified.
> Then the amdgpu_i2c_put_byte() successfully written the random value and it will cause unexpected issue.
>
> Regards,
> Bob
>
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig@xxxxxxx>
> Sent: 2024年4月23日 14:05
> To: Zhou, Bob <Bob.Zhou@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>
> Subject: Re: [PATCH 2/2] drm/amdgpu: fix uninitialized variable
> warning
>
> Am 23.04.24 um 07:33 schrieb Bob Zhou:
>> Because the val isn't initialized, a random variable is set by amdgpu_i2c_put_byte.
>> So fix the uninitialized issue.
> Well that isn't correct. See the code here:
>
> amdgpu_i2c_get_byte(amdgpu_connector->router_bus,
> amdgpu_connector->router.i2c_addr,
> 0x3, &val);
> val &= ~amdgpu_connector->router.cd_mux_control_pin;
> amdgpu_i2c_put_byte(amdgpu_connector->router_bus,
> amdgpu_connector->router.i2c_addr,
> 0x3, val);
>
> The value is first read by amdgpu_i2c_get_byte(), then modified and then written again by amdgpu_i2c_put_byte().
>
> Was this an automated warning?
>
> Either way the patch is clearly rejected.
>
> Regards,
> Christian.
>
>> Signed-off-by: Bob Zhou <bob.zhou@xxxxxxx>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
>> index 82608df43396..d4d2dc792b60 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_i2c.c
>> @@ -368,7 +368,7 @@ amdgpu_i2c_router_select_ddc_port(const struct amdgpu_connector *amdgpu_connecto
>> void
>> amdgpu_i2c_router_select_cd_port(const struct amdgpu_connector *amdgpu_connector)
>> {
>> - u8 val;
>> + u8 val = 0;
>>
>> if (!amdgpu_connector->router.cd_valid)
>> return;
