Change in flashrom[master]: pickit2_spi: Remove dead assignment
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Remove dead assignment ...................................................................... pickit2_spi: Remove dead assignment We never read the first 'ret', so remove it. Also, print the version only when the command succeeded. Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> --- M pickit2_spi.c 1 file changed, 2 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/34403/1 diff --git a/pickit2_spi.c b/pickit2_spi.c index 6d9b28f..4e9ca1d 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -93,17 +93,16 @@ { int ret; uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER}; - int transferred; - ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); - msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); if (ret != 0) { msg_perr("Command Get Firmware Version failed!\n"); return 1; } + msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); return 0; } -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-MessageType: newchange
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Remove dead assignment ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/flashrom/+/34403/1/pickit2_spi.c File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/34403/1/pickit2_spi.c@a98 PS1, Line 98: That we didn't check the return value doesn't mean that the line doesn't do something. It sends the `command` to the Pickit. Without that, we can't expect an answer in the next line. -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 18 Jul 2019 12:06:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Remove dead assignment ...................................................................... Patch Set 1: What we can do: Add a second `ret` variable and check them both: ret = libusb_interrupt_transfer(..., ENDPOINT_OUT, ...); ret2 = libusb_interrupt_transfer(..., ENDPOINT_IN, ...); if (ret != 0 || ret2 != 0) ... or check them individually, so we wouldn't run into the timeout for the second transfer if the first already failed: ret = libusb_interrupt_transfer(..., ENDPOINT_OUT, ...); if (ret != 0) ... ret = libusb_interrupt_transfer(..., ENDPOINT_IN, ...); if (ret != 0) ... -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 1 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 18 Jul 2019 12:10:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34403 to look at the new patch set (#2). Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... pickit2_spi: Fix "dead" assignment We never read the first 'ret'. Let's check the first 'ret' and exit if it failed. Also, print the version only when the command succeeded. Found-by scan-build: 7.0.1-8 Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> --- M pickit2_spi.c 1 file changed, 10 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/34403/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/c/flashrom/+/34403/1/pickit2_spi.c File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/34403/1/pickit2_spi.c@a98 PS1, Line 98:
That we didn't check the return value doesn't mean that the line doesn't do […] Done. Thank you
-- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 18 Jul 2019 12:27:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... Patch Set 2: Code-Review+2 (2 comments) Thanks! https://review.coreboot.org/c/flashrom/+/34403/2//COMMIT_MSG Commit Message: https://review.coreboot.org/c/flashrom/+/34403/2//COMMIT_MSG@10 PS2, Line 10: and exit if it failed. Please separate paragraphs with an empty line (or remove the line break). https://review.coreboot.org/c/flashrom/+/34403/2//COMMIT_MSG@13 PS2, Line 13: Found-by I guess `Found-by` should be followed by a colon. -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 2 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 18 Jul 2019 13:06:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34403 to look at the new patch set (#3). Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... pickit2_spi: Fix "dead" assignment We never read the first 'ret'. Let's check the first 'ret' and exit if it failed. Also, print the version only when the command succeeded. Found-by: scan-build 7.0.1-8 Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> --- M pickit2_spi.c 1 file changed, 10 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/34403/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 3 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/34403 to look at the new patch set (#4). Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... pickit2_spi: Fix "dead" assignment We never read the first 'ret'. Let's check the first 'ret' and exit if it failed. Also, print the version only when the command succeeded. Found-by: scan-build 7.0.1-8 Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> --- M pickit2_spi.c 1 file changed, 10 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/34403/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 4 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... Patch Set 4: Verified+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 4 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 19 Jul 2019 16:17:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/34403 ) Change subject: pickit2_spi: Fix "dead" assignment ...................................................................... pickit2_spi: Fix "dead" assignment We never read the first 'ret'. Let's check the first 'ret' and exit if it failed. Also, print the version only when the command succeeded. Found-by: scan-build 7.0.1-8 Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> Reviewed-on: https://review.coreboot.org/c/flashrom/+/34403 Tested-by: Nico Huber <nico.h@gmx.de> Reviewed-by: Nico Huber <nico.h@gmx.de> --- M pickit2_spi.c 1 file changed, 10 insertions(+), 4 deletions(-) Approvals: Nico Huber: Verified; Looks good to me, approved diff --git a/pickit2_spi.c b/pickit2_spi.c index 6d9b28f..52021d9 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -93,17 +93,23 @@ { int ret; uint8_t command[CMD_LENGTH] = {CMD_GET_VERSION, CMD_END_OF_BUFFER}; - int transferred; - ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); - ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); - msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_OUT, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + if (ret != 0) { msg_perr("Command Get Firmware Version failed!\n"); return 1; } + ret = libusb_interrupt_transfer(pickit2_handle, ENDPOINT_IN, command, CMD_LENGTH, &transferred, DFLT_TIMEOUT); + + if (ret != 0) { + msg_perr("Command Get Firmware Version failed!\n"); + return 1; + } + + msg_pdbg("PICkit2 Firmware Version: %d.%d\n", (int)command[0], (int)command[1]); return 0; } -- To view, visit https://review.coreboot.org/c/flashrom/+/34403 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4aac5e1f3bd0604b079e1fdd9b7f09f1f4fc2d7f Gerrit-Change-Number: 34403 Gerrit-PatchSet: 5 Gerrit-Owner: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: HAOUAS Elyes <ehaouas@noos.fr> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: merged
participants (2)
-
HAOUAS Elyes (Code Review) -
Nico Huber (Code Review)