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

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Anastasia Klimchuk (Code Review) 03 May '21

03 May '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/50246/comment/0181bd39_5d2811fe PS4, Line 14: The user of the registered_masters type is therefore : responsible for querying the buses_supported field before : attempting to dereference a ptr field in the anon struct. Can the user check which one out of three is not null? -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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: 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 03 May 2021 08:28:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Simon Buhrow (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Samir Ibradžić. Simon Buhrow has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 32: (1 comment) Patchset: PS32: > I wondered what device was used for testing so far and […] Thanks for your effort! hm, I´m not sure what´s wrong. Saving a few seconds is not what I was looking for... Some information: * the 1,36MHz on the screenshot is wrong (sorry for missleading!!) It´s the distance between the cursors which may be placed arbitrarily * I´m using default divisor and on my osci screenshots taken last week that results in something between 25-30 MHz (did not measure, but estimate from screenshots) * lsusb says bcdDevice 7.00 as well * I run: `make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no`. So I´m not using libusb 1.0 maybe this leads to some difference? * I´m doing tests on my side with an N25Q128..3E chip (has quite the same parameters as W25Q128FV") * I´m running flashrom on a virutal machine (don´t think this makes any difference) * at SPI-Bus level the multicmd+chunk4k looks much better and faster (i.e. less bursts) so I have no idea why this should not lead to an overall speed increase... (regardless of the setup) NB: yepp, I noticed that with the erase block size, as well and have it on my todo list. I was thinking about something like comandline parameter to select block size or an extra parameter at flashchips.c to mark the preferred block size. May be we should open a new patch for that to discuss ideas... (it was just because of lack of support for this patch for month why I did not step further) -- 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: 32 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com> 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: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Mon, 03 May 2021 07:27:04 +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]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 03 May '21

