Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent implicit integer promotion ......................................................................
soc/nvidia/tegra210: Prevent implicit integer promotion
The perennial problem with u16 << 16 strikes again - the u16 is implicitly promoted to an int before the shift, which will then have undefined behaviour if the highest bit of the u16 was set. Cast bytes to a u32 beforehand to prevent this.
Change-Id: Iaf0fb1040ccafafde0093e9bb192c802b86cb2ac Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1294800 --- M src/soc/nvidia/tegra210/dsi.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/34529/1
diff --git a/src/soc/nvidia/tegra210/dsi.c b/src/soc/nvidia/tegra210/dsi.c index ae20d44..6b94e24 100644 --- a/src/soc/nvidia/tegra210/dsi.c +++ b/src/soc/nvidia/tegra210/dsi.c @@ -380,8 +380,8 @@ }
tegra_dsi_writel(dsi, 0, DSI_PKT_LEN_0_1); - tegra_dsi_writel(dsi, bytes << 16, DSI_PKT_LEN_2_3); - tegra_dsi_writel(dsi, bytes << 16, DSI_PKT_LEN_4_5); + tegra_dsi_writel(dsi, (u32)bytes << 16, DSI_PKT_LEN_2_3); + tegra_dsi_writel(dsi, (u32)bytes << 16, DSI_PKT_LEN_4_5); tegra_dsi_writel(dsi, 0, DSI_PKT_LEN_6_7);
value = MIPI_DCS_WRITE_MEMORY_START << 8 |
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent implicit integer promotion ......................................................................
Patch Set 1:
IIRC we had some sort of general agreement in coreboot that we don't care about -Wshift-overflow=2 and don't want to litter all our code with casts just to work around this purely theoretical non-problem? If so, wouldn't this be the same kind of issue, and shouldn't we then equally ignore it?
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent implicit integer promotion ......................................................................
Patch Set 1:
Patch Set 1:
IIRC we had some sort of general agreement in coreboot that we don't care about -Wshift-overflow=2 and don't want to litter all our code with casts just to work around this purely theoretical non-problem? If so, wouldn't this be the same kind of issue, and shouldn't we then equally ignore it?
Hmmmm, I didn't know about that and that's a good point. In this case though things are slightly worse, since tegra_dsi_writel() expects a 64 bit integer for that argument, and if it's negative it will be sign extended to a very large unsigned integer. In this case, tegra_dsi_writel() only needs to write a 32 bit integer, so I think I'll change the argument to a u32 instead to avoid the sign extension.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent implicit integer promotion ......................................................................
Patch Set 1: Code-Review+2
Hmmmm, I didn't know about that and that's a good point. In this case though things are slightly worse, since tegra_dsi_writel() expects a 64 bit integer for that argument, and if it's negative it will be sign extended to a very large unsigned integer. In this case, tegra_dsi_writel() only needs to write a 32 bit integer, so I think I'll change the argument to a u32 instead to avoid the sign extension.
Okay, the sign extension is a valid concern. Your suggestion sounds good, or I'd also be fine with the original CL in that case.
Hello Julius Werner, David Hendricks, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34529
to look at the new patch set (#2).
Change subject: soc/nvidia/tegra210: Prevent unintended sign extension ......................................................................
soc/nvidia/tegra210: Prevent unintended sign extension
The perennial problem with u16 << 16 strikes again - the u16 is implicitly promoted to an int before the shift, which will then become negative if the highest bit of the u16 was set. Normally this isn't much of a problem, but in this case tegra_dsi_writel() expects a 64 bit integer for that argument, and so it will be sign-extended to a very large unsigned integer if it is negative. Cast bytes to a u32 beforehand to prevent the implicit promotion and thus this problem.
Change-Id: Iaf0fb1040ccafafde0093e9bb192c802b86cb2ac Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1294800 --- M src/soc/nvidia/tegra210/dsi.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/34529/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent unintended sign extension ......................................................................
Patch Set 2: Code-Review+1
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34529 )
Change subject: soc/nvidia/tegra210: Prevent unintended sign extension ......................................................................
soc/nvidia/tegra210: Prevent unintended sign extension
The perennial problem with u16 << 16 strikes again - the u16 is implicitly promoted to an int before the shift, which will then become negative if the highest bit of the u16 was set. Normally this isn't much of a problem, but in this case tegra_dsi_writel() expects a 64 bit integer for that argument, and so it will be sign-extended to a very large unsigned integer if it is negative. Cast bytes to a u32 beforehand to prevent the implicit promotion and thus this problem.
Change-Id: Iaf0fb1040ccafafde0093e9bb192c802b86cb2ac Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1294800 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34529 Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/nvidia/tegra210/dsi.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Julius Werner: Looks good to me, approved
diff --git a/src/soc/nvidia/tegra210/dsi.c b/src/soc/nvidia/tegra210/dsi.c index a383ff2..7d54c9e 100644 --- a/src/soc/nvidia/tegra210/dsi.c +++ b/src/soc/nvidia/tegra210/dsi.c @@ -380,8 +380,8 @@ }
tegra_dsi_writel(dsi, 0, DSI_PKT_LEN_0_1); - tegra_dsi_writel(dsi, bytes << 16, DSI_PKT_LEN_2_3); - tegra_dsi_writel(dsi, bytes << 16, DSI_PKT_LEN_4_5); + tegra_dsi_writel(dsi, (u32)bytes << 16, DSI_PKT_LEN_2_3); + tegra_dsi_writel(dsi, (u32)bytes << 16, DSI_PKT_LEN_4_5); tegra_dsi_writel(dsi, 0, DSI_PKT_LEN_6_7);
value = MIPI_DCS_WRITE_MEMORY_START << 8 |