[PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

[PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

[Karan Sanghavi]

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(-)

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>

Re: [PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

[Yazen Ghannam]

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>
> 

Re: [PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

[Karan Sanghavi]

On Tue, Nov 12, 2024 at 10:04:19AM -0500, Yazen Ghannam wrote:
> 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>
> >

Dear Yazen,

I hope this email finds you well. 
I'm following up on the patch I recently submitted linked below.
https://lore.kernel.org/all/20241108-coverity1593397signextension-v2-1-4acdf3968d2d@gmail.com/

I noticed it hasn't been applied yet, and I wanted to see if there was 
anything else needed from my end. 
Please let me know if any further information or modifications are required.
I appreciate your time and feedback.  

Thank you! 

Sincerely,
Karan.

Re: [PATCH v2] RAS/AMD/ATL: Fix unintended sign extension issue from coverity

[Markus Elfring]

…
> 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.

Would you like to reconsider such a word wrapping approach
besides typo avoidance?


See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12#n94

Regards,
Markus