| Message ID | 20241108-coverity1593397signextension-v2-1-4acdf3968d2d@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity | expand |
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@gmail.com> Overall, looks good to me. Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.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@gmail.com> >
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++) {
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 value to u32 variable and using it for the bit wise operation, thus ensuring no unintentional sign extension occurs. Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com> --- 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(-) --- base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e change-id: 20241104-coverity1593397signextension-78c9b2c21d51 Best regards,