Change in coreboot[master]: Fix control flow DEADCODE issue

List overview All Threads
Download

newer

older

[S] Change in coreboot[master]:...

Change in coreboot[master]:...

John Zhao (Code Review)

22 Sep 2020 22 Sep '20

5:10 p.m.

John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: Fix control flow DEADCODE issue ......................................................................

Fix control flow DEADCODE issue

Coverity detects DEADCODE issue in the control flow. Based on both of KEMPLD_CLK and KEMPLD_I2C_FREQ_STD definitions, execution cannot reach this statement "prescale = 0". This change removes the DEADCODE code.

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 0 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/1

diff --git a/src/ec/kontron/kempld/kempld_i2c.c b/src/ec/kontron/kempld/kempld_i2c.c index 296cf76..165c940 100644 --- a/src/ec/kontron/kempld/kempld_i2c.c +++ b/src/ec/kontron/kempld/kempld_i2c.c @@ -250,9 +250,6 @@ else prescale = KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 4) - 3000;

- if (prescale < 0) - prescale = 0; - /* Round to the best matching value */ prescale_corr = prescale / 1000; if (prescale % 1000 >= 500)

Attachments:

attachment.htm (text/html — 2.7 KB)

Show replies by date

John Zhao (Code Review)

22 Sep 22 Sep

5:14 p.m.

Hello Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: Fix control flow DEADCODE issue ......................................................................

Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 0 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/2

John Zhao (Code Review)

5:14 p.m.

Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: Fix control flow DEADCODE issue ......................................................................

Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 0 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/3

John Zhao (Code Review)

5:20 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

ec/kontron: Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 0 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/4

Tim Wawrzynczak (Code Review)

5:40 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

Seems like it might be simpler to have something like #if KEMPLD_CLK < 0 || KEMPLD_I2C_FREQ_STD < 0 #error Check your KEMPLD_CLK and KEMPLD_I2C_FREQ_STD definitions #endif

That would avoid the dead code and still prevent negative prescaler values here. what do you think?

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 4 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 20:40:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

6:10 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

...

Patch Set 4:

Seems like it might be simpler to have something like #if KEMPLD_CLK < 0 || KEMPLD_I2C_FREQ_STD < 0 #error Check your KEMPLD_CLK and KEMPLD_I2C_FREQ_STD definitions #endif

That would avoid the dead code and still prevent negative prescaler values here. what do you think?

Apart from the signs of KEMPLD_CLK and KEMPLD_I2C_FREQ_STD, prescale is determined by the following calculation "KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 5) - 1000" or "KEMPLD_CLK / (KEMPLD_I2C_FREQ_STD * 4) - 3000". Based on current KEMPLD_CLK & KEMPLD_I2C_FREQ_STD definitions, prescale won't be negative which is why Coverity complains about the DEADCODE. It seems checking the signs of KEMPLD_CLK and KEMPLD_I2C_FREQ_STD does not help to address this DEADCODE issue.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 4 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 21:10:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

7:22 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 4: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... PS4, Line 253: if (prescale < 0) This is a safeguard in case prescale ends up being negative. I'd replace it with an assertion.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 4 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 22:22:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

7:28 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

(1 comment)

Please never remove checks because of tools.

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... PS4, Line 253: if (prescale < 0)

...

This is a safeguard in case prescale ends up being negative. I'd replace it with an assertion.

I guess I kept it in case the constants would change (it is probably runtime configurable in the Linux driver). A _Static_assertion() would make most sense here I guess.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 4 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 22:28:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

7:33 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

(1 comment)

...

Patch Set 4:

(1 comment)

Please never remove checks because of tools.

Instead, try to find a more suitable kind of check.

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... PS4, Line 253: if (prescale < 0)

...

