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

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

John Zhao (Code Review)

16 Oct 2020 16 Oct '20

7:49 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

drivers/uart: Fix control flow DEADCODE issue

Coverity detects the irq_gpio_index DEADCODE which is initialized as -1. This change updates irq_gpio_index based on irq_gpio.pin_count which differentiates gpio irq or pirq.

Found-by: Coverity CID 1429983 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 --- M src/drivers/uart/acpi/acpi.c 1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/46506/1

diff --git a/src/drivers/uart/acpi/acpi.c b/src/drivers/uart/acpi/acpi.c index f9d9d8f..df26f4e 100644 --- a/src/drivers/uart/acpi/acpi.c +++ b/src/drivers/uart/acpi/acpi.c @@ -73,8 +73,10 @@ acpi_device_write_uart(&config->uart);

/* Use either Interrupt() or GpioInt() */ - if (config->irq_gpio.pin_count) + if (config->irq_gpio.pin_count) { acpi_device_write_gpio(&config->irq_gpio); + irq_gpio_index++; + } else acpi_device_write_interrupt(&config->irq);

Attachments:

attachment.htm (text/html — 2.8 KB)

Show replies by date

Angel Pons (Code Review)

16 Oct 16 Oct

8:08 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... PS1, Line 78: irq_gpio_index++; Why increment it?

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... PS1, Line 80: else The `else` should have braces too.

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 1 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: Fri, 16 Oct 2020 20:08:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

8:12 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/+/46506

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

drivers/uart: Fix control flow DEADCODE issue

Coverity detects the irq_gpio_index DEADCODE which is initialized as -1. This change updates irq_gpio_index based on irq_gpio.pin_count which differentiates gpio irq or pirq.

Found-by: Coverity CID 1429983 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 --- M src/drivers/uart/acpi/acpi.c 1 file changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/46506/2

build bot (Jenkins) (Code Review)

