February 2023 - flashrom-gerrit

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/9e789263_e0994291 PS2, Line 1416: data->refs_cnt += ((dummy_buses_supported & BUS_PROG) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_NONSPI) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_SPI) != BUS_NONE); : : #if 0 : data->refs_cnt += (dummy_buses_supported & BUS_PROG) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_NONSPI) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_SPI) ? 1 : 0; : #endif > I have another idea! :) What if you add `refs_cnt++` into every if below. […] I thought about it at first sight. But this approach may potentially cause flashrom to crash. If the dummy programmer registers more than one bus, then the next accident can happen => If the first condition the programmer enters fails because `register_*_master()` could not register shutdown (`register_shutdown()`), then it will call shutdown immediately (`mst->shutdown(data)`, opaque.c:55). And since we only have one owner in refs_cnt at that moment, data will be released. When the dummy tries to enter the second condition and increase `data->refs_cnt`, flashrom will crash because this memory isn't valid anymore. This should be mentioned in the code, but I cannot come up with brief comment for that. -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 08:47:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ni845x_spi: pass global state through func params
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Miklós Márton, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 ) Change subject: ni845x_spi: pass global state through func params ...................................................................... Patch Set 3: (4 comments) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/72154/comment/8e228805_709e6be8 PS2, Line 131: static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle, : enum USB845x_type device_pid) > I am thinking, this could fit in one line: the hard limit for line length is 112 chars. […] OK, It takes 113 chars - it's no big deal. https://review.coreboot.org/c/flashrom/+/72154/comment/0fdc1a02_ba9ad6a4 PS2, Line 152: static int ni845x_spi_open(const char *serial, uInt32 *return_handle, : enum USB845x_type *device_pid) > this can be one line too Done https://review.coreboot.org/c/flashrom/+/72154/comment/bf52b72b_21093f01 PS2, Line 233: set_io_voltage_mV > Not sure you meant it: there is `set_io_voltage_mV` which is an existing second argument in the func […] This second argument (`set_io_voltage_mV`) contains a pointer to `io_voltage_in_mV` global var. If we add one more argument to this function, it will be pointer to `io_voltage_in_mV` too! So, let's just use the second argument. Code snippets that prove my words are below. ni845x_spi.c:447: ``` if (usb8452_spi_set_io_voltage(flash->chip->voltage.max, &io_voltage_in_mV, USE_LOWER)) { ... } ``` ni845x_spi.c:470: ``` if (usb8452_spi_set_io_voltage(flash->chip->voltage.min, &io_voltage_in_mV, USE_HIGHER)) { ... } ``` ni845x_spi.c:619: ``` if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER) < 0) { ... } ``` https://review.coreboot.org/c/flashrom/+/72154/comment/134fdc2c_2be02433 PS2, Line 290: if (set_io_voltage_mV) Miklós, how about dropping this condition? Maybe not in this patch, but in the one of the next. This argument should contain a valid pointer when you call the function to get the IO voltage that was set. Or am I missing something? -- To view, visit https://review.coreboot.org/c/flashrom/+/72154 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Gerrit-Change-Number: 72154 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 08:07:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ni845x_spi: pass global state through func params
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Hello build bot (Jenkins), Miklós Márton, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/72154 to look at the new patch set (#3). Change subject: ni845x_spi: pass global state through func params ...................................................................... ni845x_spi: pass global state through func params Instead of relying on global variables, pass and use their value or pointer to functions where possible. This patch prepares the programmer to move global singleton states into a struct. TOPIC=register_master_api Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/391 --- M ni845x_spi.c 1 file changed, 27 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/72154/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/72154 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Gerrit-Change-Number: 72154 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 07:48:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 2: (1 comment) File tests/dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72665/comment/695bb03c_a71edf47 PS1, Line 126: dummy_cover_buses_test_success > I would rename to `dummy_all_buses_test_success` Done -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 07:39:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Alexander Goncharov (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/72665 to look at the new patch set (#2). Change subject: tests: add bus coverage test for dummy ...................................................................... tests: add bus coverage test for dummy Dummy programmer has a shared data between *_masters. To make sure the dummy has no memory leakage, we need a test that will covers initialization and shutdown of the programmer with different bus types, i.e. programmer specific, non-SPI and SPI. TEST=ninja test Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> --- M tests/dummyflasher.c M tests/tests.c M tests/tests.h 3 files changed, 38 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/72665/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: jedec.c: Fold up dst into toggle_ready_jedec()
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72691 ) Change subject: jedec.c: Fold up dst into toggle_ready_jedec() ...................................................................... Patch Set 1: (1 comment) File jedec.c: https://review.coreboot.org/c/flashrom/+/72691/comment/0356f8b6_13bae55c PS1, Line 72: const chipaddr dst = flash->virtual_memory; Just wondering, I see that all existing callsites have dst as `flash->virtual_memory` but is this always like that? Can dst be anything else? -- To view, visit https://review.coreboot.org/c/flashrom/+/72691 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib7a3fdbc6e0a888093dc8da6f5567a7301ec5040 Gerrit-Change-Number: 72691 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Feb 2023 06:34:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ni845x_spi: pass global state through func params
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 ) Change subject: ni845x_spi: pass global state through func params ...................................................................... Patch Set 2: (4 comments) Patchset: PS2: > This programmer requires a special environment to run, and unfortunately, it only works on Windows. […] Just a small note from me: maybe we can do the code review first (for all the patches in the chain) and then when all comments resolved, do the testing? Also as it's typically happens, testing is done at the end of the chain (not for each individual patch). Let's maybe keep this comment unresolved, and do the rest of code reviews. Miklós thank you so much for help! File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/72154/comment/d3c279e4_dc4bf5cf PS2, Line 131: static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle, : enum USB845x_type device_pid) I am thinking, this could fit in one line: the hard limit for line length is 112 chars. Of course not all the lines need to be that long, but for the case like this (function arguments list) we can use one line, mainly because it makes it easier to grep. https://review.coreboot.org/c/flashrom/+/72154/comment/396a202a_216c01c0 PS2, Line 152: static int ni845x_spi_open(const char *serial, uInt32 *return_handle, : enum USB845x_type *device_pid) this can be one line too https://review.coreboot.org/c/flashrom/+/72154/comment/d9cbca5d_cd118ba6 PS2, Line 233: set_io_voltage_mV Not sure you meant it: there is `set_io_voltage_mV` which is an existing second argument in the function, and there is also `io_voltage_in_mV` which is global. Why do you replace one with the other? To follow what the patch is doing, you would need to add one more argument to this function, `io_voltage_in_mV` and use it instead of a global var? -- To view, visit https://review.coreboot.org/c/flashrom/+/72154 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61 Gerrit-Change-Number: 72154 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 06:27:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: dummyflasher: use new API to register shutdown function
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Edward O'Callaghan, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 ) Change subject: dummyflasher: use new API to register shutdown function ...................................................................... Patch Set 2: (3 comments) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72408/comment/7529d834_1e795536 PS2, Line 79: uint8_t refs_cnt; Can you add a comment on this: to explain that dummyflasher can register multiple masters and they all share the same emu_data, so keeping track of reference count is needed to do clean up at shutdown time. https://review.coreboot.org/c/flashrom/+/72408/comment/71262ae6_032575af PS2, Line 910: > the last owner of the data must release this data yeah that's a good argument, let's keep it. I mark resolved https://review.coreboot.org/c/flashrom/+/72408/comment/ed9a83ea_04ffbf74 PS2, Line 1416: data->refs_cnt += ((dummy_buses_supported & BUS_PROG) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_NONSPI) != BUS_NONE); : data->refs_cnt += ((dummy_buses_supported & BUS_SPI) != BUS_NONE); : : #if 0 : data->refs_cnt += (dummy_buses_supported & BUS_PROG) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_NONSPI) ? 1 : 0; : data->refs_cnt += (dummy_buses_supported & BUS_SPI) ? 1 : 0; : #endif > Which one is more appropriate? I have another idea! :) What if you add `refs_cnt++` into every if below. There are the same three if conditions below, already here. -- To view, visit https://review.coreboot.org/c/flashrom/+/72408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea Gerrit-Change-Number: 72408 Gerrit-PatchSet: 2 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 05:38:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: add bus coverage test for dummy
by Anastasia Klimchuk (Code Review) 03 Feb '23

03 Feb '23

Attention is currently required from: Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 ) Change subject: tests: add bus coverage test for dummy ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Patchset: PS1: just one little comment File tests/dummyflasher.c: https://review.coreboot.org/c/flashrom/+/72665/comment/4ef5f5ca_f1791f3a PS1, Line 126: dummy_cover_buses_test_success I would rename to `dummy_all_buses_test_success` -- To view, visit https://review.coreboot.org/c/flashrom/+/72665 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319 Gerrit-Change-Number: 72665 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 03 Feb 2023 05:16:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit February 2023