Change in ...coreboot[master]: drivers/spi/spi_flash.c: Avoid static scan false positive

[Richard Spiegel (Code Review)]

Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31473

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

drivers/spi/spi_flash.c: Avoid static scan false positive

Static scan-build indicates a possible invalid return from function
spi_flash_cmd_erase(). The root cause is because the scan believes it's
possible for offset to be above the end address in the first pass, thus
not setting a value for variable ret. By making sure that input len
can't be negative we instruct the scan that the loop will be executed at
least once.

BUG=b:112253891
TEST=build grunt

Change-Id: If548728ff90b755c69143eabff6aeff01e8fd483
Signed-off-by: Richard Spiegel 
---
M src/drivers/spi/spi_flash.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/31473/1

diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c
index 204f607..5b5b319 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -204,14 +204,14 @@
    	printk(BIOS_WARNING, "SF: Erase offset/length not multiple of erase size\n");
    	return -1;
    }
-	if (len == 0) {
+	if (len <= 0) {
    	printk(BIOS_WARNING, "SF: Erase length cannot be 0\n");
    	return -1;
    }

cmd[0] = flash->erase_cmd;
    start = offset;
-	end = start + len;
+	end = offset + len;

while (offset < end) {
    	spi_flash_addr(offset, cmd);

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31473 )

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c 
File src/drivers/spi/spi_flash.c:

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@199 
PS1, Line 199: ;
If this is set to -1 here, wouldn't it make the static checker happy?

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@207 
PS1, Line 207: <
len is of type size_t. How does it end up being less than 0?

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@214 
PS1, Line 214: offset
Isn't offset the same as start? I don't understand the change here?

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31473 )

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

Patch Set 1:

(3 comments)

Though I'm sure my way would solve the issue, I liked the idea of presetting ret to -1 and will follow it.

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c 
File src/drivers/spi/spi_flash.c:

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@199 
PS1, Line 199: ;
> If this is set to -1 here, wouldn't it make the static checker happy?

Probably

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@207 
PS1, Line 207: <
> len is of type size_t. […]

I know, you know, but somehow static scan does not.

https://review.coreboot.org/#/c/31473/1/src/drivers/spi/spi_flash.c@214 
PS1, Line 214: offset
> Isn't offset the same as start? I don't understand the change here?

Because comparison is between end and offset and len now has to be positive, I explicitly tell the static check that end must start bigger then offset. I agree I could use start as originally, but this way I make it explicit.

[Richard Spiegel (Code Review)]

Hello Marshall Dawson, build bot (Jenkins), Martin Roth, Patrick Georgi,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31473

to look at the new patch set (#2).

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

drivers/spi/spi_flash.c: Avoid static scan false positive

Static scan-build indicates a possible invalid return from function
spi_flash_cmd_erase(). The root cause is because the scan believes it's
possible for offset to be above the end address in the first pass, thus
not setting a value for variable ret. Assign initial value of -1 to
variable ret to make checker happy.

BUG=b:112253891
TEST=build grunt

Change-Id: If548728ff90b755c69143eabff6aeff01e8fd483
Signed-off-by: Richard Spiegel 
---
M src/drivers/spi/spi_flash.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/31473/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31473 )

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31473 )

Change subject: drivers/spi/spi_flash.c: Avoid static scan false positive
......................................................................

drivers/spi/spi_flash.c: Avoid static scan false positive

Static scan-build indicates a possible invalid return from function
spi_flash_cmd_erase(). The root cause is because the scan believes it's
possible for offset to be above the end address in the first pass, thus
not setting a value for variable ret. Assign initial value of -1 to
variable ret to make checker happy.

BUG=b:112253891
TEST=build grunt

Change-Id: If548728ff90b755c69143eabff6aeff01e8fd483
Signed-off-by: Richard Spiegel 
Reviewed-on: https://review.coreboot.org/c/31473
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/drivers/spi/spi_flash.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c
index 204f607..d1f5227 100644
--- a/src/drivers/spi/spi_flash.c
+++ b/src/drivers/spi/spi_flash.c
@@ -196,7 +196,7 @@
 int spi_flash_cmd_erase(const struct spi_flash *flash, u32 offset, size_t len)
 {
    u32 start, end, erase_size;
-	int ret;
+	int ret = -1;
    u8 cmd[4];

erase_size = flash->sector_size;