Attention is currently required from: Nico Huber, Daniel Campello, Angel Pons.
Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 )
Change subject: cli_classic: Make -r/-w/-v argument optional
......................................................................
Patch Set 1:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/52362/comment/04d0f4b0_7b0681d5
PS1, Line 46:
I'm not too familiar with what the design considerations are here, so take this with a grain of salt, but I wonder if there is a way to make the flags more orthogonal. Right now, these two invocations would be equivalent:
-r some_file.bin -i some_region
-r -i some_region:some_file.bin
One option is to get rid of "-i" completely and make -r/-w/-v repeated with optional region parameters, e.g. something like
-r some_file.bin
-r some_file.bin:some_region
-r some_file.bin:some_region -r some_other_file.bin:some_other_region
But if we need to keep "-i" around, then everything here seems reasonable.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52362
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41
Gerrit-Change-Number: 52362
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:48:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52285 )
Change subject: linux_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52285/comment/7e10acea_861a7dd8
PS2, Line 178: size_t max_kernel_buf_size;
> s/these lines/this line/
I introduced this line here, because I remove global state in this patch, and in previous patches max_kernel_buf_size is still global and don't need local definition in init function.
What do you think?
--
To view, visit https://review.coreboot.org/c/flashrom/+/52285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078
Gerrit-Change-Number: 52285
Gerrit-PatchSet: 2
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:47:54 +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: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52283 )
Change subject: linux_spi.c: Extract get_max_kernel_buf_size() as a function
......................................................................
Patch Set 1:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52283/comment/478435d1_24c5304e
PS1, Line 169: const uint8_t bits = 8;
> missing the line: […]
Oh, I can explain why this line is not here. At this patch, max_kernel_buf_size is still a global state (together with fd), they are defined in lines 47, 49. I am not removing global state in this patch, so I left it as is, and since max_kernel_buf_size is global I don't need to define it locally in init function.
I do remove global state later in https://review.coreboot.org/c/flashrom/+/52285 , but not here.
What do you think about it? I was trying to keep the purpose for each patch very focused.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52283
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4b8c5775fb8f4b0dff702fcc0fb258221254c659
Gerrit-Change-Number: 52283
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:44: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, Daniel Campello, Angel Pons.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 )
Change subject: cli_classic: Make -r/-w/-v argument optional
......................................................................
Patch Set 1:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/52362/comment/18ea30e9_734af317
PS1, Line 140: static char *get_optional_filename(char *argv[])
> The diff is for a different functionality. […]
ACK. Do you mind just encoding that in the commit message so we remember this detail in the diff or via a BUG= line
--
To view, visit https://review.coreboot.org/c/flashrom/+/52362
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41
Gerrit-Change-Number: 52362
Gerrit-PatchSet: 1
Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:38:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support
......................................................................
Patch Set 27:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/b69b3e0b_0cbcce10
PS27, Line 324: free(tempstr);
> Not a leak fix... before the string was stored directly on the layout entry... […]
ACK my mistake. Thanks.
--
To view, visit https://review.coreboot.org/c/flashrom/+/23021
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Gerrit-Change-Number: 23021
Gerrit-PatchSet: 27
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nicola Corna <nicola(a)corna.info>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:37:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52285 )
Change subject: linux_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52285/comment/1a87eeb0_9e3865cf
PS2, Line 178: size_t max_kernel_buf_size;
> sorry I didn't notice this from the previous review but these lines belongs in the "linux_spi. […]
s/these lines/this line/
--
To view, visit https://review.coreboot.org/c/flashrom/+/52285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078
Gerrit-Change-Number: 52285
Gerrit-PatchSet: 2
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:36:08 +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: Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52285 )
Change subject: linux_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52285/comment/f6f4722d_f5569f9a
PS2, Line 178: size_t max_kernel_buf_size;
sorry I didn't notice this from the previous review but these lines belongs in the "linux_spi.c: Extract get_max_kernel_buf_size() as a function" patch.
--
To view, visit https://review.coreboot.org/c/flashrom/+/52285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078
Gerrit-Change-Number: 52285
Gerrit-PatchSet: 2
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:34:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52285 )
Change subject: linux_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File linux_spi.c:
https://review.coreboot.org/c/flashrom/+/52285/comment/6be5b41e_df40289f
PS1, Line 74: spi_data->fd = -1;
> no longer needed to set fd back to -1 as spi_data is freed after this branch.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/52285
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078
Gerrit-Change-Number: 52285
Gerrit-PatchSet: 2
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 15 Apr 2021 04:28:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment