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

Change in flashrom[master]: flashchips: Add Spansion/Cypress S25FL256L
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Thomas Walker, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52310 ) Change subject: flashchips: Add Spansion/Cypress S25FL256L ...................................................................... Patch Set 6: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/52310/comment/cdabb3aa_215be900 PS5, Line 16387: FEATURE_4BA > Forgive me if I'm incorrect, but in the datasheet there is a section: 6.1.1 Extended Address […] Yes, two ways. Not three. :) Often there is a third way to access the space above 16MiB. The extended address register (EAR) works like a page selection. If you leave it at 0, you can access the first 16MiB with 24-bit address commands, if you set it to 1, you can access the second 16MiB and so on. Or in other words, internally the 32-bit address is composed of <EAR> | <24-bit from command> Maybe `FEATURE_4BA_EXT_ADDR` is a bit confusing and should be renamed? Other vendors don't use the term "extended address" unless they refer to this particular register. -- To view, visit https://review.coreboot.org/c/flashrom/+/52310 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4e8cba554d0590c94dac92aa91e9ab400efca281 Gerrit-Change-Number: 52310 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Walker <thh.walker(a)gmail.com> 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Walker <thh.walker(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 27 Apr 2021 12:11:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Thomas Walker <thh.walker(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Daniel Campello (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Jack Rosenthal, Paul Menzel, Stefan Reinauer, David Hendricks, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 36: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/23021/comment/1d8c0546_2607b058 PS35, Line 1493: #ifdef __LIBPAYLOAD__ > It feels a bit weird for write_buf_to_include_args() to fail eagerly for __LIBPAYLOAD__ while read_b […] Agree! Removed! -- 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: 36 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: Jack Rosenthal <jrosenth(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: Jack Rosenthal <jrosenth(a)chromium.org> 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: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Tue, 27 Apr 2021 12:05:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Daniel Campello (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Nico Huber, Nicola Corna, Jack Rosenthal, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has uploaded a new patch set (#36) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... layout: Add -i <region>[:<file>] support Add an optional sub-parameter to the -i parameter to allow building the image to be written from multiple files. This will also allow regions to be read from flash and written to separate image files. This is a rebase of a patch that was ported from chromiumos. A lot of things have changed, but the idea is the same. Original patch by Louis Yung-Chieh Lo <yjlou(a)chromium.org>: Summary: Support -i partition:file feature for both read and write. Commit: 9c7525f Review URL: http://codereview.chromium.org/6611015 Ported version by Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> and Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>: Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support. Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Daniel Campello <campello(a)chromium.org> Co-Authored-by: Edward O'Callaghan <quasisec(a)google.com> Co-Authored-by: Daniel Campello <campello(a)chromium.org> --- M cli_classic.c M flash.h M flashrom.8.tmpl M flashrom.c M layout.c M layout.h 6 files changed, 234 insertions(+), 37 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/36 -- 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: 36 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: Jack Rosenthal <jrosenth(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: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org> 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-MessageType: newpatchset

1 0

