April 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_spi.c: Resolve comments left from previous patches
by Edward O'Callaghan (Code Review) 19 Apr '21

19 Apr '21

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/+/52492 ) Change subject: linux_spi.c: Resolve comments left from previous patches ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52492 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I27a718b515fc474f63b3e61be58a6f9302527559 Gerrit-Change-Number: 52492 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> 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: Mon, 19 Apr 2021 05:35:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Edward O'Callaghan (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 11: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52383/comment/894cab58_351bebce PS11, Line 9: Allows - as filename for -r/-w/-v options Do add a rational here, perhaps something like - "It is sometimes useful to script flashrom and allowing it to work with pipes allows for more flexibility in this specific use-case." I guess would be something reasonable to say? -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 11 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: Jack Rosenthal <jrosenth(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: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Mon, 19 Apr 2021 05:29:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: add -x option for do_extract()
by Edward O'Callaghan (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Sam McNally, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52450 ) Change subject: cli_classic.c: add -x option for do_extract() ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52450 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8c69223fa92cf5b50abe070f1ab9f19d3b42f6ff Gerrit-Change-Number: 52450 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Mon, 19 Apr 2021 05:27:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Simon Buhrow, Alan Green, Nico Huber, Edward O'Callaghan, Samir Ibradžić. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Pack WREN and op in one ftdi_write_data() call ...................................................................... Patch Set 25: (1 comment) File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/a2228e8c_5f439590 PS25, Line 295: 9 > I see 6 potential paths, due to the 3 if's but the second may end […] oh yes... I made a mistake, I didn't notice continue statement. I agree, if de-assert CS# is always the last thing to do, it would be ideal to have it last in the code as well. -- To view, visit https://review.coreboot.org/c/flashrom/+/40477 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4a07499ec5ef0af23818593f45dc427285a9e8a Gerrit-Change-Number: 40477 Gerrit-PatchSet: 25 Gerrit-Owner: Simon Buhrow Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Mon, 19 Apr 2021 03:26:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: automatic generated programmer_enum.h
by Xiang Wang (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Stefan Reinauer. Xiang Wang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49599 ) Change subject: flashrom.c: automatic generated programmer_enum.h ...................................................................... Patch Set 6: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/49599/comment/018cc561_7299734d PS4, Line 681: $(shell bash ./util/generator_programmer_enum.sh) > Sorry for the delay. I don't have much time and some effort the rework […] Thank you. I know these, but this modification will be inconsistent with meson. In fact, regenerating every time has little effect on compilation speed. -- To view, visit https://review.coreboot.org/c/flashrom/+/49599 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3f4370ccae2b64da3c4178243b192700d3d205d2 Gerrit-Change-Number: 49599 Gerrit-PatchSet: 6 Gerrit-Owner: Xiang Wang <wxjstz(a)126.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> 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-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Mon, 19 Apr 2021 02:38:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Xiang Wang <wxjstz(a)126.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Nico Huber. 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 3: (2 comments) Patchset: PS3: Thanks for comments! I have created another patch https://review.coreboot.org/c/flashrom/+/52492 to fix all of them (since this one is already merged and I can’t change it). I also “replied” to the comments in the new patch, if anymore changes needed I will do everything in 52492. File linux_spi.c: https://review.coreboot.org/c/flashrom/+/52285/comment/c99fd9f5_bda01253 PS3, Line 111: const > Why drop it? […] Yeah it's a step that I need to do on the way to new API, so it is not entirely reentrant at the moment - but will be! -- 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: 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: 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Mon, 19 Apr 2021 01:57:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Nico Huber. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52284 ) Change subject: linux_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Thanks for comments! I have created another patch https://review.coreboot.org/c/flashrom/+/52492 to fix all of them (since this one is already merged and I can’t change it). I also “replied” to the comments in the new patch, if anymore changes needed I will do everything in 52492. -- To view, visit https://review.coreboot.org/c/flashrom/+/52284 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1c8da2878cd0e85a1e43ba9b4b8e6f3d9f38ae5c Gerrit-Change-Number: 52284 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Mon, 19 Apr 2021 01:56:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Extract get_max_kernel_buf_size() as a function
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: 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 2: (1 comment) Patchset: PS2: Thanks for comments! I have created another patch https://review.coreboot.org/c/flashrom/+/52492 to fix all of them (since this one is already merged and I can’t change it). I also “replied” to the comments in the new patch, if anymore changes needed I will do everything in 52492. -- 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: 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-Comment-Date: Mon, 19 Apr 2021 01:55:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Resolve comments left from previous patches
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Attention is currently required from: Nico Huber, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52492 ) Change subject: linux_spi.c: Resolve comments left from previous patches ...................................................................... Patch Set 1: (4 comments) File linux_spi.c: https://review.coreboot.org/c/flashrom/+/52492/comment/ae8258a7_8122f9a5 PS1, Line 71: struct linux_spi_data *spi_data = data; Same thing may be in other shutdown functions, but since I will be going through all them soon, will try to fix where I can. https://review.coreboot.org/c/flashrom/+/52492/comment/564ada64_26c1f5d1 PS1, Line 176: int fd; First time this variable is used, it is immediately assigned as open(dev, ...) line 204, so default value is not needed. And since it is local, it will be created again every time init runs. https://review.coreboot.org/c/flashrom/+/52492/comment/98ef81cf_8d660829 PS1, Line 208: return 1; For this error path and all the above, close(fd) is not needed, because fd hasn't been opened yet. All the error paths below this one should goto init_err because it does cleanup i.e. close(fd). https://review.coreboot.org/c/flashrom/+/52492/comment/6b1706a3_159a37ef PS1, Line 252: return 1; Since SPI_GENERIC_ERROR is not used anymore, ret variable is not needed because init error always returns 1. -- To view, visit https://review.coreboot.org/c/flashrom/+/52492 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I27a718b515fc474f63b3e61be58a6f9302527559 Gerrit-Change-Number: 52492 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 19 Apr 2021 01:54:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: linux_spi.c: Resolve comments left from previous patches
by Anastasia Klimchuk (Code Review) 19 Apr '21

