| 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
| ||
|
Message-ID: <6ef5e1e8-3868-46c0-a31b-505f8a91d37a@vivo.com>
Date: Thu, 14 Aug 2025 19:56:23 +0800
From: Liao Yuanhong <liaoyuanhong@...o.com>
To: Tvrtko Ursulin <tursulin@...ulin.net>
Cc: Christian König <christian.koenig@....com>,
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 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?
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) ||
>>
>
Powered by blists - more mailing lists