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

Change in flashrom[master]: internal: Return early from map_flash for >16MiB on x86
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56721 ) Change subject: internal: Return early from map_flash for >16MiB on x86 ...................................................................... Patch Set 3: (1 comment) Patchset: PS1: > Don't call map_flash for X86 you mean? or get rid of the call to map_flash here https://review.coreboot.org/plugins/gitiles/flashrom/+/refs/heads/master/fl… ? The latter. The call should be done by the programmer drivers that make use of the mapping, I guess. -- To view, visit https://review.coreboot.org/c/flashrom/+/56721 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e Gerrit-Change-Number: 56721 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 13:38:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Angel Pons (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Nico Huber, Miklós Márton, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56637 ) Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... Patch Set 1: (2 comments) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/56637/comment/9582fb63_3b0f3859 PS1, Line 556: CS_number = CS_str[0] - '0'; > That's not a problem though, is it? These are just numbers in C. It might […] Is underflow behavior defined for `unsigned char`? https://review.coreboot.org/c/flashrom/+/56637/comment/d6659230_f79066b4 PS1, Line 556: CS_number = CS_str[0] - '0'; : free(CS_str); : if (strlen(CS_str) > 1 || 7 < CS_number) { : msg_perr("Only CS 0-7 supported\n"); : return 1; : } > I would just use strtoul() TBH :) It's simpler, sounds good. -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 10:56:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: Fix signed - unsigned comparisons
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Miklós Márton, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56637 ) Change subject: ni845x_spi: Fix signed - unsigned comparisons ...................................................................... Patch Set 1: (3 comments) File ni845x_spi.c: https://review.coreboot.org/c/flashrom/+/56637/comment/d140ebc5_d7f3f631 PS1, Line 188: strtol > Use `strtoul()` instead? +1 https://review.coreboot.org/c/flashrom/+/56637/comment/0abca4ce_09d4ea0e PS1, Line 556: CS_number = CS_str[0] - '0'; > This line converts string to number, but only two lines below the condition is checked whether the s […] That's not a problem though, is it? These are just numbers in C. It might underflow, but that's still defined behavior and the `7 < CS_number` would catch it? https://review.coreboot.org/c/flashrom/+/56637/comment/f92a4df8_ed289fae PS1, Line 556: CS_number = CS_str[0] - '0'; : free(CS_str); : if (strlen(CS_str) > 1 || 7 < CS_number) { : msg_perr("Only CS 0-7 supported\n"); : return 1; : } > The assignment to `CS_number` can underflow. I'd do the parsing as follows: […] I would just use strtoul() TBH :) -- To view, visit https://review.coreboot.org/c/flashrom/+/56637 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I48ef927aa28433fb0e3b3a1f3fb2e797095e53bd Gerrit-Change-Number: 56637 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 10:51:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: handle PROGRAMFILES(X86) env var properly
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Miklós Márton, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56636 ) Change subject: ni845x_spi: handle PROGRAMFILES(X86) env var properly ...................................................................... Patch Set 1: (1 comment) File Makefile: https://review.coreboot.org/c/flashrom/+/56636/comment/6c9eb95d_267dda5e PS1, Line 750: X Did the casing change? X instead of x -- To view, visit https://review.coreboot.org/c/flashrom/+/56636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Gerrit-Change-Number: 56636 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 17 Aug 2021 10:38:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ni845x_spi: handle PROGRAMFILES(X86) env var properly
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Miklós Márton, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56636 ) Change subject: ni845x_spi: handle PROGRAMFILES(X86) env var properly ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Unfortunately no. […] What I remember from the original review: we used the ${} notation to avoid trouble the clash with the literal parentheses. IIRC, it worked back then? so I wonder what changed :-/ Note, ${} is still expanded by `make`. It just looks like a shell expansion. -- To view, visit https://review.coreboot.org/c/flashrom/+/56636 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I397619a5038567d649a417ce6b9d8ac9e1c8c67b Gerrit-Change-Number: 56636 Gerrit-PatchSet: 1 Gerrit-Owner: Miklós Márton <martonmiklosqdev(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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 17 Aug 2021 10:37:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: internal: Return early from map_flash for >16MiB on x86
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56721 ) Change subject: internal: Return early from map_flash for >16MiB on x86 ...................................................................... Patch Set 3: Code-Review-1 (4 comments) Patchset: PS1: > just noticed this comment. […] Well, the Promontory support there looks like a WIP hack to me. For instance, how should we support >16MiB chips there? The commit message doesn't seem to mention why the memory mapping is used. Patchset: PS3: > Could this get merged? IMHO, fixing the calls is still a much better solution. File physmap.c: https://review.coreboot.org/c/flashrom/+/56721/comment/6f39dd35_e3938237 PS3, Line 264: #if IS_X86 It shouldn't be necessary to use a preprocessor guard. Also, what you probably intend to check is if an internal programmer targeting the BIOS chip on x86 is used. If another programmer is used, the host architecture shouldn't matter. https://review.coreboot.org/c/flashrom/+/56721/comment/e26e4b89_cc37203a PS3, Line 268: c `c` is for chip, while this is programmer code `p`. -- To view, visit https://review.coreboot.org/c/flashrom/+/56721 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e Gerrit-Change-Number: 56721 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Furquan Shaikh <furquan(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 09:52:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: spi_master: Move shutdown function above spi_master struct
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56101 ) Change subject: spi_master: Move shutdown function above spi_master struct ...................................................................... spi_master: Move shutdown function above spi_master struct This patch prepares spi masters to use new API which allows to register shutdown function in spi_master struct. See also later patch in this chain, where spi masters are converted to new API. BUG=b:185191942 TEST=builds and ninja test Comparing flashrom binary before and after the patch, make clean && make CONFIG_EVERYTHING=yes VERSION=none binary is the same Change-Id: I50716686552b4ddcc6089d5afadb19ef59d9f9b4 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/56101 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M bitbang_spi.c M buspirate_spi.c M ch341a_spi.c M dediprog.c M digilent_spi.c M dummyflasher.c M it87spi.c M jlink_spi.c M lspcon_i2c_spi.c M pickit2_spi.c M realtek_mst_i2c_spi.c M sb600spi.c M serprog.c M wbsio_spi.c 14 files changed, 184 insertions(+), 188 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/bitbang_spi.c b/bitbang_spi.c index 3d973c5..83c3501 100644 --- a/bitbang_spi.c +++ b/bitbang_spi.c @@ -131,6 +131,13 @@ return 0; } +static int bitbang_spi_shutdown(void *data) +{ + /* FIXME: Run bitbang_spi_release_bus here or per command? */ + free(data); + return 0; +} + static const struct spi_master spi_master_bitbang = { .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_READ_UNLIMITED, @@ -142,13 +149,6 @@ .write_aai = default_spi_write_aai, }; -static int bitbang_spi_shutdown(void *data) -{ - /* FIXME: Run bitbang_spi_release_bus here or per command? */ - free(data); - return 0; -} - int register_spi_bitbang_master(const struct bitbang_spi_master *master, void *spi_data) { struct spi_master mst = spi_master_bitbang; diff --git a/buspirate_spi.c b/buspirate_spi.c index b56d422..3c0a6ff 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -130,38 +130,6 @@ return ret; } -static struct spi_master spi_master_buspirate = { - .features = SPI_MASTER_4BA, - .max_data_read = MAX_DATA_UNSPECIFIED, - .max_data_write = MAX_DATA_UNSPECIFIED, - .command = NULL, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - -static const struct buspirate_speeds spispeeds[] = { - {"30k", 0x0}, - {"125k", 0x1}, - {"250k", 0x2}, - {"1M", 0x3}, - {"2M", 0x4}, - {"2.6M", 0x5}, - {"4M", 0x6}, - {"8M", 0x7}, - {NULL, 0x0} -}; - -static const struct buspirate_speeds serialspeeds[] = { - {"115200", 115200}, - {"230400", 230400}, - {"250000", 250000}, - {"2000000", 2000000}, - {"2M", 2000000}, - {NULL, 0} -}; - static int buspirate_spi_shutdown(void *data) { struct bp_spi_data *bp_data = data; @@ -204,6 +172,38 @@ return ret; } +static struct spi_master spi_master_buspirate = { + .features = SPI_MASTER_4BA, + .max_data_read = MAX_DATA_UNSPECIFIED, + .max_data_write = MAX_DATA_UNSPECIFIED, + .command = NULL, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + +static const struct buspirate_speeds spispeeds[] = { + {"30k", 0x0}, + {"125k", 0x1}, + {"250k", 0x2}, + {"1M", 0x3}, + {"2M", 0x4}, + {"2.6M", 0x5}, + {"4M", 0x6}, + {"8M", 0x7}, + {NULL, 0x0} +}; + +static const struct buspirate_speeds serialspeeds[] = { + {"115200", 115200}, + {"230400", 230400}, + {"250000", 250000}, + {"2000000", 2000000}, + {"2M", 2000000}, + {NULL, 0} +}; + static int buspirate_spi_send_command_v1(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { diff --git a/ch341a_spi.c b/ch341a_spi.c index 185e582..22c9781 100644 --- a/ch341a_spi.c +++ b/ch341a_spi.c @@ -385,20 +385,6 @@ return 0; } -static const struct spi_master spi_master_ch341a_spi = { - .features = SPI_MASTER_4BA, - /* flashrom's current maximum is 256 B. CH341A was tested on Linux and Windows to accept atleast - * 128 kB. Basically there should be no hard limit because transfers are broken up into USB packets - * sent to the device and most of their payload streamed via SPI. */ - .max_data_read = 4 * 1024, - .max_data_write = 4 * 1024, - .command = ch341a_spi_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - static int ch341a_spi_shutdown(void *data) { if (handle == NULL) @@ -419,6 +405,20 @@ return 0; } +static const struct spi_master spi_master_ch341a_spi = { + .features = SPI_MASTER_4BA, + /* flashrom's current maximum is 256 B. CH341A was tested on Linux and Windows to accept atleast + * 128 kB. Basically there should be no hard limit because transfers are broken up into USB packets + * sent to the device and most of their payload streamed via SPI. */ + .max_data_read = 4 * 1024, + .max_data_write = 4 * 1024, + .command = ch341a_spi_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + static int ch341a_spi_init(void) { if (handle != NULL) { diff --git a/dediprog.c b/dediprog.c index b47e8e9..d98af49 100644 --- a/dediprog.c +++ b/dediprog.c @@ -992,6 +992,24 @@ return millivolt; } +static int dediprog_shutdown(void *data) +{ + dediprog_devicetype = DEV_UNKNOWN; + + /* URB 28. Command Set SPI Voltage to 0. */ + if (dediprog_set_spi_voltage(0x0)) + return 1; + + if (libusb_release_interface(dediprog_handle, 0)) { + msg_perr("Could not release USB interface!\n"); + return 1; + } + libusb_close(dediprog_handle); + libusb_exit(usb_ctx); + + return 0; +} + static struct spi_master spi_master_dediprog = { .features = SPI_MASTER_NO_4BA_MODES, .max_data_read = 16, /* 18 seems to work fine as well, but 19 times out sometimes with FW 5.15. */ @@ -1037,24 +1055,6 @@ return 0; } -static int dediprog_shutdown(void *data) -{ - dediprog_devicetype = DEV_UNKNOWN; - - /* URB 28. Command Set SPI Voltage to 0. */ - if (dediprog_set_spi_voltage(0x0)) - return 1; - - if (libusb_release_interface(dediprog_handle, 0)) { - msg_perr("Could not release USB interface!\n"); - return 1; - } - libusb_close(dediprog_handle); - libusb_exit(usb_ctx); - - return 0; -} - static int dediprog_init(void) { char *voltage, *id_str, *device, *spispeed, *target_str; diff --git a/digilent_spi.c b/digilent_spi.c index 8a3699d..0ec402e 100644 --- a/digilent_spi.c +++ b/digilent_spi.c @@ -315,18 +315,6 @@ return 0; } -static const struct spi_master spi_master_digilent_spi = { - .features = SPI_MASTER_4BA, - .max_data_read = 252, - .max_data_write = 252, - .command = digilent_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - - static int digilent_spi_shutdown(void *data) { struct digilent_spi_data *digilent_data = data; @@ -340,6 +328,17 @@ return 0; } +static const struct spi_master spi_master_digilent_spi = { + .features = SPI_MASTER_4BA, + .max_data_read = 252, + .max_data_write = 252, + .command = digilent_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + static bool default_reset(struct libusb_device_handle *handle) { char board[17]; diff --git a/dummyflasher.c b/dummyflasher.c index 049f9d5..2311576 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -580,7 +580,23 @@ return 0; } - +static int dummy_shutdown(void *data) +{ + msg_pspew("%s\n", __func__); + struct emu_data *emu_data = (struct emu_data *)data; + if (emu_data->emu_chip != EMULATE_NONE) { + if (emu_data->emu_persistent_image && emu_data->emu_modified) { + msg_pdbg("Writing %s\n", emu_data->emu_persistent_image); + write_buf_to_file(emu_data->flashchip_contents, + emu_data->emu_chip_size, + emu_data->emu_persistent_image); + } + free(emu_data->emu_persistent_image); + free(emu_data->flashchip_contents); + } + free(data); + return 0; +} static const struct spi_master spi_master_dummyflasher = { .features = SPI_MASTER_4BA, @@ -604,24 +620,6 @@ .chip_writen = dummy_chip_writen, }; -static int dummy_shutdown(void *data) -{ - msg_pspew("%s\n", __func__); - struct emu_data *emu_data = (struct emu_data *)data; - if (emu_data->emu_chip != EMULATE_NONE) { - if (emu_data->emu_persistent_image && emu_data->emu_modified) { - msg_pdbg("Writing %s\n", emu_data->emu_persistent_image); - write_buf_to_file(emu_data->flashchip_contents, - emu_data->emu_chip_size, - emu_data->emu_persistent_image); - } - free(emu_data->emu_persistent_image); - free(emu_data->flashchip_contents); - } - free(data); - return 0; -} - static int init_data(struct emu_data *data, enum chipbustype *dummy_buses_supported) { diff --git a/it87spi.c b/it87spi.c index cbf830b..e552328 100644 --- a/it87spi.c +++ b/it87spi.c @@ -290,6 +290,12 @@ return 0; } +static int it8716f_shutdown(void *data) +{ + free(data); + return 0; +} + static const struct spi_master spi_master_it87xx = { .max_data_read = 3, .max_data_write = MAX_DATA_UNSPECIFIED, @@ -300,13 +306,6 @@ .write_aai = spi_chip_write_1, }; - -static int it8716f_shutdown(void *data) -{ - free(data); - return 0; -} - static uint16_t it87spi_probe(uint16_t port) { uint8_t tmp = 0; diff --git a/jlink_spi.c b/jlink_spi.c index 8b8533c..daa8eb6 100644 --- a/jlink_spi.c +++ b/jlink_spi.c @@ -155,19 +155,6 @@ return 0; } -static const struct spi_master spi_master_jlink_spi = { - /* Maximum data read size in one go (excluding opcode+address). */ - .max_data_read = JTAG_MAX_TRANSFER_SIZE - 5, - /* Maximum data write size in one go (excluding opcode+address). */ - .max_data_write = JTAG_MAX_TRANSFER_SIZE - 5, - .command = jlink_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, - .features = SPI_MASTER_4BA, -}; - static int jlink_spi_shutdown(void *data) { struct jlink_spi_data *jlink_data = data; @@ -181,6 +168,19 @@ return 0; } +static const struct spi_master spi_master_jlink_spi = { + /* Maximum data read size in one go (excluding opcode+address). */ + .max_data_read = JTAG_MAX_TRANSFER_SIZE - 5, + /* Maximum data write size in one go (excluding opcode+address). */ + .max_data_write = JTAG_MAX_TRANSFER_SIZE - 5, + .command = jlink_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, + .features = SPI_MASTER_4BA, +}; + static int jlink_spi_init(void) { char *arg; diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index b590f1d..e9ff2dd 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -410,16 +410,6 @@ return SPI_GENERIC_ERROR; } -static const struct spi_master spi_master_i2c_lspcon = { - .max_data_read = 16, - .max_data_write = 12, - .command = lspcon_i2c_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = lspcon_i2c_spi_read, - .write_256 = lspcon_i2c_spi_write_256, - .write_aai = lspcon_i2c_spi_write_aai, -}; - static int lspcon_i2c_spi_shutdown(void *data) { int ret = 0; @@ -436,6 +426,16 @@ return ret; } +static const struct spi_master spi_master_i2c_lspcon = { + .max_data_read = 16, + .max_data_write = 12, + .command = lspcon_i2c_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = lspcon_i2c_spi_read, + .write_256 = lspcon_i2c_spi_write_256, + .write_aai = lspcon_i2c_spi_write_aai, +}; + static int lspcon_i2c_spi_init(void) { int fd = i2c_open_from_programmer_params(REGISTER_ADDRESS, 0); diff --git a/pickit2_spi.c b/pickit2_spi.c index bec9457..73ace4b 100644 --- a/pickit2_spi.c +++ b/pickit2_spi.c @@ -340,16 +340,6 @@ return millivolt; } -static const struct spi_master spi_master_pickit2 = { - .max_data_read = 40, - .max_data_write = 40, - .command = pickit2_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = default_spi_read, - .write_256 = default_spi_write_256, - .write_aai = default_spi_write_aai, -}; - static int pickit2_shutdown(void *data) { struct pickit2_spi_data *pickit2_data = data; @@ -388,6 +378,16 @@ return ret; } +static const struct spi_master spi_master_pickit2 = { + .max_data_read = 40, + .max_data_write = 40, + .command = pickit2_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + static int pickit2_spi_init(void) { uint8_t buf[CMD_LENGTH] = { diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index a45838c..4145864 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -410,16 +410,6 @@ return SPI_GENERIC_ERROR; } -static const struct spi_master spi_master_i2c_realtek_mst = { - .max_data_read = 16, - .max_data_write = 8, - .command = realtek_mst_i2c_spi_send_command, - .multicommand = default_spi_send_multicommand, - .read = realtek_mst_i2c_spi_read, - .write_256 = realtek_mst_i2c_spi_write_256, - .write_aai = realtek_mst_i2c_spi_write_aai, -}; - static int realtek_mst_i2c_spi_shutdown(void *data) { int ret = 0; @@ -442,6 +432,16 @@ return ret; } +static const struct spi_master spi_master_i2c_realtek_mst = { + .max_data_read = 16, + .max_data_write = 8, + .command = realtek_mst_i2c_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = realtek_mst_i2c_spi_read, + .write_256 = realtek_mst_i2c_spi_write_256, + .write_aai = realtek_mst_i2c_spi_write_aai, +}; + static int get_params(int *reset, int *enter_isp) { char *reset_str = NULL, *isp_str = NULL; diff --git a/sb600spi.c b/sb600spi.c index c4b25e5..5c65475 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -570,6 +570,17 @@ return 0; } +static int sb600spi_shutdown(void *data) +{ + struct sb600spi_data *sb600_data = data; + struct flashctx *flash = sb600_data->flash; + if (flash) + finalize_flash_access(flash); + + free(data); + return 0; +} + static const struct spi_master spi_master_sb600 = { .max_data_read = FIFO_SIZE_OLD, .max_data_write = FIFO_SIZE_OLD - 3, @@ -600,17 +611,6 @@ .write_aai = default_spi_write_aai, }; -static int sb600spi_shutdown(void *data) -{ - struct sb600spi_data *sb600_data = data; - struct flashctx *flash = sb600_data->flash; - if (flash) - finalize_flash_access(flash); - - free(data); - return 0; -} - int sb600_probe_spi(struct pci_dev *dev) { struct pci_dev *smbus_dev; diff --git a/serprog.c b/serprog.c index e8d1f52..b207fd2 100644 --- a/serprog.c +++ b/serprog.c @@ -394,6 +394,25 @@ return ret; } +static int serprog_shutdown(void *data) +{ + if ((sp_opbuf_usage) || (sp_max_write_n && sp_write_n_bytes)) + if (sp_execute_opbuf() != 0) + msg_pwarn("Could not flush command buffer.\n"); + if (sp_check_commandavail(S_CMD_S_PIN_STATE)) { + uint8_t dis = 0; + if (sp_docommand(S_CMD_S_PIN_STATE, 1, &dis, 0, NULL) == 0) + msg_pdbg(MSGHEADER "Output drivers disabled\n"); + else + msg_pwarn(MSGHEADER "%s: Warning: could not disable output buffers\n", __func__); + } + /* FIXME: fix sockets on windows(?), especially closing */ + serialport_shutdown(&sp_fd); + if (sp_max_write_n) + free(sp_write_n_buf); + return 0; +} + static struct spi_master spi_master_serprog = { .features = SPI_MASTER_4BA, .max_data_read = MAX_DATA_READ_UNLIMITED, @@ -518,25 +537,6 @@ .chip_writen = fallback_chip_writen, }; -static int serprog_shutdown(void *data) -{ - if ((sp_opbuf_usage) || (sp_max_write_n && sp_write_n_bytes)) - if (sp_execute_opbuf() != 0) - msg_pwarn("Could not flush command buffer.\n"); - if (sp_check_commandavail(S_CMD_S_PIN_STATE)) { - uint8_t dis = 0; - if (sp_docommand(S_CMD_S_PIN_STATE, 1, &dis, 0, NULL) == 0) - msg_pdbg(MSGHEADER "Output drivers disabled\n"); - else - msg_pwarn(MSGHEADER "%s: Warning: could not disable output buffers\n", __func__); - } - /* FIXME: fix sockets on windows(?), especially closing */ - serialport_shutdown(&sp_fd); - if (sp_max_write_n) - free(sp_write_n_buf); - return 0; -} - static enum chipbustype serprog_buses_supported = BUS_NONE; static int serprog_init(void) diff --git a/wbsio_spi.c b/wbsio_spi.c index c3814aa..2336845 100644 --- a/wbsio_spi.c +++ b/wbsio_spi.c @@ -177,6 +177,12 @@ return 0; } +static int wbsio_spi_shutdown(void *data) +{ + free(data); + return 0; +} + static const struct spi_master spi_master_wbsio = { .max_data_read = MAX_DATA_UNSPECIFIED, .max_data_write = MAX_DATA_UNSPECIFIED, @@ -187,12 +193,6 @@ .write_aai = spi_chip_write_1, }; -static int wbsio_spi_shutdown(void *data) -{ - free(data); - return 0; -} - int wbsio_check_for_spi(void) { uint16_t wbsio_spibase = 0; -- To view, visit https://review.coreboot.org/c/flashrom/+/56101 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50716686552b4ddcc6089d5afadb19ef59d9f9b4 Gerrit-Change-Number: 56101 Gerrit-PatchSet: 6 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: spi_master: Add shutdown function in spi_master struct
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: spi_master: Add shutdown function in spi_master struct ...................................................................... spi_master: Add shutdown function in spi_master struct With this, register_spi_master can take care of register_shutdown as well, and every spi master only needs to call register_spi_master instead of calling both register_spi_master and register_shutdown. Testing: In dummyflasher, comment out free(data) in shutdown. Test fails with error: ../dummyflasher.c:949: note: block 0x55e0727a6e40 allocated here ERROR: dummy_init_and_shutdown_test_success leaked 1 block(s) Means, shutdown function is invoked for drivers with "old" API (so, transitioning from old to new API is not breaking anything). Next patches in the chain converts spi masters to use new API. BUG=b:185191942 TEST=builds and ninja test Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55932 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M programmer.h M spi.c 2 files changed, 8 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/programmer.h b/programmer.h index 95e2cda..ec55987 100644 --- a/programmer.h +++ b/programmer.h @@ -356,6 +356,7 @@ int (*read)(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len); int (*write_256)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int (*write_aai)(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); + int (*shutdown)(void *data); void *data; }; diff --git a/spi.c b/spi.c index 4ac3914..f2e91c4 100644 --- a/spi.c +++ b/spi.c @@ -135,6 +135,13 @@ { struct registered_master rmst = {0}; + if (mst->shutdown) { + if (register_shutdown(mst->shutdown, data)) { + mst->shutdown(data); /* cleanup */ + return 1; + } + } + if (!mst->write_aai || !mst->write_256 || !mst->read || !mst->command || !mst->multicommand || ((mst->command == default_spi_send_command) && -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 Gerrit-PatchSet: 8 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: spi_master: Use new API to register shutdown function
by Nico Huber (Code Review) 17 Aug '21

17 Aug '21

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56103 ) Change subject: spi_master: Use new API to register shutdown function ...................................................................... Patch Set 6: Code-Review+2 (3 comments) Patchset: PS6: This also fixes a lot resource leakage in case register_shutdown() would fail. That's worth to mention in the commit message! Maybe we can also test it? I haven't looked closely if any of the affected programmer drivers are hooked up for lifecycle testing. If they are, hardcoding register_shutdown() to fail should result in less leakage after this change. File ch341a_spi.c: https://review.coreboot.org/c/flashrom/+/56103/comment/d827b913_fb1204bd PS6, Line 511: return 0; Just raising an eyebrow: this changes unrelated behaviour. Maybe worth to mention in the commit message: Fixes propagation of register_spi_master() return values. File dediprog.c: https://review.coreboot.org/c/flashrom/+/56103/comment/8f15da3c_1009ae7d PS6, Line 1276: /* shutdown function does cleanup */ Still somewhat true, but less obvious what is meant. -- To view, visit https://review.coreboot.org/c/flashrom/+/56103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib60300f9ddb295a255d5ef3f8da0e07064207140 Gerrit-Change-Number: 56103 Gerrit-PatchSet: 6 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: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 17 Aug 2021 09:38:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: Add MX25L12833F
by Weimin Wu (Code Review) 16 Aug '21

16 Aug '21

Attention is currently required from: Nico Huber, Henry Sun, Tao Xia, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Karthik Ramasubramanian. Weimin Wu has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56546 ) Change subject: flashchips: Add MX25L12833F ...................................................................... Patch Set 2: Code-Review+1 (2 comments) Patchset: PS1: > This bug tracker is not public, though. Found it here: […] Thanks. File flashchips.c: https://review.coreboot.org/c/flashrom/+/56546/comment/76233943_bd80dabe PS1, Line 8368: 512B > Yes. The 33F has a 8K-bit OTP. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/56546 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I14ab7e04f5209d2bcf34b0d2de9da2c01bf32d00 Gerrit-Change-Number: 56546 Gerrit-PatchSet: 2 Gerrit-Owner: Tao Xia <xiatao5(a)huaqin.corp-partner.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: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Ginger Zhang <zhangqingchun(a)huaqin.corp-partner.google.com> Gerrit-CC: Krystal Han <hanlijing(a)huaqin.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Tao Xia <xiatao5(a)huaqin.corp-partner.google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Mon, 16 Aug 2021 11:59:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Tao Xia <xiatao5(a)huaqin.corp-partner.google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit August 2021