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

Change in flashrom[master]: hwaccess.c: Fix PowerPC memory barrier
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Stefan Reinauer, Edward O'Callaghan, Angel Pons, Arthur Heymans. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52470 ) Change subject: hwaccess.c: Fix PowerPC memory barrier ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: I had a thought lately about such updates with potential, hard to predict side-effects: We could set some more strict requirements after the next release (1.3). I have already C11 and libpci >= 3.0.0 on my wishlist. We could get rid of some more ugliness, but I'd feel better not to do it 1.5 years after the last release. -- To view, visit https://review.coreboot.org/c/flashrom/+/52470 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia84fd1362a1d39aca6269eef22ac994dee68ba4b Gerrit-Change-Number: 52470 Gerrit-PatchSet: 4 Gerrit-Owner: 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.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: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Sun, 20 Jun 2021 10:58:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: add support for Winbond W25Q01JV
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: David Bartley, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54968 ) Change subject: flashchips: add support for Winbond W25Q01JV ...................................................................... Patch Set 4: Code-Review+1 (3 comments) Patchset: PS4: David, do you happen to have access to the application note about this chip's dual-die nature? For read status register (in particular the write-in-progress (WIP) bit), the datasheet refers to the appli- cation note and the table at 7.3.2 seems to suggest that it depends on previously executed instructions. That shouldn't be a problem as we obviously would poll WIP only after accessing the die in question. But it would be nice to know the details. Please also provide details in the commit message how you tested the chip. File flashchips.c: https://review.coreboot.org/c/flashrom/+/54968/comment/9e8e297c_af2d217f PS1, Line 17757: /* Full chip erase is fastest, typically takes 200s */ > I'd be happy with just a flag that reverses the order, since I think we'd want it for normal writes […] Not sure about a flag. Finding a good solution for the UI might already be half the effort of fixing flashrom. Marking this resolved as the erasers are ordered now. File flashchips.c: https://review.coreboot.org/c/flashrom/+/54968/comment/200b6751_26b0b5cc PS4, Line 17057: FEATURE_4BA The datasheet doesn't seem to mention an "extended address register" (read/written with c8/c5). If this is missing, it should be only FEATURE_4BA_ENTER | FEATURE_4BA_NATIVE -- To view, visit https://review.coreboot.org/c/flashrom/+/54968 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If369409419332070c1fed96ce0e96b7cfe42c169 Gerrit-Change-Number: 54968 Gerrit-PatchSet: 4 Gerrit-Owner: David Bartley <andareed(a)gmail.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-Attention: David Bartley <andareed(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 10:42:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: David Bartley <andareed(a)gmail.com> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: Add support for Macronix MX66L1G45G
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55353 ) Change subject: flashchips: Add support for Macronix MX66L1G45G ...................................................................... flashchips: Add support for Macronix MX66L1G45G Tested on Dediprog SF600: Reading and writing works. Change-Id: I554e828c97d9ec77b08489573a34e176599d2518 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55353 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M flashchips.c M flashchips.h 2 files changed, 50 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index 950f09b..6310c12 100644 --- a/flashchips.c +++ b/flashchips.c @@ -10074,6 +10074,55 @@ .voltage = {2700, 3600}, }, + { + .vendor = "Macronix", + .name = "MX66L1G45G", + .bustype = BUS_SPI, + .manufacture_id = MACRONIX_ID, + .model_id = MACRONIX_MX66L1G45G, + .total_size = 131072, + .page_size = 256, + /* OTP: 512B total; enter 0xB1, exit 0xC1 */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 32768} }, + .block_erase = spi_block_erase_21, + }, { + .eraseblocks = { {4 * 1024, 32768} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 4096} }, + .block_erase = spi_block_erase_5c, + }, { + .eraseblocks = { {32 * 1024, 4096} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 2048} }, + .block_erase = spi_block_erase_dc, + }, { + .eraseblocks = { {64 * 1024, 2048} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {128 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {128 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + /* TODO: security register and SBLK/SBULK, configuration register */ + .printlock = spi_prettyprint_status_register_bp3_srwd, /* bit6 is quad enable */ + .unlock = spi_disable_blockprotect_bp3_srwd, + .write = spi_chip_write_256, + .read = spi_chip_read, /* Fast read (0x0B) supported */ + .voltage = {2700, 3600}, + }, + /* The ST M25P05 is a bit of a problem. It has the same ID as the * ST M25P05-A in RES mode, but supports only 128 byte writes instead * of 256 byte writes. We rely heavily on the fact that probe_spi_res1 diff --git a/flashchips.h b/flashchips.h index 37eadb2..dd985a7 100644 --- a/flashchips.h +++ b/flashchips.h @@ -511,6 +511,7 @@ #define MACRONIX_MX25L1635D 0x2415 #define MACRONIX_MX25L1635E 0x2515 /* MX25L1635{E} */ #define MACRONIX_MX66L51235F 0x201a /* MX66L51235F, MX25L51245G */ +#define MACRONIX_MX66L1G45G 0x201b /* MX66L1G45G */ #define MACRONIX_MX25U8032E 0x2534 #define MACRONIX_MX25U1635E 0x2535 #define MACRONIX_MX25U3235E 0x2536 /* Same as MX25U6435F */ -- To view, visit https://review.coreboot.org/c/flashrom/+/55353 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I554e828c97d9ec77b08489573a34e176599d2518 Gerrit-Change-Number: 55353 Gerrit-PatchSet: 4 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: flashchips: Add support for Macronix MX66L1G45G
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Angel Pons, Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55353 ) Change subject: flashchips: Add support for Macronix MX66L1G45G ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55353 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I554e828c97d9ec77b08489573a34e176599d2518 Gerrit-Change-Number: 55353 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Sun, 20 Jun 2021 09:46:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate_spi.c: Drop bp_ prefix for spi data struct members
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54886 ) Change subject: buspirate_spi.c: Drop bp_ prefix for spi data struct members ...................................................................... buspirate_spi.c: Drop bp_ prefix for spi data struct members The name of the struct type already contains bp_ prefix, so prefix doesn't need to be repeated in members name. BUG=b:185191942 TEST=builds Change-Id: Ia463e3f58497e4b10be072bdd29244f4868098e7 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54886 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 buspirate_spi.c 1 file changed, 13 insertions(+), 13 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/buspirate_spi.c b/buspirate_spi.c index 8c84261..04b8fde 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -51,8 +51,8 @@ #endif struct bp_spi_data { - unsigned char *bp_commbuf; - int bp_commbufsize; + unsigned char *commbuf; + int commbufsize; }; static int buspirate_commbuf_grow(int bufsize, unsigned char **bp_commbuf, int *bp_commbufsize) @@ -165,7 +165,7 @@ static int buspirate_spi_shutdown(void *data) { struct bp_spi_data *bp_data = data; - unsigned char *const bp_commbuf = bp_data->bp_commbuf; + unsigned char *const bp_commbuf = bp_data->commbuf; int ret = 0, ret2 = 0; /* No need to allocate a buffer here, we know that bp_commbuf is at least DEFAULT_BUFSIZE big. */ @@ -215,10 +215,10 @@ return SPI_INVALID_LENGTH; /* 3 bytes extra for CS#, len, CS#. */ - if (buspirate_commbuf_grow(writecnt + readcnt + 3, &bp_data->bp_commbuf, &bp_data->bp_commbufsize)) + if (buspirate_commbuf_grow(writecnt + readcnt + 3, &bp_data->commbuf, &bp_data->commbufsize)) return ERROR_OOM; - unsigned char *const bp_commbuf = bp_data->bp_commbuf; + unsigned char *const bp_commbuf = bp_data->commbuf; /* Assert CS# */ bp_commbuf[i++] = 0x02; @@ -272,10 +272,10 @@ /* 5 bytes extra for command, writelen, readlen. * 1 byte extra for Ack/Nack. */ - if (buspirate_commbuf_grow(max(writecnt + 5, readcnt + 1), &bp_data->bp_commbuf, &bp_data->bp_commbufsize)) + if (buspirate_commbuf_grow(max(writecnt + 5, readcnt + 1), &bp_data->commbuf, &bp_data->commbufsize)) return ERROR_OOM; - unsigned char *const bp_commbuf = bp_data->bp_commbuf; + unsigned char *const bp_commbuf = bp_data->commbuf; /* Combined SPI write/read. */ bp_commbuf[i++] = 0x04; @@ -401,8 +401,8 @@ free(bp_commbuf); return 1; } - bp_data->bp_commbuf = bp_commbuf; - bp_data->bp_commbufsize = bp_commbufsize; + bp_data->commbuf = bp_commbuf; + bp_data->commbufsize = bp_commbufsize; /* This is the brute force version, but it should work. * It is likely to fail if a previous flashrom run was aborted during a write with the new SPI commands @@ -504,8 +504,8 @@ ret = ERROR_OOM; goto init_err_cleanup_exit; } - bp_data->bp_commbuf = bp_commbuf; - bp_data->bp_commbufsize = bp_commbufsize; + bp_data->commbuf = bp_commbuf; + bp_data->commbufsize = bp_commbufsize; spi_master_buspirate.max_data_read = 2048; spi_master_buspirate.max_data_write = 256; spi_master_buspirate.command = buspirate_spi_send_command_v2; @@ -518,8 +518,8 @@ ret = ERROR_OOM; goto init_err_cleanup_exit; } - bp_data->bp_commbuf = bp_commbuf; - bp_data->bp_commbufsize = bp_commbufsize; + bp_data->commbuf = bp_commbuf; + bp_data->commbufsize = bp_commbufsize; spi_master_buspirate.max_data_read = 12; spi_master_buspirate.max_data_write = 12; spi_master_buspirate.command = buspirate_spi_send_command_v1; -- To view, visit https://review.coreboot.org/c/flashrom/+/54886 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia463e3f58497e4b10be072bdd29244f4868098e7 Gerrit-Change-Number: 54886 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: buspirate_spi.c: Refactor singleton states into reentrant pattern
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52958 ) Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern ...................................................................... buspirate_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". BUG=b:185191942 TEST=builds Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52958 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M buspirate_spi.c 1 file changed, 42 insertions(+), 19 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, but someone else must approve diff --git a/buspirate_spi.c b/buspirate_spi.c index 3d81935..8c84261 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -50,26 +50,28 @@ #define sp_flush_incoming(...) 0 #endif -static unsigned char *bp_commbuf = NULL; -static int bp_commbufsize = 0; +struct bp_spi_data { + unsigned char *bp_commbuf; + int bp_commbufsize; +}; -static int buspirate_commbuf_grow(int bufsize) +static int buspirate_commbuf_grow(int bufsize, unsigned char **bp_commbuf, int *bp_commbufsize) { unsigned char *tmpbuf; /* Never shrink. realloc() calls are expensive. */ - if (bufsize <= bp_commbufsize) + if (bufsize <= *bp_commbufsize) return 0; - tmpbuf = realloc(bp_commbuf, bufsize); + tmpbuf = realloc(*bp_commbuf, bufsize); if (!tmpbuf) { /* Keep the existing buffer because memory is already tight. */ msg_perr("Out of memory!\n"); return ERROR_OOM; } - bp_commbuf = tmpbuf; - bp_commbufsize = bufsize; + *bp_commbuf = tmpbuf; + *bp_commbufsize = bufsize; return 0; } @@ -162,6 +164,8 @@ static int buspirate_spi_shutdown(void *data) { + struct bp_spi_data *bp_data = data; + unsigned char *const bp_commbuf = bp_data->bp_commbuf; int ret = 0, ret2 = 0; /* No need to allocate a buffer here, we know that bp_commbuf is at least DEFAULT_BUFSIZE big. */ @@ -189,14 +193,14 @@ /* Keep the oldest error, it is probably the best indicator. */ if (ret2 && !ret) ret = ret2; - bp_commbufsize = 0; + free(bp_commbuf); - bp_commbuf = NULL; if (ret) msg_pdbg("Bus Pirate shutdown failed.\n"); else msg_pdbg("Bus Pirate shutdown completed.\n"); + free(data); return ret; } @@ -205,14 +209,17 @@ { unsigned int i = 0; int ret = 0; + struct bp_spi_data *bp_data = flash->mst->spi.data; if (writecnt > 16 || readcnt > 16 || (readcnt + writecnt) > 16) return SPI_INVALID_LENGTH; /* 3 bytes extra for CS#, len, CS#. */ - if (buspirate_commbuf_grow(writecnt + readcnt + 3)) + if (buspirate_commbuf_grow(writecnt + readcnt + 3, &bp_data->bp_commbuf, &bp_data->bp_commbufsize)) return ERROR_OOM; + unsigned char *const bp_commbuf = bp_data->bp_commbuf; + /* Assert CS# */ bp_commbuf[i++] = 0x02; @@ -257,6 +264,7 @@ const unsigned char *writearr, unsigned char *readarr) { int i = 0, ret = 0; + struct bp_spi_data *bp_data = flash->mst->spi.data; if (writecnt > 4096 || readcnt > 4096 || (readcnt + writecnt) > 4096) return SPI_INVALID_LENGTH; @@ -264,9 +272,11 @@ /* 5 bytes extra for command, writelen, readlen. * 1 byte extra for Ack/Nack. */ - if (buspirate_commbuf_grow(max(writecnt + 5, readcnt + 1))) + if (buspirate_commbuf_grow(max(writecnt + 5, readcnt + 1), &bp_data->bp_commbuf, &bp_data->bp_commbufsize)) return ERROR_OOM; + unsigned char *const bp_commbuf = bp_data->bp_commbuf; + /* Combined SPI write/read. */ bp_commbuf[i++] = 0x04; bp_commbuf[i++] = (writecnt >> 8) & 0xff; @@ -316,6 +326,8 @@ int serialspeed_index = -1; int ret = 0; int pullup = 0; + unsigned char *bp_commbuf; + int bp_commbufsize; dev = extract_programmer_param("dev"); if (dev && !strlen(dev)) { @@ -369,7 +381,6 @@ #define DEFAULT_BUFSIZE (16 + 3) bp_commbuf = malloc(DEFAULT_BUFSIZE); if (!bp_commbuf) { - bp_commbufsize = 0; msg_perr("Out of memory!\n"); free(dev); return ERROR_OOM; @@ -379,12 +390,20 @@ ret = buspirate_serialport_setup(dev); free(dev); if (ret) { - bp_commbufsize = 0; free(bp_commbuf); - bp_commbuf = NULL; return ret; } + + struct bp_spi_data *bp_data = calloc(1, sizeof(*bp_data)); + if (!bp_data) { + msg_perr("Unable to allocate space for SPI master data\n"); + free(bp_commbuf); + return 1; + } + bp_data->bp_commbuf = bp_commbuf; + bp_data->bp_commbufsize = bp_commbufsize; + /* This is the brute force version, but it should work. * It is likely to fail if a previous flashrom run was aborted during a write with the new SPI commands * in firmware v5.5 because that firmware may wait for up to 4096 bytes of input before responding to @@ -481,10 +500,12 @@ if (BP_FWVERSION(fw_version_major, fw_version_minor) >= BP_FWVERSION(5, 5)) { msg_pdbg("Using SPI command set v2.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(260 + 5)) { + if (buspirate_commbuf_grow(260 + 5, &bp_commbuf, &bp_commbufsize)) { ret = ERROR_OOM; goto init_err_cleanup_exit; } + bp_data->bp_commbuf = bp_commbuf; + bp_data->bp_commbufsize = bp_commbufsize; spi_master_buspirate.max_data_read = 2048; spi_master_buspirate.max_data_write = 256; spi_master_buspirate.command = buspirate_spi_send_command_v2; @@ -493,10 +514,12 @@ msg_pinfo("Reading/writing a flash chip may take hours.\n"); msg_pinfo("It is recommended to upgrade to firmware 5.5 or newer.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(16 + 3)) { + if (buspirate_commbuf_grow(16 + 3, &bp_commbuf, &bp_commbufsize)) { ret = ERROR_OOM; goto init_err_cleanup_exit; } + bp_data->bp_commbuf = bp_commbuf; + bp_data->bp_commbufsize = bp_commbufsize; spi_master_buspirate.max_data_read = 12; spi_master_buspirate.max_data_write = 12; spi_master_buspirate.command = buspirate_spi_send_command_v1; @@ -669,16 +692,16 @@ goto init_err_cleanup_exit; } - if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { + if (register_shutdown(buspirate_spi_shutdown, bp_data) != 0) { ret = 1; goto init_err_cleanup_exit; } - register_spi_master(&spi_master_buspirate, NULL); + register_spi_master(&spi_master_buspirate, bp_data); return 0; init_err_cleanup_exit: - buspirate_spi_shutdown(NULL); + buspirate_spi_shutdown(bp_data); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52958 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8 Gerrit-Change-Number: 52958 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: 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]: buspirate_spi.c: Separate shutdown from failed init cleanup
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52877 ) Change subject: buspirate_spi.c: Separate shutdown from failed init cleanup ...................................................................... buspirate_spi.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 BUG=b:185191942 TEST=builds Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52877 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M buspirate_spi.c 1 file changed, 71 insertions(+), 49 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/buspirate_spi.c b/buspirate_spi.c index 87b022f..3d81935 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -385,13 +385,6 @@ return ret; } - if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { - bp_commbufsize = 0; - free(bp_commbuf); - bp_commbuf = NULL; - return 1; - } - /* This is the brute force version, but it should work. * It is likely to fail if a previous flashrom run was aborted during a write with the new SPI commands * in firmware v5.5 because that firmware may wait for up to 4096 bytes of input before responding to @@ -403,7 +396,7 @@ /* Send the command, don't read the response. */ ret = buspirate_sendrecv(bp_commbuf, 1, 0); if (ret) - return ret; + goto init_err_cleanup_exit; /* The old way to handle responses from a Bus Pirate already in BBIO mode was to flush any * response which came in over serial. Unfortunately that does not work reliably on Linux * with FTDI USB-serial. @@ -416,19 +409,19 @@ } /* We know that 20 commands of \0 should elicit at least one BBIO1 response. */ if ((ret = buspirate_wait_for_string(bp_commbuf, "BBIO"))) - return ret; + goto init_err_cleanup_exit; /* Reset the Bus Pirate. */ bp_commbuf[0] = 0x0f; /* Send the command, don't read the response. */ if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, "irate "))) - return ret; + goto init_err_cleanup_exit; /* Read the hardware version string. Last byte of the buffer is reserved for \0. */ for (i = 0; i < DEFAULT_BUFSIZE - 1; i++) { if ((ret = buspirate_sendrecv(bp_commbuf + i, 0, 1))) - return ret; + goto init_err_cleanup_exit; if (strchr("\r\n\t ", bp_commbuf[i])) break; } @@ -449,11 +442,11 @@ msg_pdbg("\n"); if ((ret = buspirate_wait_for_string(bp_commbuf, "irmware "))) - return ret; + goto init_err_cleanup_exit; /* Read the firmware version string. Last byte of the buffer is reserved for \0. */ for (i = 0; i < DEFAULT_BUFSIZE - 1; i++) { if ((ret = buspirate_sendrecv(bp_commbuf + i, 0, 1))) - return ret; + goto init_err_cleanup_exit; if (strchr("\r\n\t ", bp_commbuf[i])) break; } @@ -474,21 +467,24 @@ msg_pdbg("\n"); if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>"))) - return ret; + goto init_err_cleanup_exit; /* Tell the user about missing SPI binary mode in firmware 2.3 and older. */ if (BP_FWVERSION(fw_version_major, fw_version_minor) < BP_FWVERSION(2, 4)) { msg_pinfo("Bus Pirate firmware 2.3 and older does not support binary SPI access.\n"); msg_pinfo("Please upgrade to the latest firmware (at least 2.4).\n"); - return SPI_PROGRAMMER_ERROR; + ret = SPI_PROGRAMMER_ERROR; + goto init_err_cleanup_exit; } /* Use fast SPI mode in firmware 5.5 and newer. */ if (BP_FWVERSION(fw_version_major, fw_version_minor) >= BP_FWVERSION(5, 5)) { msg_pdbg("Using SPI command set v2.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(260 + 5)) - return ERROR_OOM; + if (buspirate_commbuf_grow(260 + 5)) { + ret = ERROR_OOM; + goto init_err_cleanup_exit; + } spi_master_buspirate.max_data_read = 2048; spi_master_buspirate.max_data_write = 256; spi_master_buspirate.command = buspirate_spi_send_command_v2; @@ -497,8 +493,10 @@ msg_pinfo("Reading/writing a flash chip may take hours.\n"); msg_pinfo("It is recommended to upgrade to firmware 5.5 or newer.\n"); /* Sensible default buffer size. */ - if (buspirate_commbuf_grow(16 + 3)) - return ERROR_OOM; + if (buspirate_commbuf_grow(16 + 3)) { + ret = ERROR_OOM; + goto init_err_cleanup_exit; + } spi_master_buspirate.max_data_read = 12; spi_master_buspirate.max_data_write = 12; spi_master_buspirate.command = buspirate_spi_send_command_v1; @@ -538,37 +536,37 @@ /* Enter baud rate configuration mode */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "b\n"); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, ">"))) - return ret; + goto init_err_cleanup_exit; /* Enter manual clock divisor entry mode */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "10\n"); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, ">"))) - return ret; + goto init_err_cleanup_exit; /* Set the clock divisor to the value calculated from the user's input */ cnt = snprintf((char *)bp_commbuf, DEFAULT_BUFSIZE, "%d\n", BP_DIVISOR(serialspeeds[serialspeed_index].speed)); if ((ret = buspirate_sendrecv(bp_commbuf, cnt, 0))) - return ret; + goto init_err_cleanup_exit; sleep(1); /* Reconfigure the host's serial baud rate to the new value */ if ((ret = serialport_config(sp_fd, serialspeeds[serialspeed_index].speed))) { msg_perr("Unable to configure system baud rate to specified value."); - return ret; + goto init_err_cleanup_exit; } /* Return to the main prompt */ bp_commbuf[0] = ' '; if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_wait_for_string(bp_commbuf, "HiZ>"))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Serial speed is %d baud\n", serialspeeds[serialspeed_index].speed); } @@ -581,30 +579,34 @@ for (i = 0; i < 20; i++) { bp_commbuf[0] = 0x00; if ((ret = buspirate_sendrecv(bp_commbuf, 1, 0))) - return ret; + goto init_err_cleanup_exit; } if ((ret = buspirate_wait_for_string(bp_commbuf, "BBIO"))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_sendrecv(bp_commbuf, 0, 1))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Raw bitbang mode version %c\n", bp_commbuf[0]); if (bp_commbuf[0] != '1') { msg_perr("Can't handle raw bitbang mode version %c!\n", bp_commbuf[0]); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Enter raw SPI mode */ bp_commbuf[0] = 0x01; ret = buspirate_sendrecv(bp_commbuf, 1, 0); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if ((ret = buspirate_wait_for_string(bp_commbuf, "SPI"))) - return ret; + goto init_err_cleanup_exit; if ((ret = buspirate_sendrecv(bp_commbuf, 0, 1))) - return ret; + goto init_err_cleanup_exit; msg_pdbg("Raw SPI mode version %c\n", bp_commbuf[0]); if (bp_commbuf[0] != '1') { msg_perr("Can't handle raw SPI mode version %c!\n", bp_commbuf[0]); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Initial setup (SPI peripherals config): Enable power, CS high, AUX */ @@ -614,21 +616,27 @@ msg_pdbg("Enabling pull-up resistors.\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting power/CS/AUX(/Pull-up resistors)!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Set SPI speed */ bp_commbuf[0] = 0x60 | spispeed; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI speed!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* Set SPI config: output type, idle, clock edge, sample */ @@ -638,26 +646,40 @@ msg_pdbg("Pull-ups enabled, so using HiZ pin output! (Open-Drain mode)\n"); } ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while setting SPI config!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } /* De-assert CS# */ bp_commbuf[0] = 0x03; ret = buspirate_sendrecv(bp_commbuf, 1, 1); - if (ret) - return 1; + if (ret) { + ret = 1; + goto init_err_cleanup_exit; + } if (bp_commbuf[0] != 0x01) { msg_perr("Protocol error while raising CS#!\n"); - return 1; + ret = 1; + goto init_err_cleanup_exit; } + if (register_shutdown(buspirate_spi_shutdown, NULL) != 0) { + ret = 1; + goto init_err_cleanup_exit; + } register_spi_master(&spi_master_buspirate, NULL); return 0; + +init_err_cleanup_exit: + buspirate_spi_shutdown(NULL); + return ret; } const struct programmer_entry programmer_buspirate_spi = { -- To view, visit https://review.coreboot.org/c/flashrom/+/52877 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d57e2552d23a1a4a906c68539f0ccefc13e8a0 Gerrit-Change-Number: 52877 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: 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-MessageType: merged

1 0

Change in flashrom[master]: libflashrom: remove flashrom_system_info
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55351 ) Change subject: libflashrom: remove flashrom_system_info ...................................................................... libflashrom: remove flashrom_system_info void flashrom_system_info(void) has no implementation nor any relation to existing code. Change-Id: Iad805322aeead149a8d51ad8bf0babde2a12f01f Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55351 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 libflashrom.h M libflashrom.map 2 files changed, 0 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/libflashrom.h b/libflashrom.h index f2b3e46..eab78ad 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -75,7 +75,6 @@ }; const char *flashrom_version_info(void); -void flashrom_system_info(void); struct flashrom_flashchip_info *flashrom_supported_flash_chips(void); struct flashrom_board_info *flashrom_supported_boards(void); struct flashrom_chipset_info *flashrom_supported_chipsets(void); diff --git a/libflashrom.map b/libflashrom.map index 0c28a68..feb20f8 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -24,7 +24,6 @@ flashrom_programmer_shutdown; flashrom_set_log_callback; flashrom_shutdown; - flashrom_system_info; flashrom_version_info; local: *; }; -- To view, visit https://review.coreboot.org/c/flashrom/+/55351 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iad805322aeead149a8d51ad8bf0babde2a12f01f Gerrit-Change-Number: 55351 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> 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-MessageType: merged