19 Apr '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52492 ) Change subject: linux_spi.c: Resolve comments left from previous patches ...................................................................... linux_spi.c: Resolve comments left from previous patches In previous patches 52283, 52284, 52285 there were some unresolved comments left, resolving [and replying to] all of that here. TEST=builds and ninja test from 51487 BUG=b:185191942 Change-Id: I27a718b515fc474f63b3e61be58a6f9302527559 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M linux_spi.c 1 file changed, 7 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/52492/1 diff --git a/linux_spi.c b/linux_spi.c index 541ca65..4668e7f 100644 --- a/linux_spi.c +++ b/linux_spi.c @@ -68,10 +68,8 @@ static int linux_spi_shutdown(void *data) { - struct linux_spi_data *spi_data = (struct linux_spi_data *) data; - if (spi_data->fd != -1) - close(spi_data->fd); - + struct linux_spi_data *spi_data = data; + close(spi_data->fd); free(spi_data); return 0; } @@ -127,7 +125,8 @@ }; /* Read max buffer size from sysfs, or use page size as fallback. */ -static size_t get_max_kernel_buf_size() { +static size_t get_max_kernel_buf_size() +{ size_t result = 0; FILE *fp; fp = fopen(BUF_SIZE_FROM_SYSFS, "r"); @@ -174,10 +173,9 @@ /* SPI mode 0 (beware this also includes: MSB first, CS active low and others */ const uint8_t mode = SPI_MODE_0; const uint8_t bits = 8; - int fd = -1; + int fd; size_t max_kernel_buf_size; struct linux_spi_data *spi_data; - int ret = 0; p = extract_programmer_param("spispeed"); if (p && strlen(p)) { @@ -214,7 +212,6 @@ if (ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed_hz) == -1) { msg_perr("%s: failed to set speed to %"PRIu32"Hz: %s\n", __func__, speed_hz, strerror(errno)); - ret = 1; goto init_err; } msg_pdbg("Using %"PRIu32"kHz clock\n", speed_hz / 1000); @@ -222,14 +219,12 @@ if (ioctl(fd, SPI_IOC_WR_MODE, &mode) == -1) { msg_perr("%s: failed to set SPI mode to 0x%02x: %s\n", __func__, mode, strerror(errno)); - ret = 1; goto init_err; } if (ioctl(fd, SPI_IOC_WR_BITS_PER_WORD, &bits) == -1) { msg_perr("%s: failed to set the number of bits per SPI word to %u: %s\n", __func__, bits == 0 ? 8 : bits, strerror(errno)); - ret = 1; goto init_err; } @@ -239,7 +234,6 @@ spi_data = calloc(1, sizeof(*spi_data)); if (!spi_data) { msg_perr("Unable to allocated space for SPI master data\n"); - ret = SPI_GENERIC_ERROR; goto init_err; } spi_data->fd = fd; @@ -248,18 +242,14 @@ if (register_shutdown(linux_spi_shutdown, spi_data)) { free(spi_data); - ret = 1; goto init_err; } register_spi_master(&spi_master_linux); return 0; init_err: - if (fd != -1) { - close(fd); - fd = -1; - } - return ret; + close(fd); + return 1; } #endif // CONFIG_LINUX_SPI == 1 -- To view, visit https://review.coreboot.org/c/flashrom/+/52492 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I27a718b515fc474f63b3e61be58a6f9302527559 Gerrit-Change-Number: 52492 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021