Change in flashrom[master]: s25f.c: Fix mismatched function definitions
by Edward O'Callaghan (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52473 ) Change subject: s25f.c: Fix mismatched function definitions ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/52473/comment/668a50dd_5b0a8840 PS1, Line 8: : This was missed because `uint32_t` is `unsigned int` in most cases. : However, it is not the case for DJGPP 6.1.0 for some reason. > > Last couple of weeks have been pretty painful :| I've just been on and off review unofficially to […] Understood re the regression :< C + many platforms + bit rot under rugs in Flashrom has been a potent combo to start fires. Generally since it bisect's to Nikolai work we should get him on the hook as he works approx 50% on Flashrom at the moment. I am trying to get more folks on IRC and integrating with upstream so everyone is working together and things are moving in that direction, certainly better than a year ago for example. I could make the patch but I think you are better positioned to test it? It is very very hard at Google to have any other environments than the ones that are given, I don't know how to address this easily. I think buildbot is the way forwards here. For DJGPP it sounds like what we really need to do here is ensure this is integrated into the buildbot somehow? I think that is actually the problem here to be scalable. I'll ping Patrick to see if he has any cycles he could spare to assist in how we can do that. For the sanity check I think you have thought about it way more deeply than I. My concern was restricted to `addr`. Is this something we can perhaps get Anastasia across who is keen to get these unit tests to a point of maturity. I wonder if there is anything in this for her? -- To view, visit https://review.coreboot.org/c/flashrom/+/52473 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I656a72b85d4c70b57f6ff9268186a4a60933f8a9 Gerrit-Change-Number: 52473 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 27 Apr 2021 11:57:04 +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> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ft2232_spi.c: Implement spi_send_multicommand()
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Simon Buhrow, Alan Green, Edward O'Callaghan, Anastasia Klimchuk, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40477 ) Change subject: ft2232_spi.c: Implement spi_send_multicommand() ...................................................................... Patch Set 30: Code-Review+1 (4 comments) Patchset: PS30: This looks pretty good now, thank you! I'll try to find some time to also perform some tests. File ft2232_spi.c: https://review.coreboot.org/c/flashrom/+/40477/comment/25da835b_ee78e2c5 PS30, Line 291: <= buffer_size; Sorry, looks like you copy-pasted from my comment? Needs tabs. https://review.coreboot.org/c/flashrom/+/40477/comment/34af67c9_db1c43da PS30, Line 303: * Minimize FTDI-calls by packing as many commands as possible Nit, the line break just seems odd. https://review.coreboot.org/c/flashrom/+/40477/comment/60e4105d_c0a15933 PS30, Line 311: ){ Missing space before opening brace. -- 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: 30 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: Simon Buhrow Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Tue, 27 Apr 2021 10:57:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: mec1308.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52597 ) Change subject: mec1308.c: Separate shutdown from failed init cleanup ...................................................................... mec1308.c: Separate shutdown from failed init cleanup Shutdown function was covering two different jobs here: 1) the actual shutdown which is run at the end of the driver's lifecycle and 2) cleanup in cases when initialisation failed. Now, shutdown is only doing its main job (#1), and the driver itself is doing cleanup when init fails (#2). The good thing is that now resources are released/closed immediately in cases when init fails (vs shutdown function which was run at some point later), and the driver leaves clean space after itself if init fails. And very importantly this unlocks API change which plans to move register_shutdown inside register master API, see https://review.coreboot.org/c/flashrom/+/51761 TEST=builds and ninja test from 51487 BUG=b:185191942 Change-Id: I6d62d43dd8b6ebc595f9fd747e0f4cd80f3c10da Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52597 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M mec1308.c 1 file changed, 8 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/mec1308.c b/mec1308.c index 641f5f8..0ec9be9 100644 --- a/mec1308.c +++ b/mec1308.c @@ -496,9 +496,6 @@ goto init_err_exit; } - if (register_shutdown(mec1308_shutdown, ctx_data)) - goto init_err_exit; - /* * Enter SPI Pass-Thru Mode after commands which do not require access * to SPI ROM are complete. We'll start by doing the exit_passthru_mode @@ -507,15 +504,22 @@ mec1308_exit_passthru_mode(ctx_data); if (enter_passthru_mode(ctx_data)) - goto init_err_exit; + goto init_err_cleanup_exit; internal_buses_supported |= BUS_LPC; /* for LPC <--> SPI bridging */ spi_master_mec1308.data = ctx_data; + + if (register_shutdown(mec1308_shutdown, ctx_data)) + goto init_err_cleanup_exit; register_spi_master(&spi_master_mec1308); msg_pdbg("%s(): successfully initialized mec1308\n", __func__); return 0; +init_err_cleanup_exit: + mec1308_shutdown(ctx_data); + return 1; + init_err_exit: free(ctx_data); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/52597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6d62d43dd8b6ebc595f9fd747e0f4cd80f3c10da Gerrit-Change-Number: 52597 Gerrit-PatchSet: 4 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: Victor Ding <victording(a)google.com> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: mec1308.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52597 ) Change subject: mec1308.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 3: Code-Review+2 (1 comment) File mec1308.c: https://review.coreboot.org/c/flashrom/+/52597/comment/e9d1c060_7dff7fbf PS2, Line 496: goto mec1308_init_exit; > I noticed this in other places, that shutdown != cleanup and all of them I want to sort eventually. […] Right, behavioral changes should be separate commits :) -- To view, visit https://review.coreboot.org/c/flashrom/+/52597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6d62d43dd8b6ebc595f9fd747e0f4cd80f3c10da Gerrit-Change-Number: 52597 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Victor Ding <victording(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Apr 2021 10:32:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes 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]: mec1308.c: Untangle successful vs failed init paths
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52596 ) Change subject: mec1308.c: Untangle successful vs failed init paths ...................................................................... mec1308.c: Untangle successful vs failed init paths Label mec1308_init_exit now serves as failed init path, it does cleanup and returns 1, so it is renamed into init_err_exit. Since all error paths return 1, and successful init is separated from failure, there is no need to have ret variable anymore. TEST=builds and ninja test from 51487 BUG=b:185191942 Change-Id: Ibf35335501e59636c544af124ad7a04a186790b4 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52596 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M mec1308.c 1 file changed, 16 insertions(+), 27 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/mec1308.c b/mec1308.c index 682e7a9..641f5f8 100644 --- a/mec1308.c +++ b/mec1308.c @@ -426,7 +426,6 @@ uint16_t sio_port; uint8_t device_id; uint8_t tmp8; - int ret = 0; mec1308_data_t *ctx_data = NULL; msg_pdbg("%s(): entered\n", __func__); @@ -437,15 +436,12 @@ return 1; } - if (check_params()) { - ret = 1; - goto mec1308_init_exit; - } + if (check_params()) + goto init_err_exit; if (mec1308_get_sio_index(ctx_data, &sio_port) < 0) { msg_pdbg("MEC1308 not found (probe failed).\n"); - ret = 1; - goto mec1308_init_exit; + goto init_err_exit; } device_id = sio_read(sio_port, MEC1308_DEVICE_ID_REG); switch(device_id) { @@ -461,8 +457,7 @@ break; default: msg_pdbg("MEC1308 not found\n"); - ret = 1; - goto mec1308_init_exit; + goto init_err_exit; } /* @@ -486,28 +481,23 @@ * command to finish.*/ if (mbx_wait(ctx_data) != 0) { msg_perr("%s: mailbox is not available\n", __func__); - ret = 1; - goto mec1308_init_exit; + goto init_err_exit; } /* Further setup -- disable SMI and ACPI. FIXME: is there an ordering dependency? */ if (mbx_write(ctx_data, MEC1308_MBX_CMD, MEC1308_CMD_ACPI_DISABLE)) { msg_pdbg("%s: unable to disable ACPI\n", __func__); - ret = 1; - goto mec1308_init_exit; + goto init_err_exit; } if (mbx_write(ctx_data, MEC1308_MBX_CMD, MEC1308_CMD_SMI_DISABLE)) { msg_pdbg("%s: unable to disable SMI\n", __func__); - ret = 1; - goto mec1308_init_exit; + goto init_err_exit; } - if (register_shutdown(mec1308_shutdown, ctx_data)) { - ret = 1; - goto mec1308_init_exit; - } + if (register_shutdown(mec1308_shutdown, ctx_data)) + goto init_err_exit; /* * Enter SPI Pass-Thru Mode after commands which do not require access @@ -516,19 +506,18 @@ */ mec1308_exit_passthru_mode(ctx_data); - if (enter_passthru_mode(ctx_data)) { - ret = 1; - goto mec1308_init_exit; - } + if (enter_passthru_mode(ctx_data)) + goto init_err_exit; internal_buses_supported |= BUS_LPC; /* for LPC <--> SPI bridging */ spi_master_mec1308.data = ctx_data; register_spi_master(&spi_master_mec1308); msg_pdbg("%s(): successfully initialized mec1308\n", __func__); -mec1308_init_exit: - if (ret) - free(ctx_data); - return ret; + return 0; + +init_err_exit: + free(ctx_data); + return 1; } #endif -- To view, visit https://review.coreboot.org/c/flashrom/+/52596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibf35335501e59636c544af124ad7a04a186790b4 Gerrit-Change-Number: 52596 Gerrit-PatchSet: 4 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: Victor Ding <victording(a)google.com> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: mec1308.c: Untangle successful vs failed init paths
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52596 ) Change subject: mec1308.c: Untangle successful vs failed init paths ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52596 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibf35335501e59636c544af124ad7a04a186790b4 Gerrit-Change-Number: 52596 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-CC: Victor Ding <victording(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 27 Apr 2021 10:29:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: mec1308.c: Extract params check into a function
by Nico Huber (Code Review) 27 Apr '21

27 Apr '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52595 ) Change subject: mec1308.c: Extract params check into a function ...................................................................... mec1308.c: Extract params check into a function This allows char *p to become a local variable in check_params, and it is allocated and freed within check_params function. Which means init function does not need char *p anymore, in particular does not need to free it - and this makes cleanup after failed init easier. As a good side effect, init function becomes easier to read. TEST=builds and ninja test from 51487 BUG=b:185191942 Change-Id: If5be7709e93233a2e7ea9133de50382d2524a55f Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52595 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M mec1308.c 1 file changed, 16 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved diff --git a/mec1308.c b/mec1308.c index c4b34f5..682e7a9 100644 --- a/mec1308.c +++ b/mec1308.c @@ -406,13 +406,27 @@ .write_256 = default_spi_write_256, }; +static int check_params(void) +{ + int ret = 0; + char *p = NULL; + + p = extract_programmer_param("type"); + if (p && strcmp(p, "ec")) { + msg_pdbg("mec1308 only supports \"ec\" type devices\n"); + ret = 1; + } + + free(p); + return ret; +} + int mec1308_init(void) { uint16_t sio_port; uint8_t device_id; uint8_t tmp8; int ret = 0; - char *p = NULL; mec1308_data_t *ctx_data = NULL; msg_pdbg("%s(): entered\n", __func__); @@ -423,9 +437,7 @@ return 1; } - p = extract_programmer_param("type"); - if (p && strcmp(p, "ec")) { - msg_pdbg("mec1308 only supports \"ec\" type devices\n"); + if (check_params()) { ret = 1; goto mec1308_init_exit; } @@ -515,7 +527,6 @@ msg_pdbg("%s(): successfully initialized mec1308\n", __func__); mec1308_init_exit: - free(p); if (ret) free(ctx_data); return ret; -- To view, visit https://review.coreboot.org/c/flashrom/+/52595 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If5be7709e93233a2e7ea9133de50382d2524a55f Gerrit-Change-Number: 52595 Gerrit-PatchSet: 4 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: Victor Ding <victording(a)google.com> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021