I guess I kept it in case the constants would change (it is probably runtime […]

Sounds good. Regular `assert()` now provides compile-time checks if possible, too.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 4 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 22:33:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

John Zhao (Code Review)

8:12 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

ec/kontron: Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/5

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 5 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

John Zhao (Code Review)

8:13 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/4/src/ec/kontron/kempld/kempl... PS4, Line 253: if (prescale < 0)

...

Sounds good. Regular `assert()` now provides compile-time checks if possible, too.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 5 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 23:13:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

8:23 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45623/5//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45623/5//COMMIT_MSG@11 PS5, Line 11: This change removes this DEADCODE. This change makes Coverity happy by using an assertion instead.

https://review.coreboot.org/c/coreboot/+/45623/5/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/5/src/ec/kontron/kempld/kempl... PS5, Line 253: assert(prescale < 0); Um, no. We want `prescale` to be larger than or equal to 0.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 5 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 23:23:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

8:35 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

ec/kontron: Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 6 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

John Zhao (Code Review)

8:37 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45623/5//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45623/5//COMMIT_MSG@11 PS5, Line 11: This change removes this DEADCODE.

...

This change makes Coverity happy by using an assertion instead.

Done

https://review.coreboot.org/c/coreboot/+/45623/5/src/ec/kontron/kempld/kempl... File src/ec/kontron/kempld/kempld_i2c.c:

https://review.coreboot.org/c/coreboot/+/45623/5/src/ec/kontron/kempld/kempl... PS5, Line 253: assert(prescale < 0);

...

Um, no. We want `prescale` to be larger than or equal to 0.

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 6 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 22 Sep 2020 23:37:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

John Zhao (Code Review)

8:37 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

ec/kontron: Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/7

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 7 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Angel Pons (Code Review)

23 Sep 23 Sep

8:41 a.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45623/7//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45623/7//COMMIT_MSG@11 PS7, Line 11: asserts check IMHO, this doesn't parse well. Also, the sentence I suggested hints at Coverity not being smart enough about code safety:

This change makes Coverity happy by using an assertion instead.

Let's not forget that, while tools can be helpful, they can also cause lots of pain. Have you ever hit your fingers while trying to hammer a nail? 😉

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 7 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 23 Sep 2020 11:41:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

8:43 a.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 7: Code-Review+2

Other than the comment on the commit message (which is important), LGTM.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 7 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 23 Sep 2020 11:43:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

John Zhao (Code Review)

3:17 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Tim Wawrzynczak, Angel Pons,

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

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

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

ec/kontron: Fix control flow DEADCODE issue

Found-by: Coverity CID 1431154 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a --- M src/ec/kontron/kempld/kempld_i2c.c 1 file changed, 1 insertion(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/45623/8

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

John Zhao (Code Review)

3:18 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45623/7//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45623/7//COMMIT_MSG@11 PS7, Line 11: asserts check

...

IMHO, this doesn't parse well. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 23 Sep 2020 18:18:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

4:44 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 8: Code-Review+2

Thank you!

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 23 Sep 2020 19:44:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

24 Sep 24 Sep

4:35 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45623 )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 8: Code-Review-1

I strongly advice against using assert() like this. If we know, event want, that the check is done at build time, we should use _Static_assertion(). Beside being clear to the reader it also allows us to specify a proper error message.

Do we even know if using assert() (which only does the build- time check with our custom implementation) will calm Coverity? iow. is it smart enough to detect that this is no usual assert()?

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 24 Sep 2020 19:35:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

4:48 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 8:

...

Patch Set 8: Code-Review-1

I strongly advice against using assert() like this. If we know, event want, that the check is done at build time, we should use _Static_assertion(). Beside being clear to the reader it also allows us to specify a proper error message.

Do we even know if using assert() (which only does the build- time check with our custom implementation) will calm Coverity? iow. is it smart enough to detect that this is no usual assert()?

There's CB:44476 which would probably not work if using a _Static_assert().

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 24 Sep 2020 19:48:54 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

4:49 p.m.

New subject: Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

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

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Patch Set 8: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 24 Sep 2020 19:49:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Stefan Reinauer (Code Review)

10 Jun 10 Jun

12:07 a.m.

New subject: [XS] Change in coreboot[master]: ec/kontron: Fix control flow DEADCODE issue

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45623?usp=email )

Change subject: ec/kontron: Fix control flow DEADCODE issue ......................................................................

Abandoned

-- To view, visit https://review.coreboot.org/c/coreboot/+/45623?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic002e708636961358969b2c1eaec0fee5bbcb73a Gerrit-Change-Number: 45623 Gerrit-PatchSet: 8 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: abandon

382

days inactive

1373

days old

coreboot-gerrit@coreboot.org

23 comments

5 participants

tags (0)

participants (5)

Angel Pons (Code Review)
John Zhao (Code Review)
Nico Huber (Code Review)
Stefan Reinauer (Code Review)
Tim Wawrzynczak (Code Review)