Change in coreboot[master]: drivers/mrc_cache: Avoid unused variable assignment

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Paul Menzel (Code Review)

25 Jun 2020 25 Jun '20

9:21 a.m.

Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

drivers/mrc_cache: Avoid unused variable assignment

Fix the scan-build warning below:

CC romstage/drivers/mrc_cache/mrc_cache.o src/drivers/mrc_cache/mrc_cache.c:450:26: warning: Value stored to 'flash' during its initialization is never read const struct spi_flash *flash = boot_device_spi_flash(); ^~~~~ ~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated.

The function can return early before the value is read. Fix this, by getting rid of the variable, as the value is only read once.

Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Found-by: scan-build (clang-tools-9 1:9.0.1-12) Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/drivers/mrc_cache/mrc_cache.c 1 file changed, 1 insertion(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/42798/1

diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index 3a005db..d567a20 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -447,15 +447,13 @@ /* Apply protection to a range of flash */ static int nvm_protect(const struct region *r) { - const struct spi_flash *flash = boot_device_spi_flash(); - if (!CONFIG(MRC_SETTINGS_PROTECT)) return 0;

if (!CONFIG(BOOT_DEVICE_SPI_FLASH)) return 0;

- return spi_flash_ctrlr_protect_region(flash, r, WRITE_PROTECT); + return spi_flash_ctrlr_protect_region(boot_device_spi_flash(), r, WRITE_PROTECT); }

/* Protect mrc region with a Protected Range Register */

Attachments:

attachment.htm (text/html — 3.4 KB)

Show replies by date

Angel Pons (Code Review)

25 Jun 25 Jun

2:38 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... PS1, Line 456: boot_device_spi_flash Note that this function has side effects. Calling it here means that it won't run in some cases.

Paul Menzel (Code Review)

3:26 p.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... PS1, Line 456: boot_device_spi_flash

...

Note that this function has side effects. Calling it here means that it won't run in some cases.

You mean, in case the two if statements above are true?

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Thu, 25 Jun 2020 13:26:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

3:37 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... PS1, Line 456: boot_device_spi_flash

...

You mean, in case the two if statements above are true?

No. Previously, every call to "nvm_protect" would call `boot_device_spi_flash`. Now, calls to "nvm_protect" will only call "boot_device_spi_flash" if the two checks are false. Would be good to know if this makes any difference.

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Thu, 25 Jun 2020 13:37:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Paul Menzel (Code Review)

26 Jun 26 Jun

2:40 p.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... PS1, Line 456: boot_device_spi_flash

...

No. Previously, every call to "nvm_protect" would call `boot_device_spi_flash`. […]

Sorry, I meant *false* indeed.

`boot_device_init()` is called by that function. I believe the SPI flash API ensures, the device is initialized each time.

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Fri, 26 Jun 2020 12:40:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Paul Menzel (Code Review)

28 Jun 28 Jun

9:59 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

Strangely, for comparing with `BUILD_TIMELESS=1`, building the Lenovo ThinkPad X201 with `CONFIG_MRC_SETTINGS_PROTECT` selected, the object `mrc_cache.o` differs, but it’s stripped by the linker for `coreboot.rom`.

``` $ more defconfig CONFIG_VENDOR_LENOVO=y CONFIG_BOARD_LENOVO_X201=y CONFIG_NO_GFX_INIT=y CONFIG_MRC_SETTINGS_PROTECT=y CONFIG_PAYLOAD_NONE=y ```

``` $ LANG=C diff -ur build-1 build-2 Binary files build-1/cbfs/fallback/ramstage.debug and build-2/cbfs/fallback/ramstage.debug differ Binary files build-1/cbfs/fallback/romstage.debug and build-2/cbfs/fallback/romstage.debug differ Binary files build-1/cbfs/fallback/romstage.elf and build-2/cbfs/fallback/romstage.elf differ diff -ur build-1/dsdt.dsl build-2/dsdt.dsl --- build-1/dsdt.dsl 2020-06-28 09:45:38.065799763 +0200 +++ build-2/dsdt.dsl 2020-06-28 09:36:09.835617515 +0200 @@ -5,7 +5,7 @@ * * Disassembling to symbolic ASL+ operators * - * Disassembly of build/dsdt.aml, Sun Jun 28 07:45:38 2020 + * Disassembly of build/dsdt.aml, Sun Jun 28 07:36:09 2020 * * Original Table Header: * Signature "DSDT" Binary files build-1/generated/ramstage.o and build-2/generated/ramstage.o differ Only in build-1/mainboard/lenovo/x201: cbfs-file.3v1XCM Only in build-1/mainboard/lenovo/x201: cbfs-file.3v1XCM.out Only in build-2/mainboard/lenovo/x201: cbfs-file.frB3XA Only in build-2/mainboard/lenovo/x201: cbfs-file.frB3XA.out Binary files build-1/ramstage/drivers/mrc_cache/mrc_cache.o and build-2/ramstage/drivers/mrc_cache/mrc_cache.o differ Binary files build-1/ramstage/drivers/mrc_cache/ramstage.a and build-2/ramstage/drivers/mrc_cache/ramstage.a differ Binary files build-1/romstage/drivers/mrc_cache/mrc_cache.o and build-2/romstage/drivers/mrc_cache/mrc_cache.o differ ```

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Sun, 28 Jun 2020 07:59:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

12:37 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1:

...

Patch Set 1:

Strangely, for comparing with `BUILD_TIMELESS=1`, building the Lenovo ThinkPad X201 with `CONFIG_MRC_SETTINGS_PROTECT` selected, the object `mrc_cache.o` differs, but it’s stripped by the linker for `coreboot.rom`.

*snip*

Well, does moving the local variable declaration after the two conditional statements also change the binary?

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Sun, 28 Jun 2020 10:37:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Patrick Rudolph (Code Review)

29 Jun 29 Jun

8:01 a.m.

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 1: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Mon, 29 Jun 2020 06:01:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

23 Jul 23 Jul

4:50 p.m.

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/42798/1/src/drivers/mrc_cache/mrc_c... PS1, Line 456: boot_device_spi_flash

...

Sorry, I meant *false* indeed. […]

We can discuss the purity of `boot_device_spi_flash` in another commit.

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-Comment-Date: Thu, 23 Jul 2020 14:50:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

26 Jul 26 Jul

11:26 p.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42798 )

Change subject: drivers/mrc_cache: Avoid unused variable assignment ......................................................................

drivers/mrc_cache: Avoid unused variable assignment

Fix the scan-build warning below:

The function can return early before the value is read. Fix this, by getting rid of the variable, as the value is only read once.

Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Found-by: scan-build (clang-tools-9 1:9.0.1-12) Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/42798 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/drivers/mrc_cache/mrc_cache.c 1 file changed, 1 insertion(+), 3 deletions(-)

Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved

if (!CONFIG(BOOT_DEVICE_SPI_FLASH)) return 0;

- return spi_flash_ctrlr_protect_region(flash, r, WRITE_PROTECT); + return spi_flash_ctrlr_protect_region(boot_device_spi_flash(), r, WRITE_PROTECT); }

/* Protect mrc region with a Protected Range Register */

-- To view, visit https://review.coreboot.org/c/coreboot/+/42798 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3c94b123f4994eed9d7568b63971fd5b1d94bc09 Gerrit-Change-Number: 42798 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: merged

1563

days inactive

1594

days old

coreboot-gerrit@coreboot.org

9 comments

4 participants

tags (0)

participants (4)

Angel Pons (Code Review)
Patrick Georgi (Code Review)
Patrick Rudolph (Code Review)
Paul Menzel (Code Review)