John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
drivers/usb: Avoid value overwritten before its usage
Coverity detects that value assigned to variable "ret" is overwritten before it is used. Fix the issue by returning right value.
Found-by: Coverity CID 1255942, 1241836 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I2e1fb5400ff64c6178bb30601896780f8d67b5c6 --- M src/drivers/usb/ehci_debug.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/44185/1
diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 5998172..3a52cca 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -581,7 +581,7 @@ ctrl = read32(&ehci_debug->control); ctrl &= ~(DBGP_CLAIM | DBGP_OUT); write32(&ehci_debug->control, ctrl); - //return ret; + return ret;
next_debug_port: if (CONFIG_USBDEBUG_DEFAULT_PORT == 0) {
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44185/1/src/drivers/usb/ehci_debug.... File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/c/coreboot/+/44185/1/src/drivers/usb/ehci_debug.... PS1, Line 603: return -10; I'd put `return ret;` here instead, to not alter the behavior of the code. I suspect the return statement was commented-out for some reason
Hello build bot (Jenkins), Angel Pons, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44185
to look at the new patch set (#2).
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
drivers/usb: Avoid value overwritten before its usage
Coverity detects that value assigned to variable "ret" is overwritten before it is used. Fix the issue by returning right value.
Found-by: Coverity CID 1255942, 1241836 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I2e1fb5400ff64c6178bb30601896780f8d67b5c6 --- M src/drivers/usb/ehci_debug.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/44185/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44185/1/src/drivers/usb/ehci_debug.... File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/c/coreboot/+/44185/1/src/drivers/usb/ehci_debug.... PS1, Line 603: return -10;
I'd put `return ret;` here instead, to not alter the behavior of the code. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 2: Code-Review+1
git blame shows that `//return ret` line has been there for 9 years... and it sure looks wrong.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 2: Code-Review+1
Patch Set 2: Code-Review+1
git blame shows that `//return ret` line has been there for 9 years... and it sure looks wrong.
I wonder if this patch as-is breaks the try_next_port logic.
Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44185
to look at the new patch set (#3).
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
drivers/usb: Avoid value overwritten before its usage
Coverity detects that value assigned to variable "ret" is overwritten before it is used. Fix the issue by returning right value.
Found-by: Coverity CID 1255942, 1241836 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I2e1fb5400ff64c6178bb30601896780f8d67b5c6 --- M src/drivers/usb/ehci_debug.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/44185/3
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+1
Patch Set 2: Code-Review+1
git blame shows that `//return ret` line has been there for 9 years... and it sure looks wrong.
I wonder if this patch as-is breaks the try_next_port logic.
Then what about keeping "//return ret"? It would keep original logic and get rid of the defect as detected by Coverity.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3:
Maybe mention, that currently the (negative) return value does not seem to matter.
Would a follow-up be good to print the value in the debug message?
if (rv < 0) printk(BIOS_DEBUG, "usbdebug: Failed hardware init\n");
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG@7 PS3, Line 7: drivers/usb: Avoid value overwritten before its usage Maybe:
drivers/usb/ehci_debug: Replace return value -10 by variable
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3:
Patch Set 3:
Maybe mention, that currently the (negative) return value does not seem to matter.
Would a follow-up be good to print the value in the debug message?
if (rv < 0) printk(BIOS_DEBUG, "usbdebug: Failed hardware init\n");
There's a Kconfig option to debug console initialisation. Plus, if the console was unable to initialise, why print a message? I think there's a message like that already.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Maybe mention, that currently the (negative) return value does not seem to matter.
Would a follow-up be good to print the value in the debug message?
if (rv < 0) printk(BIOS_DEBUG, "usbdebug: Failed hardware init\n");
There's a Kconfig option to debug console initialisation. Plus, if the console was unable to initialise, why print a message?
There could be working other consoles like serial console.
I think there's a message like that already.
Sorry for the misunderstanding. I meant extending the current message.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Avoid value overwritten before its usage ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG@7 PS3, Line 7: drivers/usb: Avoid value overwritten before its usage
Maybe: […]
replace *with*
Hello build bot (Jenkins), Tim Wawrzynczak, Angel Pons, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44185
to look at the new patch set (#4).
Change subject: drivers/usb: Replace return value -10 with variable ......................................................................
drivers/usb: Replace return value -10 with variable
Coverity detects that value assigned to variable "ret" is overwritten before it is used. Fix the issue by returning right value.
Found-by: Coverity CID 1255942, 1241836 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I2e1fb5400ff64c6178bb30601896780f8d67b5c6 --- M src/drivers/usb/ehci_debug.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/44185/4
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Replace return value -10 with variable ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44185/3//COMMIT_MSG@7 PS3, Line 7: drivers/usb: Avoid value overwritten before its usage
replace *with*
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44185 )
Change subject: drivers/usb: Replace return value -10 with variable ......................................................................
drivers/usb: Replace return value -10 with variable
Coverity detects that value assigned to variable "ret" is overwritten before it is used. Fix the issue by returning right value.
Found-by: Coverity CID 1255942, 1241836 TEST=None
Signed-off-by: John Zhao john.zhao@intel.com Change-Id: I2e1fb5400ff64c6178bb30601896780f8d67b5c6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44185 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/usb/ehci_debug.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Tim Wawrzynczak: Looks good to me, but someone else must approve
diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index 5998172..ab76f3b 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -600,7 +600,7 @@ goto try_next_time; }
- return -10; + return ret; }
static int dbgp_enabled(void)
-
Angel Pons (Code Review) -
John Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)