1 0

Change in flashrom[master]: libflashrom: remove flashrom_supported_programmers
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55350 ) Change subject: libflashrom: remove flashrom_supported_programmers ...................................................................... libflashrom: remove flashrom_supported_programmers const char **flashrom_supported_programmers(void) returns an array of strings without returning the array size or making a NULL termination. This can lead to undefined behavior when iterating over the array. Change-Id: I0157926a654e337c14d840dd398e5576471c304f Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55350 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M libflashrom.c M libflashrom.h M libflashrom.map 3 files changed, 0 insertions(+), 22 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved diff --git a/libflashrom.c b/libflashrom.c index 67d3ef3..d999efc 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -114,26 +114,6 @@ } /** - * @brief Returns list of supported programmers - * @return List of supported programmers, or NULL if an error occurred - */ -const char **flashrom_supported_programmers(void) -{ - size_t p = 0; - const char **supported_programmers = malloc((programmer_table_size + 1) * sizeof(char*)); - - if (supported_programmers != NULL) { - for (; p < programmer_table_size; ++p) { - supported_programmers[p] = programmer_table[p]->name; - } - } else { - msg_gerr("Memory allocation error!\n"); - } - - return supported_programmers; -} - -/** * @brief Returns list of supported flash chips * @return List of supported flash chips, or NULL if an error occurred */ diff --git a/libflashrom.h b/libflashrom.h index d0d5826..f2b3e46 100644 --- a/libflashrom.h +++ b/libflashrom.h @@ -76,7 +76,6 @@ const char *flashrom_version_info(void); void flashrom_system_info(void); -const char **flashrom_supported_programmers(void); struct flashrom_flashchip_info *flashrom_supported_flash_chips(void); struct flashrom_board_info *flashrom_supported_boards(void); struct flashrom_chipset_info *flashrom_supported_chipsets(void); diff --git a/libflashrom.map b/libflashrom.map index d6dd24d..0c28a68 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -24,7 +24,6 @@ flashrom_programmer_shutdown; flashrom_set_log_callback; flashrom_shutdown; - flashrom_supported_programmers; flashrom_system_info; flashrom_version_info; local: *; -- To view, visit https://review.coreboot.org/c/flashrom/+/55350 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0157926a654e337c14d840dd398e5576471c304f Gerrit-Change-Number: 55350 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: ?ukasz Dmitrowski <lukasz.dmitrowski(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: libflashrom: remove flashrom_supported_programmers
by Nico Huber (Code Review) 20 Jun '21

20 Jun '21

Attention is currently required from: Michał Żygowski, ?ukasz Dmitrowski, Thomas Heijligen, David Hendricks, Piotr Król. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55350 ) Change subject: libflashrom: remove flashrom_supported_programmers ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55350 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0157926a654e337c14d840dd398e5576471c304f Gerrit-Change-Number: 55350 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: ?ukasz Dmitrowski <lukasz.dmitrowski(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: ?ukasz Dmitrowski <lukasz.dmitrowski(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com> Gerrit-Comment-Date: Sun, 20 Jun 2021 09:09:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021