linux-kernel - Re: [PATCH] drm/radeon: Move the memset() function call after the return statement

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f0d58122-5958-4c93-b1a7-daa229a723b6@amd.com>
Date: Thu, 14 Aug 2025 14:29:14 +0200
From: Christian König <christian.koenig@....com>
To: Liao Yuanhong <liaoyuanhong@...o.com>,
 Tvrtko Ursulin <tursulin@...ulin.net>
Cc: Alex Deucher <alexander.deucher@....com>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 "open list:RADEON and AMDGPU DRM DRIVERS" <amd-gfx@...ts.freedesktop.org>,
 "open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/radeon: Move the memset() function call after the
 return statement

On 14.08.25 13:56, Liao Yuanhong wrote:
> On 8/14/2025 7:07 PM, Tvrtko Ursulin wrote:
> 
>>
>> On 14/08/2025 11:11, Christian König wrote:
>>> On 14.08.25 11:39, Liao Yuanhong wrote:
>>>> Adjust the position of the memset() call to avoid unnecessary invocations.
>>>
>>> Hui? That doesn't seem to make much sense to me.
>>>
>>> We memset the local variable because we need to make sure that everything (including padding bytes) is zeroed out.
>>>
>>> Even if that isn't sometimes necessary because of error handling we clearly shouldn't try to optimize this.
>>
>> To interject with a curiousity, it is even worse to move the memset away from the declaration block when the often enabled CONFIG_INIT_STACK_ALL_ZERO is on. At least when they are close compiler can figure out it only needs to memset it once. Move them further apart and sometimes memset happens twice, yay.
>>
>> Main point when CONFIG_INIT_STACK_ALL_ZERO is on, and it often is, there is no way to avoid the sometimes pointless init. I have some local branches which try to address the worst offenders but it is so much in the noise that I don't think I ever sent them out.
>>
>> Regards,
>>
>> Tvrtko
>>
> If we converts memset() to = { },can it alleviate the problem?

No, memset is intentionally used here to make sure we set all bytes to zero. Including padding.

Regards,
Christian.

> 
> 
> Thanks,
> 
> Liao
> 
>>>> Signed-off-by: Liao Yuanhong <liaoyuanhong@...o.com>
>>>> ---
>>>>   drivers/gpu/drm/radeon/atombios_crtc.c     |  8 ++++----
>>>>   drivers/gpu/drm/radeon/atombios_encoders.c | 20 ++++++++++----------
>>>>   2 files changed, 14 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
>>>> index 9b3a3a9d60e2..0aae84f5e27c 100644
>>>> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
>>>> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
>>>> @@ -770,13 +770,13 @@ static void atombios_crtc_set_disp_eng_pll(struct radeon_device *rdev,
>>>>       int index;
>>>>       union set_pixel_clock args;
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       index = GetIndexIntoMasterTable(COMMAND, SetPixelClock);
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev,
>>>>                      &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       switch (frev) {
>>>>       case 1:
>>>>           switch (crev) {
>>>> @@ -832,12 +832,12 @@ static void atombios_crtc_program_pll(struct drm_crtc *crtc,
>>>>       int index = GetIndexIntoMasterTable(COMMAND, SetPixelClock);
>>>>       union set_pixel_clock args;
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev,
>>>>                      &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       switch (frev) {
>>>>       case 1:
>>>>           switch (crev) {
>>>> diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
>>>> index d1c5e471bdca..f706e21a3509 100644
>>>> --- a/drivers/gpu/drm/radeon/atombios_encoders.c
>>>> +++ b/drivers/gpu/drm/radeon/atombios_encoders.c
>>>> @@ -492,11 +492,11 @@ atombios_dvo_setup(struct drm_encoder *encoder, int action)
>>>>       int index = GetIndexIntoMasterTable(COMMAND, DVOEncoderControl);
>>>>       uint8_t frev, crev;
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       /* some R4xx chips have the wrong frev */
>>>>       if (rdev->family <= CHIP_RV410)
>>>>           frev = 1;
>>>> @@ -856,8 +856,6 @@ atombios_dig_encoder_setup2(struct drm_encoder *encoder, int action, int panel_m
>>>>       if (dig->dig_encoder == -1)
>>>>           return;
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       if (ASIC_IS_DCE4(rdev))
>>>>           index = GetIndexIntoMasterTable(COMMAND, DIGxEncoderControl);
>>>>       else {
>>>> @@ -870,6 +868,8 @@ atombios_dig_encoder_setup2(struct drm_encoder *encoder, int action, int panel_m
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       switch (frev) {
>>>>       case 1:
>>>>           switch (crev) {
>>>> @@ -1453,11 +1453,11 @@ atombios_external_encoder_setup(struct drm_encoder *encoder,
>>>>               (radeon_connector->connector_object_id & OBJECT_ID_MASK) >> OBJECT_ID_SHIFT;
>>>>       }
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       switch (frev) {
>>>>       case 1:
>>>>           /* no params on frev 1 */
>>>> @@ -1853,11 +1853,11 @@ atombios_set_encoder_crtc_source(struct drm_encoder *encoder)
>>>>       uint8_t frev, crev;
>>>>       struct radeon_encoder_atom_dig *dig;
>>>>   -    memset(&args, 0, sizeof(args));
>>>> -
>>>>       if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
>>>>           return;
>>>>   +    memset(&args, 0, sizeof(args));
>>>> +
>>>>       switch (frev) {
>>>>       case 1:
>>>>           switch (crev) {
>>>> @@ -2284,11 +2284,11 @@ atombios_dac_load_detect(struct drm_encoder *encoder, struct drm_connector *conn
>>>>           int index = GetIndexIntoMasterTable(COMMAND, DAC_LoadDetection);
>>>>           uint8_t frev, crev;
>>>>   -        memset(&args, 0, sizeof(args));
>>>> -
>>>>           if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
>>>>               return false;
>>>>   +        memset(&args, 0, sizeof(args));
>>>> +
>>>>           args.sDacload.ucMisc = 0;
>>>>             if ((radeon_encoder->encoder_id == ENCODER_OBJECT_ID_INTERNAL_DAC1) ||
>>>
>>