John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32903
Change subject: soc/intel/apollolake: Fix issue found by Coverity ......................................................................
soc/intel/apollolake: Fix issue found by Coverity
Function soc_init: Value stored to 'gnvs' is never read. ACPI NVS in CBMEM allocation has been implemented in commom/acpi. Remove the redundant cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnve)).
BRANCH=None TEST=Built image successfully.
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 --- M src/soc/intel/apollolake/chip.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/1
diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c index f6880a7..ffe3ed1 100644 --- a/src/soc/intel/apollolake/chip.c +++ b/src/soc/intel/apollolake/chip.c @@ -44,7 +44,6 @@ #include <soc/intel/common/vbt.h> #include <soc/iomap.h> #include <soc/itss.h> -#include <soc/nvs.h> #include <soc/pci_devs.h> #include <soc/pm.h> #include <soc/systemagent.h> @@ -389,8 +388,6 @@
static void soc_init(void *data) { - struct global_nvs_t *gnvs; - /* Snapshot the current GPIO IRQ polarities. FSP is setting a * default policy that doesn't honor boards' requirements. */ itss_snapshot_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END); @@ -417,9 +414,6 @@ */ p2sb_unhide();
- /* Allocate ACPI NVS in CBMEM */ - gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs)); - /* Set RAPL MSR for Package power limits*/ set_power_limits();
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Coverity ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG@13 PS1, Line 13: BRANCH=None List coverity ID number here, no need to mentioned BRANCH here.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Coverity ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG@11 PS1, Line 11: gnve Typo, gnvs
Hello Patrick Rudolph, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#2).
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
soc/intel/apollolake: Fix issue found by Clang Static Analyzer
Function soc_init: Value stored to 'gnvs' is never read. ACPI NVS in CBMEM allocation has been implemented in commom/acpi. Remove the redundant cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs)).
TEST=Built image successfully.
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 --- M src/soc/intel/apollolake/chip.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG@11 PS1, Line 11: gnve
Typo, gnvs
done
https://review.coreboot.org/#/c/32903/1//COMMIT_MSG@13 PS1, Line 13: BRANCH=None
List coverity ID number here, no need to mentioned BRANCH here.
The issue was actually found by Clang Static Analyzer, not by Coverity. I just updated it accordingly.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c@a421 PS2, Line 421: Even though gnvs is not used, the CBMEM space allocated for GNVS is utilized by common code for setting/getting nvs variables.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2: -Code-Review
Remove my +2 until Furquan's comment get addressed.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32903/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/apollolake: Fix issue found by Clang Static Analyzer Please describe the issue here. It’s not useful in the commit message *summary* to know that it was found by a certain tool. This should be mentioned in the commit message body.
So please rephrase. Maybe (no idea if it’s correct):
Remove redundant ACPI GNVS addition to CBMEM
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix issue found by Clang Static Analyzer ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c@a421 PS2, Line 421:
Even though gnvs is not used, the CBMEM space allocated for GNVS is utilized by common code for sett […]
I think what furquan is saying is that we still need to do the cbmem_add, even if we don't assign it to a variable.
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#3).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found value stored to gnvs is never read. Common code southbridge_inject_dsdt already takes of allocating ACPI NVS in CBMEM cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs)). Remove the redundant ACPI GNVS addition to CBMEM.
TEST=Built and boot up to kernel.
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 --- M src/soc/intel/apollolake/chip.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/3
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32903/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/apollolake: Fix issue found by Clang Static Analyzer
Please describe the issue here. […]
Updated. Please let me know whether it is proper description.
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c@a421 PS2, Line 421:
I think what furquan is saying is that we still need to do the cbmem_add, even if we don't assign it […]
Common code southbridge_inject_dsdt already takes care of gnvs. gnvs=cbmem_find(CBMEM_ID_ACPI_GNVS). If gnvs is null, then gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs)) is executed. We can remove the redundant code here.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c@a421 PS2, Line 421:
Common code southbridge_inject_dsdt already takes care of gnvs. gnvs=cbmem_find(CBMEM_ID_ACPI_GNVS). […]
gnvs is used by other blocks in common code as well e.g. uart. Did you verify the order between calls to southbridge_inject_dsdt() and other consumers of gnvs? soc_init is run as part of chip enable which happens before any device enable. It would be good to verify the order.
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#4).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 4: Code-Review+2
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#5).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/5
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/2/src/soc/intel/apollolake/chip.c@a421 PS2, Line 421:
gnvs is used by other blocks in common code as well e.g. uart. […]
The order appears soc_init->uart->southbridge_inject_dsdt. While enabling uart resources, uart refers to GNVS in CBMEM. Since *gnvs is never used here, just changing sizeof(*gnvs) to sizeof(global_nvs_t).
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32903/5/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/5/src/soc/intel/apollolake/chip.c@419 PS5, Line 419: global_nvs_t struct global_nvs_t
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32903/5/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/5/src/soc/intel/apollolake/chip.c@419 PS5, Line 419: global_nvs_t
struct global_nvs_t
Looks like global_nvs_t usage is valid as its structure is defined.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 5: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@4 PS5, Line 4: zhaojohn John Zhao (maybe update Gerrit?)
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@14 PS5, Line 14: Please add a tag
Found-by: Clang static analyzer <version>
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@15 PS5, Line 15: Signed-off-by: John Zhao john.zhao@intel.com Please move this below the Change-Id line.
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#6).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/6
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 6: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 6: Code-Review+2
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#7).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found version 8.0.0 gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/7
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@4 PS5, Line 4: zhaojohn
John Zhao (maybe update Gerrit?)
It seems I can not change it.
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@14 PS5, Line 14:
Please add a tag […]
done.
https://review.coreboot.org/#/c/32903/5//COMMIT_MSG@15 PS5, Line 15: Signed-off-by: John Zhao john.zhao@intel.com
Please move this below the Change-Id line.
done.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 7: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/32903/7/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/7/src/soc/intel/apollolake/chip.c@419 PS7, Line 419: global_nvs_t struct global_nvs_t
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/32903/7/src/soc/intel/apollolake/chip.c File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/#/c/32903/7/src/soc/intel/apollolake/chip.c@419 PS7, Line 419: global_nvs_t
struct global_nvs_t
typedef struct global_nvs_t { .... } __packed global_nvs_t;
It appears valid with global_nvs_t usage.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 7:
Patch Set 7:
(1 comment)
using typedefs for pointers or structs is generally discouraged.
Hello Patrick Rudolph, Balaji Manigandan, Lance Zhao, build bot (Jenkins), Furquan Shaikh, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32903
to look at the new patch set (#8).
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found version 8.0.0 gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/32903/8
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32903 )
Change subject: soc/intel/apollolake: Fix value stored to gnvs is never read ......................................................................
soc/intel/apollolake: Fix value stored to gnvs is never read
Clang Static Analyzer found version 8.0.0 gnvs is allocated, but it is never used. Change sizeof(*gnvs) to sizeof(global_nvs_t) while adding ACPI GNVS to CBMEM.
TEST=Built and boot up to kernel.
Change-Id: Ie9421af4a556d1d88183aa938ee2a124a10ab727 Signed-off-by: John Zhao john.zhao@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32903 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/apollolake/chip.c 1 file changed, 1 insertion(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c index f6880a7..e9030dc 100644 --- a/src/soc/intel/apollolake/chip.c +++ b/src/soc/intel/apollolake/chip.c @@ -389,8 +389,6 @@
static void soc_init(void *data) { - struct global_nvs_t *gnvs; - /* Snapshot the current GPIO IRQ polarities. FSP is setting a * default policy that doesn't honor boards' requirements. */ itss_snapshot_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END); @@ -418,7 +416,7 @@ p2sb_unhide();
/* Allocate ACPI NVS in CBMEM */ - gnvs = cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(*gnvs)); + cbmem_add(CBMEM_ID_ACPI_GNVS, sizeof(struct global_nvs_t));
/* Set RAPL MSR for Package power limits*/ set_power_limits();
-
Arthur Heymans (Code Review) -
Furquan Shaikh (Code Review)
-
Jacob Garber (Code Review)
-
John Zhao (Code Review)
-
Lance Zhao (Code Review)
-
Lijian Zhao (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)