[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241112150419.GA3017802@yaz-khff2.amd.com>
Date: Tue, 12 Nov 2024 10:04:19 -0500
From: Yazen Ghannam <yazen.ghannam@....com>
To: Karan Sanghavi <karansanghvi98@...il.com>
Cc: Tony Luck <tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>,
linux-edac@...r.kernel.org, linux-kernel@...r.kernel.org,
Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from
coverity
On Fri, Nov 08, 2024 at 04:40:41PM +0000, Karan Sanghavi wrote:
> This error is reported by coverity scan stating as
>
> CID 1593397: (#1 of 1): Unintended sign extension (SIGN_EXTENSION)
> sign_extension: Suspicious implicit sign extension: pc
> with type u16 (16 bits, unsigned) is promoted in
> pc << bit_shifts.pc to type int (32 bits, signed),
> then sign-extended to type unsigned long (64 bits, unsigned).
> If pc << bit_shifts.pc is greater than 0x7FFFFFFF,
> the upper bits of the result will all be 1.
>
> Following the code styleof the file, assigning the u16
styleof -> style of
> value to u32 variable and using it for the bit wise
> operation, thus ensuring no unintentional sign
> extension occurs.
>
Please make sure you use an imperative voice here. For example, "assign
the value...and use it...". This should read like you are giving
commands.
> Signed-off-by: Karan Sanghavi <karansanghvi98@...il.com>
Overall, looks good to me.
Reviewed-by: Yazen Ghannam <yazen.ghannam@....com>
Thanks,
Yazen
> ---
> Coverity Link:
> https://scan7.scan.coverity.com/#/project-view/51975/11354?selectedIssue=1593397
> ---
> Changes in v2:
> - Assigning pc value to temp variable before left shifting as mentioned
> in feedback rather then typecasting pc to u32.
> - Link to v1: https://lore.kernel.org/r/20241104-coverity1593397signextension-v1-1-4cfae6532140@gmail.com
> ---
> drivers/ras/amd/atl/umc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ras/amd/atl/umc.c b/drivers/ras/amd/atl/umc.c
> index dc8aa12f63c8..3f4b1f31e14f 100644
> --- a/drivers/ras/amd/atl/umc.c
> +++ b/drivers/ras/amd/atl/umc.c
> @@ -293,7 +293,8 @@ static unsigned long convert_dram_to_norm_addr_mi300(unsigned long addr)
> }
>
> /* PC bit */
> - addr |= pc << bit_shifts.pc;
> + temp = pc;
> + addr |= temp << bit_shifts.pc;
>
> /* SID bits */
> for (i = 0; i < NUM_SID_BITS; i++) {
>
> ---
> base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e
> change-id: 20241104-coverity1593397signextension-78c9b2c21d51
>
> Best regards,
> --
> Karan Sanghavi <karansanghvi98@...il.com>
>
Powered by blists - more mailing lists