03 May '21

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/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 2: (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/9e0db6a6_26e095d2 PS1, Line 472: pickit2_data = calloc(1, sizeof(*pickit2_data)); > No NULL check? (Could be avoided by not using a struct. […] This my mistake sorry :( NULL check added, I do need a struct. -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 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: 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: Mon, 03 May 2021 07:24:00 +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]: pickit2_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 03 May '21

03 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52774 to look at the new patch set (#2). Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... pickit2_spi.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". TEST=builds BUG=b:185191942 Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M pickit2_spi.c 1 file changed, 37 insertions(+), 16 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/52774/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 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: 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-MessageType: newpatchset

1 0

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

03 May '21

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/+/52774 ) Change subject: pickit2_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (2 comments) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52774/comment/4ba8fb6d_1ab4429c PS1, Line 57: }; > Do we need this struct? I guess it would make it easier to extend in the […] Oh I am so happy that you asked! We haven't discussed this aspect in details, which I was slightly worried about - but now I have an excuse to write a very long comment :) One thing I want to achieve is to hide data memory management under register_master API, so that a driver (like this one) does not need to calloc in init and free in shutdown. calloc will be in register_master, free will be in programmer_shutdown. flashrom.c#L662 Looking closer at programmer_shutdown, it runs all shutdown functions with data, and seems like this is the last place in driver lifecycle that needs data, so I thought to free data here after corresponding shutdown function has completed. programmer_shutdown does not care what the data is, it can run free(data) for everything. "Everything" here is all the places that do register_shutdown, and those are not only spi masters (or not only any masters). So lots of various places, lots of various datas, but for most of that free(data) is perfectly fine, exactly what's needed. But some places have special data, which is initialised and destroyed in a special way. Like this one does libusb_init + libusb_open_device_with_vid_pid and then libusb_close + libusb_exit. programmer_shutdown has no way to detect, it only has void* as data. So, to be able to run free(data) for all datas in programmer_shutdown I need to wrap those special datas into a struct, and then it can be allocated and freed in a standard way in the core. Once I have all shutdown datas calloc-able and free-able, I can actually do a patch which only moves free(data) from individual drivers into programmer_shutdown. And this patch could be independent of register_master patch! https://review.coreboot.org/c/flashrom/+/52774/comment/9a61f3cf_8dcfd07b PS1, Line 343: static struct spi_master spi_master_pickit2 = { > Same and this is something Anastasia and I actively discussed. […] >>> The per-instance `data` pointer should simply be passed to register_spi_master() Yes I agree absolutely, you are right, and that's the goal! (I really want to fix the API as soon as it becomes possible.) But we need data pointer to exist (to be defined) for that. Another thing is that driver which uses global state "pretends" that it does not need data (gives NULL to register_master and register_shutdown), but it actually needs data. When I go through all of that and remove global state, create data structs, I can see all the variety of data which exists in flashrom and with all this knowledge I can be more confident and make sure new API works for all cases, including edge cases. I also wanted to say, this has more diffs, and most of them will stay, they are useful with new API: specifically all static functions should not rely on global state, and accept data that they need as arguments. I can even say, at the beginning I was trying to find the shortest way to fix the broken API - but after looking thought the code I realised that the shortest way is still long. This is fine, will go through everything. Please tell me if I haven't answered your question. I am very happy to talk more! -- To view, visit https://review.coreboot.org/c/flashrom/+/52774 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibacc4738bee02c371c41583d321e0337128ad18a Gerrit-Change-Number: 52774 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: Mon, 03 May 2021 07:02:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello Sam McNally, build bot (Jenkins), Alan Green, Stefan Reinauer, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/50246 to look at the new patch set (#4). Change subject: programmer.h: Convert anon union to anon struct ...................................................................... programmer.h: Convert anon union to anon struct Convert the anon union of registered masters in the mst field of the flashctx to a anon struct. If we are going to dereference a pointer there in an undefined way we should crash and not plow ahead with invalid memory. The user of the registered_masters type is therefore responsible for querying the buses_supported field before attempting to dereference a ptr field in the anon struct. BUG=b:175849641 TEST=`flashrom -p internal --flash-name` Change-Id: I576967a8599b923c902e39f177f39146291cc242 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M opaque.c M programmer.c M programmer.h M spi.c 4 files changed, 5 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/50246/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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: 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Sam McNally (Code Review) 03 May '21

03 May '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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: 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 03 May 2021 05:25:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Sam McNally, Stefan Reinauer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50246 ) Change subject: programmer.h: Convert anon union to anon struct ...................................................................... Patch Set 3: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/50246/comment/6e0de756_650c5f98 PS2, Line 29: printf("%d\n", *foo.b); I just removed it to avoid confusion. > Still, I believe this change is a good idea: it's better to have null pointers than non-null but invalid pointers. Indeed that's the idea. File programmer.h: https://review.coreboot.org/c/flashrom/+/50246/comment/78e8b15e_4ac8fed1 PS2, Line 752: struct { > This will need corresponding updates to ensure unused masters are zeroed out. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 03 May 2021 04:56:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer.h: Convert anon union to anon struct
by Edward O'Callaghan (Code Review) 03 May '21

03 May '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan. Hello Sam McNally, Alan Green, build bot (Jenkins), Stefan Reinauer, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/50246 to look at the new patch set (#3). Change subject: programmer.h: Convert anon union to anon struct ...................................................................... programmer.h: Convert anon union to anon struct Convert the anon union of registered masters in the mst field of the flashctx to a anon struct. If we are going to dereference a pointer there in an undefined way we should crash and not plow ahead with invalid memory. The user of the registered_masters type is therefore responsible for querying the buses_supported field before attempting to dereference a ptr field in the anon struct. BUG=none TEST=`flashrom -p internal --flash-name` Change-Id: I576967a8599b923c902e39f177f39146291cc242 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M opaque.c M programmer.c M programmer.h M spi.c 4 files changed, 5 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/50246/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/50246 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I576967a8599b923c902e39f177f39146291cc242 Gerrit-Change-Number: 50246 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

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

03 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52773 ) Change subject: pickit2_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 1: (1 comment) File pickit2_spi.c: https://review.coreboot.org/c/flashrom/+/52773/comment/88bfc4ef_89b58987 PS1, Line 482: goto init_err_cleanup_exit; > Just one comment to make this review not too boring: This actually changes […] I can explain my thinking process on this, because very oddly, the answer to the question whether this changes behaviour is yes and no at the same time :/ You explained why the answer is yes, and you are right of course! At the same time, what was before: if failure happened between pickit2_get_firmware_version() and register_spi_master() then pickit2_shutdown() would be called (because at the point when pickit2_get_firmware_version() run, shutdown function has already been registered). This stays the same: all failures before pickit2_get_firmware_version() do cleanup for themselves, all failures after pickit2_get_firmware_version() rely on pickit2_shutdown(). It seems odd, because register_shutdown used to be on a dividing line between pickit2_get_firmware_version() and the rest, and now it's in the deep. I don't know what is ideal solution here. One thing I do know is that register_shutdown will disappear from here soon, so whatever we decide to do it's not for long. What do you think? I can change code here if you think I should do that, no problem! -- To view, visit https://review.coreboot.org/c/flashrom/+/52773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b672b33169a7a1b6ceab190ad3f48c2f35c3a1f Gerrit-Change-Number: 52773 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 03 May 2021 01:54:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021