8:13 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46506 )

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... PS2, Line 80: else { else should follow close brace '}'

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 2 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: Fri, 16 Oct 2020 20:13:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

8:18 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... PS1, Line 78: irq_gpio_index++;

...

Why increment it?

The irq_gpio_index increment is similar to reset_gpio_index and enable_gpio_index handling in uart_acpi_write_gpio as long as gpio->pin_count is not 0.

https://review.coreboot.org/c/coreboot/+/46506/1/src/drivers/uart/acpi/acpi.... PS1, Line 80: else

...

The `else` should have braces too.

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 2 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: Fri, 16 Oct 2020 20:18:56 +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:26 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/+/46506

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

drivers/uart: Fix control flow DEADCODE issue

Coverity detects the irq_gpio_index DEADCODE which is initialized as -1. This change updates irq_gpio_index based on irq_gpio.pin_count which differentiates gpio irq or pirq.

Found-by: Coverity CID 1429983 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 --- M src/drivers/uart/acpi/acpi.c 1 file changed, 3 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/46506/3

Tim Wawrzynczak (Code Review)

8:27 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... PS2, Line 77: acpi_device_write_gpio(&config->irq_gpio); : irq_gpio_index++; Reading back over what this is doing, even though this is the first pin added, it's probably best to do what the other pins are doing, e.g.: ``` irq_gpio_index = uart_acpi_write_gpio(&config->irq_gpio, &curr_index); ``` since curr_index is used as state between calls to uart_acpi_write_gpio

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 2 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: Fri, 16 Oct 2020 20:27:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

8:45 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/+/46506

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

drivers/uart: Fix control flow DEADCODE issue

Coverity detects the irq_gpio_index DEADCODE which is initialized as -1. This change updates irq_gpio_index based on irq_gpio.pin_count which differentiates gpio irq or pirq.

Found-by: Coverity CID 1429983 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 --- M src/drivers/uart/acpi/acpi.c 1 file changed, 4 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/46506/4

John Zhao (Code Review)

8:46 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/2/src/drivers/uart/acpi/acpi.... PS2, Line 77: acpi_device_write_gpio(&config->irq_gpio); : irq_gpio_index++;

...

Reading back over what this is doing, even though this is the first pin added, it's probably best to […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 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: Fri, 16 Oct 2020 20:46:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Paul Menzel (Code Review)

9 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46506/4/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/4/src/drivers/uart/acpi/acpi.... PS4, Line 81: acpi_device_write_interrupt(&config->irq); From https://doc.coreboot.org/coding_style.html#placing-braces-and-spaces:

...

This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches:

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 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-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 16 Oct 2020 21:00:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

John Zhao (Code Review)

9:22 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/+/46506

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

drivers/uart: Fix control flow DEADCODE issue

Coverity detects the irq_gpio_index DEADCODE which is initialized as -1. This change updates irq_gpio_index based on irq_gpio.pin_count which differentiates gpio irq or pirq.

Found-by: Coverity CID 1429983 TEST=None

Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 --- M src/drivers/uart/acpi/acpi.c 1 file changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/46506/5

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 5 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-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

John Zhao (Code Review)

9:23 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46506/4/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/4/src/drivers/uart/acpi/acpi.... PS4, Line 81: acpi_device_write_interrupt(&config->irq);

...

From https://doc.coreboot.org/coding_style.html#placing-braces-and-spaces: […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 5 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-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 16 Oct 2020 21:23:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

19 Oct 19 Oct

5:25 p.m.

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Patch Set 5:

(1 comment)

Furquan, Duncan, is this a bug in this driver or

https://review.coreboot.org/c/coreboot/+/46506/5/src/drivers/uart/acpi/acpi.... File src/drivers/uart/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/46506/5/src/drivers/uart/acpi/acpi.... PS5, Line 75: /* Use either Interrupt() or GpioInt() */ : if (config->irq_gpio.pin_count) { : acpi_device_write_gpio(&config->irq_gpio); : irq_gpio_index = uart_acpi_write_gpio(&config->irq_gpio, : &curr_index); : } else { : acpi_device_write_interrupt(&config->irq); : } : : /* Add enable/reset GPIOs if needed */ : if (uart_acpi_add_gpios_to_crs(config)) { : reset_gpio_index = uart_acpi_write_gpio(&config->reset_gpio, : &curr_index); : enable_gpio_index = uart_acpi_write_gpio(&config->enable_gpio, : &curr_index); : } Actually, I see the GPIOs are only added conditionally to _CRS, so I think it would actually need to be: ``` /* Use either Interrupt() or GpioInt() */ if (!config->irq_gpio.pin_count) acpi_device_write_interrupt(&config->irq);

/* Add enable/reset GPIOs if needed */ if (uart_acpi_add_gpios_to_crs(config)) { irq_gpio_index = uart_acpi_write_gpio(&config->irq_gpio, &curr_index); reset_gpio_index = uart_acpi_write_gpio(&config->reset_gpio, &curr_index); if (config->irq_gpio.pin_count) enable_gpio_index = uart_acpi_write_gpio(&config->enable_gpio, &curr_index); } ```

Furquan, Duncan, does this look right?

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 5 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 19 Oct 2020 17:25:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Stefan Reinauer (Code Review)

10 Jun 10 Jun

3:07 a.m.

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

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

Change subject: drivers/uart: Fix control flow DEADCODE issue ......................................................................

Abandoned

-- To view, visit https://review.coreboot.org/c/coreboot/+/46506?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: I79f85f05b78e5569615ae4c4f7c81cc85c3999c9 Gerrit-Change-Number: 46506 Gerrit-PatchSet: 5 Gerrit-Owner: John Zhao john.zhao@intel.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Duncan L Gerrit-Reviewer: Furquan Shaikh furquan.m.shaikh@gmail.com Gerrit-Reviewer: Tim Wawrzynczak inforichland@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-MessageType: abandon

477

days inactive

1444

days old

coreboot-gerrit@coreboot.org

13 comments

6 participants

tags (0)

participants (6)

Angel Pons (Code Review)
build bot (Jenkins) (Code Review)
John Zhao (Code Review)
Paul Menzel (Code Review)
Stefan Reinauer (Code Review)
Tim Wawrzynczak (Code Review)