Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 )
Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 3:
(2 comments)
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/52958/comment/a8785a3c_e81c42ef
PS3, Line 58: unsigned char **bp_commbuf
> does this actually need double indirection?
I think so, because the pointer can change after realloc (and the size of the buffer can also change) - but please correct me if I am wrong.
https://review.coreboot.org/c/flashrom/+/52958/comment/ca94ff25_803e2b85
PS3, Line 221: unsigned char *const bp_commbuf = bp_data->bp_commbuf;
> perhaps just move this up a few lines to the functions preamble so that the branch line above can be […]
I thought that buspirate_commbuf_grow can change the pointer, and I can have this local variable as constant pointer after buspirate_commbuf_grow.
Both your comments have the same answer, depending on whether realloc can change the pointer?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52958
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8
Gerrit-Change-Number: 52958
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 25 May 2021 05:21:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54889 )
Change subject: spi_master: Make use of new register_spi_master() API
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54889/comment/c232b097_fee78adf
PS1, Line 14: TEST=builds
> can you test raiden debug with a servo micro board since you have access to the hardware?
Yes sure, let's leave it unresolved and I will reply to the comment after I test with hardware.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54889
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If72f54c28a95b402b3565fd14ea481f734e1c970
Gerrit-Change-Number: 54889
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 25 May 2021 05:12:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 )
Change subject: programmer: Introduce default shutdown function
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54890/comment/ea8517a2_c0333805
PS1, Line 13: lot of par masters that
: will use it in the nearest future.
> lets rebase this once we have a few more users rather than too prematurely introducing it. […]
Yes sure, my main goal was to get some comments, thank you for comment!
In terms of more users, there are 10 out of 15 par masters which would use this.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517
Gerrit-Change-Number: 54890
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Tue, 25 May 2021 05:11:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54889 )
Change subject: spi_master: Make use of new register_spi_master() API
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54889/comment/a19905a3_dbbb0714
PS1, Line 14: TEST=builds
can you test raiden debug with a servo micro board since you have access to the hardware?
--
To view, visit https://review.coreboot.org/c/flashrom/+/54889
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If72f54c28a95b402b3565fd14ea481f734e1c970
Gerrit-Change-Number: 54889
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 25 May 2021 05:10:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52958 )
Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 3: Code-Review+1
(2 comments)
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/52958/comment/2867bc0d_94923a74
PS3, Line 58: unsigned char **bp_commbuf
does this actually need double indirection?
https://review.coreboot.org/c/flashrom/+/52958/comment/67990204_dd146a96
PS3, Line 221: unsigned char *const bp_commbuf = bp_data->bp_commbuf;
perhaps just move this up a few lines to the functions preamble so that the branch line above can be made shorter by using the itermediate.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52958
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8
Gerrit-Change-Number: 52958
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 25 May 2021 05:05:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 )
Change subject: programmer: Introduce default shutdown function
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54890/comment/1489d06d_5cf8caab
PS1, Line 13: lot of par masters that
: will use it in the nearest future.
lets rebase this once we have a few more users rather than too prematurely introducing it. However, good direction.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517
Gerrit-Change-Number: 54890
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 25 May 2021 05:00:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54890 )
Change subject: programmer: Introduce default shutdown function
......................................................................
programmer: Introduce default shutdown function
Default shutdown function is only freeing master data (if data exists)
and not doing anything else.
There is one spi master which can use default shutdown function
(usbblaster, included in this patch), and a lot of par masters that
will use it in the nearest future.
BUG=b:185191942
TEST=builds
Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M flashrom.c
M programmer.h
M usbblaster_spi.c
3 files changed, 9 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/54890/1
diff --git a/flashrom.c b/flashrom.c
index 0af5057..069d59a 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -553,6 +553,13 @@
static int check_block_eraser(const struct flashctx *flash, int k, int log);
+int default_shutdown(void *data)
+{
+ if (data)
+ free(data);
+ return 0;
+}
+
/* Register a function to be executed on programmer shutdown.
* The advantage over atexit() is that you can supply a void pointer which will
* be used as parameter to the registered function upon programmer shutdown.
diff --git a/programmer.h b/programmer.h
index 790fcc8..17bfd2f 100644
--- a/programmer.h
+++ b/programmer.h
@@ -612,6 +612,7 @@
extern unsigned long flashbase;
unsigned int count_max_decode_exceedings(const struct flashctx *flash);
char *extract_programmer_param(const char *param_name);
+int default_shutdown(void *data);
/* spi.c */
#define MAX_DATA_UNSPECIFIED 0
diff --git a/usbblaster_spi.c b/usbblaster_spi.c
index 115876e..61edc2e 100644
--- a/usbblaster_spi.c
+++ b/usbblaster_spi.c
@@ -162,12 +162,6 @@
return ret;
}
-static int usbblaster_shutdown(void *data)
-{
- free(data);
- return 0;
-}
-
static const struct spi_master spi_master_usbblaster = {
.max_data_read = 256,
.max_data_write = 256,
@@ -227,7 +221,7 @@
}
usbblaster_data->ftdic = ftdic;
- if (register_shutdown(usbblaster_shutdown, usbblaster_data)) {
+ if (register_shutdown(default_shutdown, usbblaster_data)) {
free(usbblaster_data);
return -1;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/54890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517
Gerrit-Change-Number: 54890
Gerrit-PatchSet: 1
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newchange