- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: ni845x_spi: refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 ) Change subject: ni845x_spi: refactor singleton states into reentrant pattern ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: Miklos, could you please help to review and test this chain of patches? I think testing can only be done once, at the end of the chain. But there are few questions to you in the comments of the first one in the chain CB:72154 , just to check that you are fine with the change? Thank you so much! -- To view, visit https://review.coreboot.org/c/flashrom/+/72160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484 Gerrit-Change-Number: 72160 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 13 Apr 2023 10:41:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: ni845x_spi: handle errors using goto during initialization
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72156 ) Change subject: ni845x_spi: handle errors using goto during initialization ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72156 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie9620d59db229729fd8523f99b0917d938bcc4ed Gerrit-Change-Number: 72156 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 13 Apr 2023 10:13:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: layout.c: Ensure filename is always consistent in layout structure
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72943 ) Change subject: layout.c: Ensure filename is always consistent in layout structure ...................................................................... layout.c: Ensure filename is always consistent in layout structure Ensure construction and extraction filenames are symmetrical consistently within the layout structure. Change-Id: I9a0c3130c0e7d88a8a69fd29362c338e20d2bae8 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/72943 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M layout.c 1 file changed, 28 insertions(+), 8 deletions(-) Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/layout.c b/layout.c index 4f02be6..2d7747e 100644 --- a/layout.c +++ b/layout.c @@ -194,6 +194,15 @@ return 1; } +static char *sanitise_filename(char *filename) +{ + for (unsigned i = 0; filename[i]; i++) { + if (isspace((unsigned char)filename[i])) + filename[i] = '_'; + } + return filename; +} + /* returns 0 to indicate success, 1 to indicate failure */ static int include_region(struct flashrom_layout *const l, const char *name, const char *file) @@ -202,7 +211,7 @@ if (entry) { entry->included = true; if (file) - entry->file = strdup(file); + entry->file = sanitise_filename(strdup(file)); return 0; } return 1; @@ -342,18 +351,12 @@ { const struct flashrom_layout *const l = get_layout(flash); struct romentry *entry = NULL; - unsigned int i; while ((entry = mutable_layout_next(l, entry))) { entry->included = true; if (!entry->file) - entry->file = strdup(entry->region.name); - - for (i = 0; entry->file[i]; ++i) { - if (isspace((unsigned char)entry->file[i])) - entry->file[i] = '_'; - } + entry->file = sanitise_filename(strdup(entry->region.name)); } } -- To view, visit https://review.coreboot.org/c/flashrom/+/72943 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a0c3130c0e7d88a8a69fd29362c338e20d2bae8 Gerrit-Change-Number: 72943 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: layout.c: Ensure filename is always consistent in layout structure
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72943 ) Change subject: layout.c: Ensure filename is always consistent in layout structure ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/72943 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a0c3130c0e7d88a8a69fd29362c338e20d2bae8 Gerrit-Change-Number: 72943 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Thu, 13 Apr 2023 09:45:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate: Add option for setting the aux pin
by Sean Anderson (Code Review) 13 Apr '23

13 Apr '23

Sean Anderson has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43608 ) Change subject: buspirate: Add option for setting the aux pin ...................................................................... buspirate: Add option for setting the aux pin This adds a parameter to drive the aux pin low (or high if you explicitly want the previous behavior). Some boards need to have a reset pin driven low before the firmware can be safely flashed. With the Bus Pirate, this is most easily done with the auxiliary pin. Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775 Signed-off-by: Sean Anderson <seanga2(a)gmail.com> --- M buspirate_spi.c M flashrom.8.tmpl 2 files changed, 33 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/43608/1 diff --git a/buspirate_spi.c b/buspirate_spi.c index fdfc0e4..1ccb107 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -228,6 +228,7 @@ int serialspeed_index = -1; int ret = 0; int pullup = 0; + int aux = 1; dev = extract_programmer_param("dev"); if (dev && !strlen(dev)) { @@ -277,6 +278,17 @@ } free(tmp); + tmp = extract_programmer_param("aux"); + if (tmp) { + if (strcasecmp("high", tmp) == 0) + ; /* Default */ + else if (strcasecmp("low", tmp) == 0) + aux = 0; + else + msg_perr("Invalid AUX state, driving high by default.\n"); + } + free(tmp); + /* Default buffer size is 19: 16 bytes data, 3 bytes control. */ #define DEFAULT_BUFSIZE (16 + 3) bp_commbuf = malloc(DEFAULT_BUFSIZE); @@ -520,11 +532,18 @@ } /* Initial setup (SPI peripherals config): Enable power, CS high, AUX */ - bp_commbuf[0] = 0x40 | 0x0b; + bp_commbuf[0] = 0x40 | 0x09; if (pullup == 1) { bp_commbuf[0] |= (1 << 2); msg_pdbg("Enabling pull-up resistors.\n"); } + if (aux) { + bp_commbuf[0] |= (1 << 1); + msg_pdbg("Driving AUX high.\n"); + } else { + msg_pdbg("Driving AUX low.\n"); + } + ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) return 1; diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index db50d59..caf860c 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -902,6 +902,19 @@ .URLB "http://dangerousprototypes.com/docs/Practical_guide_to_Bus_Pirate_pull-up_r…" \ "in a guide by dangerousprototypes" . Only the external supply voltage (Vpu) is supported as of this writing. +.sp +An optional aux parameter specifies the state of the Bus Pirate auxiliary pin. +This may be used to drive the auxiliary pin high or low before a transfer. +Syntax is +.sp +.B " flashrom -p buspirate_spi:aux=state" +.sp +where +.B state +can be +.BR high " or " low . +The default +.BR state " is " high . .SS .BR "pickit2_spi " programmer .IP -- To view, visit https://review.coreboot.org/c/flashrom/+/43608 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieeecfdf1afc06dadda9b8f99547cd74854ca6775 Gerrit-Change-Number: 43608 Gerrit-PatchSet: 1 Gerrit-Owner: Sean Anderson <seanga2(a)gmail.com> Gerrit-MessageType: newchange

6 14

[S] Change in flashrom[master]: ch341a_spi: drop validation of handle in routines
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72808 ) Change subject: ch341a_spi: drop validation of handle in routines ...................................................................... ch341a_spi: drop validation of handle in routines Since the handle is no longer a global variable and only exists during the lifetime of the driver, we can stop checking if the handle equals NULL. Change-Id: I1872495b83a522ceced331fef35d9d9d3b43fce5 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Reviewed-on: https://review.coreboot.org/c/flashrom/+/72808 Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M ch341a_spi.c 1 file changed, 18 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/ch341a_spi.c b/ch341a_spi.c index 760a354..8f42f09 100644 --- a/ch341a_spi.c +++ b/ch341a_spi.c @@ -137,9 +137,6 @@ static int32_t usb_transfer(const struct ch341a_spi_data *data, const char *func, unsigned int writecnt, unsigned int readcnt, const uint8_t *writearr, uint8_t *readarr) { - if (data->handle == NULL) - return -1; - int state_out = TRANS_IDLE; data->transfer_out->buffer = (uint8_t*)writearr; data->transfer_out->length = writecnt; @@ -262,9 +259,6 @@ * Set the SPI bus data width (speed(b2): 0 = Single, 1 = Double). */ static int32_t config_stream(const struct ch341a_spi_data *data, uint32_t speed) { - if (data->handle == NULL) - return -1; - uint8_t buf[] = { CH341A_CMD_I2C_STREAM, CH341A_CMD_I2C_STM_SET | (speed & 0x7), @@ -344,8 +338,6 @@ static int ch341a_spi_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { struct ch341a_spi_data *data = flash->mst->spi.data; - if (data->handle == NULL) - return -1; /* How many packets ... */ const size_t packets = (writecnt + readcnt + CH341_PACKET_LENGTH - 2) / (CH341_PACKET_LENGTH - 1); @@ -394,8 +386,6 @@ static int ch341a_spi_shutdown(void *data) { struct ch341a_spi_data *ch341a_data = data; - if (ch341a_data->handle == NULL) - return -1; enable_pins(ch341a_data, false); libusb_free_transfer(ch341a_data->transfer_out); -- To view, visit https://review.coreboot.org/c/flashrom/+/72808 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1872495b83a522ceced331fef35d9d9d3b43fce5 Gerrit-Change-Number: 72808 Gerrit-PatchSet: 4 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: ch341a_spi: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/72807 ) Change subject: ch341a_spi: Refactor singleton states into reentrant pattern ...................................................................... ch341a_spi: 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 patchset also includes stdlib.h to be able to work with memory allocation. Pass programmer's data as a parameter to functions that need it. 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 specified below. TOPIC=register_master_api TEST=Tested on flash W25Q128JVSQ flashrom -E # Result: success flashrom -v ff.bin # Result: verified flashrom -w firmware.bin # Result: success flashrom -v firmware.bin # Result: verified Change-Id: I9fe72bff88b7f8778c1199bdab8ba43bf32e1c8c Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/391 Reviewed-on: https://review.coreboot.org/c/flashrom/+/72807 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M ch341a_spi.c 1 file changed, 116 insertions(+), 78 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/ch341a_spi.c b/ch341a_spi.c index a66503d..760a354 100644 --- a/ch341a_spi.c +++ b/ch341a_spi.c @@ -17,6 +17,7 @@ * GNU General Public License for more details. */ +#include <stdlib.h> #include <string.h> #include <libusb.h> #include "flash.h" @@ -72,16 +73,18 @@ /* Number of parallel IN transfers. 32 seems to produce the most stable throughput on Windows. */ #define USB_IN_TRANSFERS 32 -/* We need to use many queued IN transfers for any resemblance of performance (especially on Windows) - * because USB spec says that transfers end on non-full packets and the device sends the 31 reply - * data bytes to each 32-byte packet with command + 31 bytes of data... */ -static struct libusb_transfer *transfer_out = NULL; -static struct libusb_transfer *transfer_ins[USB_IN_TRANSFERS] = {0}; +struct ch341a_spi_data { + struct libusb_device_handle *handle; -/* Accumulate delays to be plucked between CS deassertion and CS assertions. */ -static unsigned int stored_delay_us = 0; + /* We need to use many queued IN transfers for any resemblance of performance (especially on Windows) + * because USB spec says that transfers end on non-full packets and the device sends the 31 reply + * data bytes to each 32-byte packet with command + 31 bytes of data... */ + struct libusb_transfer *transfer_out; + struct libusb_transfer *transfer_ins[USB_IN_TRANSFERS]; -static struct libusb_device_handle *handle = NULL; + /* Accumulate delays to be plucked between CS deassertion and CS assertions. */ + unsigned int stored_delay_us; +}; static const struct dev_entry devs_ch341a_spi[] = { {0x1A86, 0x5512, OK, "Winchiphead (WCH)", "CH341A"}, @@ -131,20 +134,21 @@ cb_common(__func__, transfer); } -static int32_t usb_transfer(const char *func, unsigned int writecnt, unsigned int readcnt, const uint8_t *writearr, uint8_t *readarr) +static int32_t usb_transfer(const struct ch341a_spi_data *data, const char *func, + unsigned int writecnt, unsigned int readcnt, const uint8_t *writearr, uint8_t *readarr) { - if (handle == NULL) + if (data->handle == NULL) return -1; int state_out = TRANS_IDLE; - transfer_out->buffer = (uint8_t*)writearr; - transfer_out->length = writecnt; - transfer_out->user_data = &state_out; + data->transfer_out->buffer = (uint8_t*)writearr; + data->transfer_out->length = writecnt; + data->transfer_out->user_data = &state_out; /* Schedule write first */ if (writecnt > 0) { state_out = TRANS_ACTIVE; - int ret = libusb_submit_transfer(transfer_out); + int ret = libusb_submit_transfer(data->transfer_out); if (ret) { msg_perr("%s: failed to submit OUT transfer: %s\n", func, libusb_error_name(ret)); state_out = TRANS_ERR; @@ -165,10 +169,10 @@ /* Schedule new reads as long as there are free transfers and unscheduled bytes to read. */ while ((in_done + in_active) < readcnt && state_in[free_idx] == TRANS_IDLE) { unsigned int cur_todo = min(CH341_PACKET_LENGTH - 1, readcnt - in_done - in_active); - transfer_ins[free_idx]->length = cur_todo; - transfer_ins[free_idx]->buffer = in_buf; - transfer_ins[free_idx]->user_data = &state_in[free_idx]; - int ret = libusb_submit_transfer(transfer_ins[free_idx]); + data->transfer_ins[free_idx]->length = cur_todo; + data->transfer_ins[free_idx]->buffer = in_buf; + data->transfer_ins[free_idx]->user_data = &state_in[free_idx]; + int ret = libusb_submit_transfer(data->transfer_ins[free_idx]); if (ret) { state_in[free_idx] = TRANS_ERR; msg_perr("%s: failed to submit IN transfer: %s\n", @@ -223,14 +227,14 @@ (state_out == TRANS_ERR) ? writecnt : readcnt); /* First, we must cancel any ongoing requests and wait for them to be canceled. */ if ((writecnt > 0) && (state_out == TRANS_ACTIVE)) { - if (libusb_cancel_transfer(transfer_out) != 0) + if (libusb_cancel_transfer(data->transfer_out) != 0) state_out = TRANS_ERR; } if (readcnt > 0) { unsigned int i; for (i = 0; i < USB_IN_TRANSFERS; i++) { if (state_in[i] == TRANS_ACTIVE) - if (libusb_cancel_transfer(transfer_ins[i]) != 0) + if (libusb_cancel_transfer(data->transfer_ins[i]) != 0) state_in[i] = TRANS_ERR; } } @@ -256,9 +260,9 @@ /* Set the I2C bus speed (speed(b1b0): 0 = 20kHz; 1 = 100kHz, 2 = 400kHz, 3 = 750kHz). * Set the SPI bus data width (speed(b2): 0 = Single, 1 = Double). */ -static int32_t config_stream(uint32_t speed) +static int32_t config_stream(const struct ch341a_spi_data *data, uint32_t speed) { - if (handle == NULL) + if (data->handle == NULL) return -1; uint8_t buf[] = { @@ -267,7 +271,7 @@ CH341A_CMD_I2C_STM_END }; - int32_t ret = usb_transfer(__func__, sizeof(buf), 0, buf, NULL); + int32_t ret = usb_transfer(data, __func__, sizeof(buf), 0, buf, NULL); if (ret < 0) { msg_perr("Could not configure stream interface.\n"); } @@ -287,7 +291,7 @@ * D6/21 unused (DIN2) * D7/22 SO/2 (DIN) */ -static int32_t enable_pins(bool enable) +static int32_t enable_pins(const struct ch341a_spi_data *data, bool enable) { uint8_t buf[] = { CH341A_CMD_UIO_STREAM, @@ -296,7 +300,7 @@ CH341A_CMD_UIO_STM_END, }; - int32_t ret = usb_transfer(__func__, sizeof(buf), 0, buf, NULL); + int32_t ret = usb_transfer(data, __func__, sizeof(buf), 0, buf, NULL); if (ret < 0) { msg_perr("Could not %sable output pins.\n", enable ? "en" : "dis"); } @@ -304,14 +308,14 @@ } /* De-assert and assert CS in one operation. */ -static void pluck_cs(uint8_t *ptr) +static void pluck_cs(uint8_t *ptr, unsigned int *stored_delay_us) { /* This was measured to give a minimum deassertion time of 2.25 us, * >20x more than needed for most SPI chips (100ns). */ int delay_cnt = 2; - if (stored_delay_us) { - delay_cnt = (stored_delay_us * 4) / 3; - stored_delay_us = 0; + if (*stored_delay_us) { + delay_cnt = (*stored_delay_us * 4) / 3; + *stored_delay_us = 0; } *ptr++ = CH341A_CMD_UIO_STREAM; *ptr++ = CH341A_CMD_UIO_STM_OUT | 0x37; /* deasserted */ @@ -324,20 +328,23 @@ static void ch341a_spi_delay(const struct flashctx *flash, unsigned int usecs) { + struct ch341a_spi_data *data = flash->mst->spi.data; + /* There is space for 28 bytes instructions of 750 ns each in the CS packet (32 - 4 for the actual CS * instructions), thus max 21 us, but we avoid getting too near to this boundary and use * default_delay() for durations over 20 us. */ - if ((usecs + stored_delay_us) > 20) { - unsigned int inc = 20 - stored_delay_us; + if ((usecs + data->stored_delay_us) > 20) { + unsigned int inc = 20 - data->stored_delay_us; default_delay(usecs - inc); usecs = inc; } - stored_delay_us += usecs; + data->stored_delay_us += usecs; } static int ch341a_spi_spi_send_command(const struct flashctx *flash, unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr) { - if (handle == NULL) + struct ch341a_spi_data *data = flash->mst->spi.data; + if (data->handle == NULL) return -1; /* How many packets ... */ @@ -352,7 +359,7 @@ uint8_t *ptr = wbuf[0]; /* CS usage is optimized by doing both transitions in one packet. * Final transition to deselected state is in the pin disable. */ - pluck_cs(ptr); + pluck_cs(ptr, &data->stored_delay_us); unsigned int write_left = writecnt; unsigned int read_left = readcnt; unsigned int p; @@ -371,7 +378,7 @@ write_left -= write_now; } - int32_t ret = usb_transfer(__func__, CH341_PACKET_LENGTH + packets + writecnt + readcnt, + int32_t ret = usb_transfer(data, __func__, CH341_PACKET_LENGTH + packets + writecnt + readcnt, writecnt + readcnt, wbuf[0], rbuf); if (ret < 0) return -1; @@ -386,22 +393,21 @@ static int ch341a_spi_shutdown(void *data) { - if (handle == NULL) + struct ch341a_spi_data *ch341a_data = data; + if (ch341a_data->handle == NULL) return -1; - enable_pins(false); - libusb_free_transfer(transfer_out); - transfer_out = NULL; + enable_pins(ch341a_data, false); + libusb_free_transfer(ch341a_data->transfer_out); int i; - for (i = 0; i < USB_IN_TRANSFERS; i++) { - libusb_free_transfer(transfer_ins[i]); - transfer_ins[i] = NULL; - } - libusb_release_interface(handle, 0); - libusb_attach_kernel_driver(handle, 0); - libusb_close(handle); + for (i = 0; i < USB_IN_TRANSFERS; i++) + libusb_free_transfer(ch341a_data->transfer_ins[i]); + libusb_release_interface(ch341a_data->handle, 0); + libusb_attach_kernel_driver(ch341a_data->handle, 0); + libusb_close(ch341a_data->handle); libusb_exit(NULL); - handle = NULL; + + free(data); return 0; } @@ -421,11 +427,6 @@ static int ch341a_spi_init(const struct programmer_cfg *cfg) { - if (handle != NULL) { - msg_cerr("%s: handle already set! Please report a bug at flashrom(a)flashrom.org\n", __func__); - return -1; - } - int32_t ret = libusb_init(NULL); if (ret < 0) { msg_perr("Couldn't initialize libusb!\n"); @@ -439,27 +440,33 @@ libusb_set_option(NULL, LIBUSB_OPTION_LOG_LEVEL, LIBUSB_LOG_LEVEL_INFO); #endif - uint16_t vid = devs_ch341a_spi[0].vendor_id; - uint16_t pid = devs_ch341a_spi[0].device_id; - handle = libusb_open_device_with_vid_pid(NULL, vid, pid); - if (handle == NULL) { - msg_perr("Couldn't open device %04x:%04x.\n", vid, pid); - return -1; + struct ch341a_spi_data *data = calloc(1, sizeof(*data)); + if (!data) { + msg_perr("Out of memory!\n"); + return 1; } - ret = libusb_detach_kernel_driver(handle, 0); + uint16_t vid = devs_ch341a_spi[0].vendor_id; + uint16_t pid = devs_ch341a_spi[0].device_id; + data->handle = libusb_open_device_with_vid_pid(NULL, vid, pid); + if (data->handle == NULL) { + msg_perr("Couldn't open device %04x:%04x.\n", vid, pid); + goto free_data; + } + + ret = libusb_detach_kernel_driver(data->handle, 0); if (ret != 0 && ret != LIBUSB_ERROR_NOT_FOUND) msg_pwarn("Cannot detach the existing USB driver. Claiming the interface may fail. %s\n", libusb_error_name(ret)); - ret = libusb_claim_interface(handle, 0); + ret = libusb_claim_interface(data->handle, 0); if (ret != 0) { msg_perr("Failed to claim interface 0: '%s'\n", libusb_error_name(ret)); goto close_handle; } struct libusb_device *dev; - if (!(dev = libusb_get_device(handle))) { + if (!(dev = libusb_get_device(data->handle))) { msg_perr("Failed to get device from device handle.\n"); goto close_handle; } @@ -477,44 +484,43 @@ (desc.bcdDevice >> 0) & 0x000F); /* Allocate and pre-fill transfer structures. */ - transfer_out = libusb_alloc_transfer(0); - if (!transfer_out) { + data->transfer_out = libusb_alloc_transfer(0); + if (!data->transfer_out) { msg_perr("Failed to alloc libusb OUT transfer\n"); goto release_interface; } int i; for (i = 0; i < USB_IN_TRANSFERS; i++) { - transfer_ins[i] = libusb_alloc_transfer(0); - if (transfer_ins[i] == NULL) { + data->transfer_ins[i] = libusb_alloc_transfer(0); + if (data->transfer_ins[i] == NULL) { msg_perr("Failed to alloc libusb IN transfer %d\n", i); goto dealloc_transfers; } } /* We use these helpers but dont fill the actual buffer yet. */ - libusb_fill_bulk_transfer(transfer_out, handle, WRITE_EP, NULL, 0, cb_out, NULL, USB_TIMEOUT); + libusb_fill_bulk_transfer(data->transfer_out, data->handle, WRITE_EP, NULL, 0, cb_out, NULL, USB_TIMEOUT); for (i = 0; i < USB_IN_TRANSFERS; i++) - libusb_fill_bulk_transfer(transfer_ins[i], handle, READ_EP, NULL, 0, cb_in, NULL, USB_TIMEOUT); + libusb_fill_bulk_transfer(data->transfer_ins[i], data->handle, READ_EP, NULL, 0, cb_in, NULL, USB_TIMEOUT); - if ((config_stream(CH341A_STM_I2C_100K) < 0) || (enable_pins(true) < 0)) + if ((config_stream(data, CH341A_STM_I2C_100K) < 0) || (enable_pins(data, true) < 0)) goto dealloc_transfers; - return register_spi_master(&spi_master_ch341a_spi, NULL); + return register_spi_master(&spi_master_ch341a_spi, data); dealloc_transfers: for (i = 0; i < USB_IN_TRANSFERS; i++) { - if (transfer_ins[i] == NULL) + if (data->transfer_ins[i] == NULL) break; - libusb_free_transfer(transfer_ins[i]); - transfer_ins[i] = NULL; + libusb_free_transfer(data->transfer_ins[i]); } - libusb_free_transfer(transfer_out); - transfer_out = NULL; + libusb_free_transfer(data->transfer_out); release_interface: - libusb_release_interface(handle, 0); + libusb_release_interface(data->handle, 0); close_handle: - libusb_attach_kernel_driver(handle, 0); - libusb_close(handle); - handle = NULL; + libusb_attach_kernel_driver(data->handle, 0); + libusb_close(data->handle); +free_data: + free(data); return -1; } -- To view, visit https://review.coreboot.org/c/flashrom/+/72807 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9fe72bff88b7f8778c1199bdab8ba43bf32e1c8c Gerrit-Change-Number: 72807 Gerrit-PatchSet: 7 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[master]: cli_classic: refactor arguments parsing into separate func
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/66343 ) Change subject: cli_classic: refactor arguments parsing into separate func ...................................................................... cli_classic: refactor arguments parsing into separate func Move variables that represent parsed options to `cli_options` structure. This patchset also introduces the following functions: - parse_options() which parses command line arguments and fills the structure. - free_options() that releases an allocated data from the structure. This is one of the steps on the way to simplify the main function by using helper functions with descriptive names. TOPIC=split_main_func TEST=the following scenarious run successfully ./flashrom -p dummy:emulate=S25FL128L -V ./flashrom -p dummy:size=8388608,emulate=VARIABLE_SIZE \ -r /tmp/dump.rom ./flashrom -p dummy:emulate=W25Q128FV -l /tmp/rom.layout \ -i boot -w /tmp/rom.tr.img ./flashrom -p dummy:emulate=W25Q128FV --wp-list ./flashrom -p dummy:emulate=W25Q128FV,hwwp=yes \ --wp-enable \ --wp-range=0x00c00000,0x00400000 \ --wp-status $ head -c 16MiB </dev/urandom >/tmp/image.rom ./flashrom -p dummy:image=/tmp/image.rom,emulate=S25FL128L \ -c S25FL128L -E -V Change-Id: Id573bc74e3bb46b7dc42f1452fff6394d4f091bc Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Reviewed-on: https://review.coreboot.org/c/flashrom/+/66343 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M cli_classic.c 1 file changed, 409 insertions(+), 343 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index da68919..787de67 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -31,6 +31,53 @@ #include "programmer.h" #include "libflashrom.h" +enum { + OPTION_IFD = 0x0100, + OPTION_FMAP, + OPTION_FMAP_FILE, + OPTION_FLASH_CONTENTS, + OPTION_FLASH_NAME, + OPTION_FLASH_SIZE, + OPTION_WP_STATUS, + OPTION_WP_SET_RANGE, + OPTION_WP_SET_REGION, + OPTION_WP_ENABLE, + OPTION_WP_DISABLE, + OPTION_WP_LIST, + OPTION_PROGRESS, +}; + +struct cli_options { + bool read_it, extract_it, write_it, erase_it, verify_it; + bool dont_verify_it, dont_verify_all; + bool list_supported; +#if CONFIG_PRINT_WIKI == 1 + bool list_supported_wiki; +#endif + char *filename; + + const struct programmer_entry *prog; + char *pparam; + + bool ifd, fmap; + struct flashrom_layout *layout; + struct layout_include_args *include_args; + char *layoutfile; + char *fmapfile; + + unsigned int wp_start, wp_len; + bool enable_wp, disable_wp, print_wp_status; + bool set_wp_range, set_wp_region, print_wp_ranges; + char *wp_region; + + bool force; + bool flash_name, flash_size; + bool show_progress; + char *logfile; + char *referencefile; + const char *chip_to_probe; +}; + static void cli_classic_usage(const char *name) { printf("Usage: %s [-h|-R|-L|" @@ -565,47 +612,260 @@ return limitexceeded; } +static void parse_options(int argc, char **argv, const char *optstring, + const struct option *long_options, + struct cli_options *options) +{ + const char *name; + int namelen, opt; + int option_index = 0, operation_specified = 0; + + /* FIXME: Delay all operation_specified checks until after command + * line parsing to allow --help overriding everything else. + */ + while ((opt = getopt_long(argc, argv, optstring, + long_options, &option_index)) != EOF) { + switch (opt) { + case 'r': + cli_classic_validate_singleop(&operation_specified); + options->filename = strdup(optarg); + options->read_it = true; + break; + case 'w': + cli_classic_validate_singleop(&operation_specified); + options->filename = strdup(optarg); + options->write_it = true; + break; + case 'v': + //FIXME: gracefully handle superfluous -v + cli_classic_validate_singleop(&operation_specified); + if (options->dont_verify_it) { + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); + } + options->filename = strdup(optarg); + options->verify_it = true; + break; + case 'n': + if (options->verify_it) { + cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); + } + options->dont_verify_it = true; + break; + case 'N': + options->dont_verify_all = true; + break; + case 'x': + cli_classic_validate_singleop(&operation_specified); + options->extract_it = true; + break; + case 'c': + options->chip_to_probe = strdup(optarg); + break; + case 'V': + verbose_screen++; + if (verbose_screen > FLASHROM_MSG_DEBUG2) + verbose_logfile = verbose_screen; + break; + case 'E': + cli_classic_validate_singleop(&operation_specified); + options->erase_it = true; + break; + case 'f': + options->force = true; + break; + case 'l': + if (options->layoutfile) + cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); + if (options->ifd) + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); + if (options->fmap) + cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); + options->layoutfile = strdup(optarg); + break; + case OPTION_IFD: + if (options->layoutfile) + cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); + if (options->fmap) + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); + options->ifd = true; + break; + case OPTION_FMAP_FILE: + if (options->fmap) + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " + "more than once. Aborting.\n"); + if (options->ifd) + cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); + if (options->layoutfile) + cli_classic_abort_usage("Error: --fmap-file and --layout both specified. Aborting.\n"); + options->fmapfile = strdup(optarg); + options->fmap = true; + break; + case OPTION_FMAP: + if (options->fmap) + cli_classic_abort_usage("Error: --fmap or --fmap-file specified " + "more than once. Aborting.\n"); + if (options->ifd) + cli_classic_abort_usage("Error: --fmap and --ifd both specified. Aborting.\n"); + if (options->layoutfile) + cli_classic_abort_usage("Error: --layout and --fmap both specified. Aborting.\n"); + options->fmap = true; + break; + case 'i': + if (register_include_arg(&options->include_args, optarg)) + cli_classic_abort_usage(NULL); + break; + case OPTION_FLASH_CONTENTS: + if (options->referencefile) + cli_classic_abort_usage("Error: --flash-contents specified more than once." + "Aborting.\n"); + options->referencefile = strdup(optarg); + break; + case OPTION_FLASH_NAME: + cli_classic_validate_singleop(&operation_specified); + options->flash_name = true; + break; + case OPTION_FLASH_SIZE: + cli_classic_validate_singleop(&operation_specified); + options->flash_size = true; + break; + case OPTION_WP_STATUS: + options->print_wp_status = true; + break; + case OPTION_WP_LIST: + options->print_wp_ranges = true; + break; + case OPTION_WP_SET_RANGE: + if (parse_wp_range(&options->wp_start, &options->wp_len) < 0) + cli_classic_abort_usage("Incorrect wp-range arguments provided.\n"); + + options->set_wp_range = true; + break; + case OPTION_WP_SET_REGION: + options->set_wp_region = true; + options->wp_region = strdup(optarg); + break; + case OPTION_WP_ENABLE: + options->enable_wp = true; + break; + case OPTION_WP_DISABLE: + options->disable_wp = true; + break; + case 'L': + cli_classic_validate_singleop(&operation_specified); + options->list_supported = true; + break; + case 'z': +#if CONFIG_PRINT_WIKI == 1 + cli_classic_validate_singleop(&operation_specified); + options->list_supported_wiki = true; +#else + cli_classic_abort_usage("Error: Wiki output was not " + "compiled in. Aborting.\n"); +#endif + break; + case 'p': + if (options->prog != NULL) { + cli_classic_abort_usage("Error: --programmer specified " + "more than once. You can separate " + "multiple\nparameters for a programmer " + "with \",\". Please see the man page " + "for details.\n"); + } + size_t p; + for (p = 0; p < programmer_table_size; p++) { + name = programmer_table[p]->name; + namelen = strlen(name); + if (strncmp(optarg, name, namelen) == 0) { + switch (optarg[namelen]) { + case ':': + options->pparam = strdup(optarg + namelen + 1); + if (!strlen(options->pparam)) { + free(options->pparam); + options->pparam = NULL; + } + options->prog = programmer_table[p]; + break; + case '\0': + options->prog = programmer_table[p]; + break; + default: + /* The continue refers to the + * for loop. It is here to be + * able to differentiate between + * foo and foobar. + */ + continue; + } + break; + } + } + if (options->prog == NULL) { + fprintf(stderr, "Error: Unknown programmer \"%s\". Valid choices are:\n", + optarg); + list_programmers_linebreak(0, 80, 0); + msg_ginfo(".\n"); + cli_classic_abort_usage(NULL); + } + break; + case 'R': + /* print_version() is always called during startup. */ + cli_classic_validate_singleop(&operation_specified); + exit(0); + break; + case 'h': + cli_classic_validate_singleop(&operation_specified); + cli_classic_usage(argv[0]); + exit(0); + break; + case 'o': + if (options->logfile) { + fprintf(stderr, "Warning: -o/--output specified multiple times.\n"); + free(options->logfile); + } + + options->logfile = strdup(optarg); + if (options->logfile[0] == '\0') { + cli_classic_abort_usage("No log filename specified.\n"); + } + break; + case OPTION_PROGRESS: + options->show_progress = true; + break; + default: + cli_classic_abort_usage(NULL); + break; + } + } + + if (optind < argc) + cli_classic_abort_usage("Error: Extra parameter found.\n"); +} + +static void free_options(struct cli_options *options) +{ + cleanup_include_args(&options->include_args); + free(options->filename); + free(options->fmapfile); + free(options->referencefile); + free(options->layoutfile); + free(options->pparam); + free(options->wp_region); + free(options->logfile); + free((char *)options->chip_to_probe); +} + int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; /* Probe for up to eight flash chips. */ struct flashctx flashes[8] = {{0}}; struct flashctx *fill_flash; - const char *name; - int namelen, opt, i, j; - int startchip = -1, chipcount = 0, option_index = 0; - int operation_specified = 0; - unsigned int wp_start = 0, wp_len = 0; - bool force = false, ifd = false, fmap = false; -#if CONFIG_PRINT_WIKI == 1 - bool list_supported_wiki = false; -#endif - bool flash_name = false, flash_size = false; - bool enable_wp = false, disable_wp = false, print_wp_status = false; - bool set_wp_range = false, set_wp_region = false, print_wp_ranges = false; - bool read_it = false, extract_it = false, write_it = false, erase_it = false, verify_it = false; - bool dont_verify_it = false, dont_verify_all = false; - bool list_supported = false; - bool show_progress = false; - struct flashrom_layout *layout = NULL; - static const struct programmer_entry *prog = NULL; - enum { - OPTION_IFD = 0x0100, - OPTION_FMAP, - OPTION_FMAP_FILE, - OPTION_FLASH_CONTENTS, - OPTION_FLASH_NAME, - OPTION_FLASH_SIZE, - OPTION_WP_STATUS, - OPTION_WP_SET_RANGE, - OPTION_WP_SET_REGION, - OPTION_WP_ENABLE, - OPTION_WP_DISABLE, - OPTION_WP_LIST, - OPTION_PROGRESS, - }; + char *tempstr = NULL; + int startchip = -1, chipcount = 0; + int i, j; int ret = 0; + struct cli_options options = { 0 }; static const char optstring[] = "r:Rw:v:nNVEfc:l:i:p:Lzho:x"; static const struct option long_options[] = { {"read", 1, NULL, 'r'}, @@ -644,17 +904,6 @@ {NULL, 0, NULL, 0}, }; - char *filename = NULL; - char *referencefile = NULL; - char *layoutfile = NULL; - char *fmapfile = NULL; - char *logfile = NULL; - char *tempstr = NULL; - char *pparam = NULL; - struct layout_include_args *include_args = NULL; - char *wp_region = NULL; - const char *chip_to_probe = NULL; - /* * Safety-guard against a user who has (mistakenly) closed * stdout or stderr before exec'ing flashrom. We disable @@ -673,246 +922,30 @@ exit(1); setbuf(stdout, NULL); - /* FIXME: Delay all operation_specified checks until after command - * line parsing to allow --help overriding everything else. - */ - while ((opt = getopt_long(argc, argv, optstring, - long_options, &option_index)) != EOF) { - switch (opt) { - case 'r': - cli_classic_validate_singleop(&operation_specified); - filename = strdup(optarg); - read_it = true; - break; - case 'w': - cli_classic_validate_singleop(&operation_specified); - filename = strdup(optarg); - write_it = true; - break; - case 'v': - //FIXME: gracefully handle superfluous -v - cli_classic_validate_singleop(&operation_specified); - if (dont_verify_it) { - cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); - } - filename = strdup(optarg); - verify_it = true; - break; - case 'n': - if (verify_it) { - cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); - } - dont_verify_it = true; - break; - case 'N': - dont_verify_all = true; - break; - case 'x': - cli_classic_validate_singleop(&operation_specified); - extract_it = true; - break; - case 'c': - chip_to_probe = strdup(optarg); - break; - case 'V': - verbose_screen++; - if (verbose_screen > FLASHROM_MSG_DEBUG2) - verbose_logfile = verbose_screen; - break; - case 'E': - cli_classic_validate_singleop(&operation_specified); - erase_it = true; - break; - case 'f': - force = true; - break; - case 'l': - if (layoutfile) - cli_classic_abort_usage("Error: --layout specified more than once. Aborting.\n"); - if (ifd) - cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); - if (fmap) - cli_classic_abort_usage("Error: --layout and --fmap-file both specified. Aborting.\n"); - layoutfile = strdup(optarg); - break; - case OPTION_IFD: - if (layoutfile) - cli_classic_abort_usage("Error: --layout and --ifd both specified. Aborting.\n"); - if (fmap) - cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); - ifd = true; - break; - case OPTION_FMAP_FILE: - if (fmap) - cli_classic_abort_usage("Error: --fmap or --fmap-file specified " - "more than once. Aborting.\n"); - if (ifd) - cli_classic_abort_usage("Error: --fmap-file and --ifd both specified. Aborting.\n"); - if (layoutfile) - cli_classic_abort_usage("Error: --fmap-file and --layout both specified. Aborting.\n"); - fmapfile = strdup(optarg); - fmap = true; - break; - case OPTION_FMAP: - if (fmap) - cli_classic_abort_usage("Error: --fmap or --fmap-file specified " - "more than once. Aborting.\n"); - if (ifd) - cli_classic_abort_usage("Error: --fmap and --ifd both specified. Aborting.\n"); - if (layoutfile) - cli_classic_abort_usage("Error: --layout and --fmap both specified. Aborting.\n"); - fmap = true; - break; - case 'i': - if (register_include_arg(&include_args, optarg)) - cli_classic_abort_usage(NULL); - break; - case OPTION_FLASH_CONTENTS: - if (referencefile) - cli_classic_abort_usage("Error: --flash-contents specified more than once." - "Aborting.\n"); - referencefile = strdup(optarg); - break; - case OPTION_FLASH_NAME: - cli_classic_validate_singleop(&operation_specified); - flash_name = true; - break; - case OPTION_FLASH_SIZE: - cli_classic_validate_singleop(&operation_specified); - flash_size = true; - break; - case OPTION_WP_STATUS: - print_wp_status = true; - break; - case OPTION_WP_LIST: - print_wp_ranges = true; - break; - case OPTION_WP_SET_RANGE: - if (parse_wp_range(&wp_start, &wp_len) < 0) - cli_classic_abort_usage("Incorrect wp-range arguments provided.\n"); - set_wp_range = true; - break; - case OPTION_WP_SET_REGION: - set_wp_region = true; - wp_region = strdup(optarg); - break; - case OPTION_WP_ENABLE: - enable_wp = true; - break; - case OPTION_WP_DISABLE: - disable_wp = true; - break; - case 'L': - cli_classic_validate_singleop(&operation_specified); - list_supported = true; - break; - case 'z': -#if CONFIG_PRINT_WIKI == 1 - cli_classic_validate_singleop(&operation_specified); - list_supported_wiki = true; -#else - cli_classic_abort_usage("Error: Wiki output was not " - "compiled in. Aborting.\n"); -#endif - break; - case 'p': - if (prog != NULL) { - cli_classic_abort_usage("Error: --programmer specified " - "more than once. You can separate " - "multiple\nparameters for a programmer " - "with \",\". Please see the man page " - "for details.\n"); - } - size_t p; - for (p = 0; p < programmer_table_size; p++) { - name = programmer_table[p]->name; - namelen = strlen(name); - if (strncmp(optarg, name, namelen) == 0) { - switch (optarg[namelen]) { - case ':': - pparam = strdup(optarg + namelen + 1); - if (!strlen(pparam)) { - free(pparam); - pparam = NULL; - } - prog = programmer_table[p]; - break; - case '\0': - prog = programmer_table[p]; - break; - default: - /* The continue refers to the - * for loop. It is here to be - * able to differentiate between - * foo and foobar. - */ - continue; - } - break; - } - } - if (prog == NULL) { - fprintf(stderr, "Error: Unknown programmer \"%s\". Valid choices are:\n", - optarg); - list_programmers_linebreak(0, 80, 0); - msg_ginfo(".\n"); - cli_classic_abort_usage(NULL); - } - break; - case 'R': - /* print_version() is always called during startup. */ - cli_classic_validate_singleop(&operation_specified); - exit(0); - break; - case 'h': - cli_classic_validate_singleop(&operation_specified); - cli_classic_usage(argv[0]); - exit(0); - break; - case 'o': - if (logfile) { - fprintf(stderr, "Warning: -o/--output specified multiple times.\n"); - free(logfile); - } + parse_options(argc, argv, optstring, long_options, &options); - logfile = strdup(optarg); - if (logfile[0] == '\0') { - cli_classic_abort_usage("No log filename specified.\n"); - } - break; - case OPTION_PROGRESS: - show_progress = true; - break; - default: - cli_classic_abort_usage(NULL); - break; - } - } - - if (optind < argc) - cli_classic_abort_usage("Error: Extra parameter found.\n"); - if ((read_it | write_it | verify_it) && check_filename(filename, "image")) + if ((options.read_it | options.write_it | options.verify_it) && check_filename(options.filename, "image")) cli_classic_abort_usage(NULL); - if (layoutfile && check_filename(layoutfile, "layout")) + if (options.layoutfile && check_filename(options.layoutfile, "layout")) cli_classic_abort_usage(NULL); - if (fmapfile && check_filename(fmapfile, "fmap")) + if (options.fmapfile && check_filename(options.fmapfile, "fmap")) cli_classic_abort_usage(NULL); - if (referencefile && check_filename(referencefile, "reference")) + if (options.referencefile && check_filename(options.referencefile, "reference")) cli_classic_abort_usage(NULL); - if (logfile && check_filename(logfile, "log")) + if (options.logfile && check_filename(options.logfile, "log")) cli_classic_abort_usage(NULL); - if (logfile && open_logfile(logfile)) + if (options.logfile && open_logfile(options.logfile)) cli_classic_abort_usage(NULL); #if CONFIG_PRINT_WIKI == 1 - if (list_supported_wiki) { + if (options.list_supported_wiki) { print_supported_wiki(); goto out; } #endif - if (list_supported) { + if (options.list_supported) { if (print_supported()) ret = 1; goto out; @@ -927,22 +960,22 @@ } msg_gdbg("\n"); - if (layoutfile && layout_from_file(&layout, layoutfile)) { + if (options.layoutfile && layout_from_file(&options.layout, options.layoutfile)) { ret = 1; goto out; } - if (!ifd && !fmap && process_include_args(layout, include_args)) { + if (!options.ifd && !options.fmap && process_include_args(options.layout, options.include_args)) { ret = 1; goto out; } /* Does a chip with the requested name exist in the flashchips array? */ - if (chip_to_probe) { + if (options.chip_to_probe) { for (chip = flashchips; chip && chip->name; chip++) - if (!strcmp(chip->name, chip_to_probe)) + if (!strcmp(chip->name, options.chip_to_probe)) break; if (!chip || !chip->name) { - msg_cerr("Error: Unknown chip '%s' specified.\n", chip_to_probe); + msg_cerr("Error: Unknown chip '%s' specified.\n", options.chip_to_probe); msg_gerr("Run flashrom -L to view the hardware supported in this flashrom version.\n"); ret = 1; goto out; @@ -950,15 +983,15 @@ /* Keep chip around for later usage in case a forced read is requested. */ } - if (prog == NULL) { + if (options.prog == NULL) { const struct programmer_entry *const default_programmer = CONFIG_DEFAULT_PROGRAMMER_NAME; if (default_programmer) { - prog = default_programmer; + options.prog = default_programmer; /* We need to strdup here because we free(pparam) unconditionally later. */ - pparam = strdup(CONFIG_DEFAULT_PROGRAMMER_ARGS); + options.pparam = strdup(CONFIG_DEFAULT_PROGRAMMER_ARGS); msg_pinfo("Using default programmer \"%s\" with arguments \"%s\".\n", - default_programmer->name, pparam); + default_programmer->name, options.pparam); } else { msg_perr("Please select a programmer with the --programmer parameter.\n" #if CONFIG_INTERNAL == 1 @@ -972,7 +1005,7 @@ } } - if (programmer_init(prog, pparam)) { + if (programmer_init(options.prog, options.pparam)) { msg_perr("Error: Programmer initialization failed.\n"); ret = 1; goto out_shutdown; @@ -984,7 +1017,7 @@ for (j = 0; j < registered_master_count; j++) { startchip = 0; while (chipcount < (int)ARRAY_SIZE(flashes)) { - startchip = probe_flash(&registered_masters[j], startchip, &flashes[chipcount], 0, chip_to_probe); + startchip = probe_flash(&registered_masters[j], startchip, &flashes[chipcount], 0, options.chip_to_probe); if (startchip == -1) break; chipcount++; @@ -1002,11 +1035,11 @@ goto out_shutdown; } else if (!chipcount) { msg_cinfo("No EEPROM/flash device found.\n"); - if (!force || !chip_to_probe) { + if (!options.force || !options.chip_to_probe) { msg_cinfo("Note: flashrom can never write if the flash chip isn't found " "automatically.\n"); } - if (force && read_it && chip_to_probe) { + if (options.force && options.read_it && options.chip_to_probe) { struct registered_master *mst; int compatible_masters = 0; msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n"); @@ -1027,25 +1060,25 @@ "chip, using the first one.\n"); for (j = 0; j < registered_master_count; j++) { mst = &registered_masters[j]; - startchip = probe_flash(mst, 0, &flashes[0], 1, chip_to_probe); + startchip = probe_flash(mst, 0, &flashes[0], 1, options.chip_to_probe); if (startchip != -1) break; } if (startchip == -1) { // FIXME: This should never happen! Ask for a bug report? - msg_cinfo("Probing for flash chip '%s' failed.\n", chip_to_probe); + msg_cinfo("Probing for flash chip '%s' failed.\n", options.chip_to_probe); ret = 1; goto out_shutdown; } msg_cinfo("Please note that forced reads most likely contain garbage.\n"); - flashrom_flag_set(&flashes[0], FLASHROM_FLAG_FORCE, force); - ret = do_read(&flashes[0], filename); + flashrom_flag_set(&flashes[0], FLASHROM_FLAG_FORCE, options.force); + ret = do_read(&flashes[0], options.filename); free(flashes[0].chip); goto out_shutdown; } ret = 1; goto out_shutdown; - } else if (!chip_to_probe) { + } else if (!options.chip_to_probe) { /* repeat for convenience when looking at foreign logs */ tempstr = flashbuses_to_text(flashes[0].chip->bustype); msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n", @@ -1059,13 +1092,13 @@ struct flashrom_progress progress_state = { .user_data = progress_user_data }; - if (show_progress) + if (options.show_progress) flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state); print_chip_support_status(fill_flash->chip); unsigned int limitexceeded = count_max_decode_exceedings(fill_flash, &max_rom_decode); - if (limitexceeded > 0 && !force) { + if (limitexceeded > 0 && !options.force) { enum chipbustype commonbuses = fill_flash->mst->buses_supported & fill_flash->chip->bustype; /* Sometimes chip and programmer have more than one bus in common, @@ -1081,29 +1114,30 @@ } const bool any_wp_op = - set_wp_range || set_wp_region || enable_wp || - disable_wp || print_wp_status || print_wp_ranges; + options.set_wp_range || options.set_wp_region || options.enable_wp || + options.disable_wp || options.print_wp_status || options.print_wp_ranges; - const bool any_op = read_it || write_it || verify_it || erase_it || - flash_name || flash_size || extract_it || any_wp_op; + const bool any_op = options.read_it || options.write_it || options.verify_it || + options.erase_it || options.flash_name || options.flash_size || + options.extract_it || any_wp_op; if (!any_op) { msg_ginfo("No operations were specified.\n"); goto out_shutdown; } - if (enable_wp && disable_wp) { + if (options.enable_wp && options.disable_wp) { msg_ginfo("Error: --wp-enable and --wp-disable are mutually exclusive\n"); ret = 1; goto out_shutdown; } - if (set_wp_range && set_wp_region) { + if (options.set_wp_range && options.set_wp_region) { msg_gerr("Error: Cannot use both --wp-range and --wp-region simultaneously.\n"); ret = 1; goto out_shutdown; } - if (flash_name) { + if (options.flash_name) { if (fill_flash->chip->vendor && fill_flash->chip->name) { printf("vendor=\"%s\" name=\"%s\"\n", fill_flash->chip->vendor, @@ -1114,19 +1148,19 @@ goto out_shutdown; } - if (flash_size) { + if (options.flash_size) { printf("%zu\n", flashrom_flash_getsize(fill_flash)); goto out_shutdown; } - if (ifd && (flashrom_layout_read_from_ifd(&layout, fill_flash, NULL, 0) || - process_include_args(layout, include_args))) { + if (options.ifd && (flashrom_layout_read_from_ifd(&options.layout, fill_flash, NULL, 0) || + process_include_args(options.layout, options.include_args))) { ret = 1; goto out_shutdown; - } else if (fmap && fmapfile) { + } else if (options.fmap && options.fmapfile) { struct stat s; - if (stat(fmapfile, &s) != 0) { - msg_gerr("Failed to stat fmapfile \"%s\"\n", fmapfile); + if (stat(options.fmapfile, &s) != 0) { + msg_gerr("Failed to stat fmapfile \"%s\"\n", options.fmapfile); ret = 1; goto out_shutdown; } @@ -1138,72 +1172,73 @@ goto out_shutdown; } - if (read_buf_from_file(fmapfile_buffer, fmapfile_size, fmapfile)) { + if (read_buf_from_file(fmapfile_buffer, fmapfile_size, options.fmapfile)) { ret = 1; free(fmapfile_buffer); goto out_shutdown; } - if (flashrom_layout_read_fmap_from_buffer(&layout, fill_flash, fmapfile_buffer, fmapfile_size) || - process_include_args(layout, include_args)) { + if (flashrom_layout_read_fmap_from_buffer(&options.layout, fill_flash, fmapfile_buffer, fmapfile_size) || + process_include_args(options.layout, options.include_args)) { ret = 1; free(fmapfile_buffer); goto out_shutdown; } free(fmapfile_buffer); - } else if (fmap && (flashrom_layout_read_fmap_from_rom(&layout, fill_flash, 0, - flashrom_flash_getsize(fill_flash)) || process_include_args(layout, include_args))) { + } else if (options.fmap && (flashrom_layout_read_fmap_from_rom(&options.layout, fill_flash, 0, + flashrom_flash_getsize(fill_flash)) || + process_include_args(options.layout, options.include_args))) { ret = 1; goto out_shutdown; } - flashrom_layout_set(fill_flash, layout); + flashrom_layout_set(fill_flash, options.layout); if (any_wp_op) { - if (set_wp_region && wp_region) { - if (!layout) { + if (options.set_wp_region && options.wp_region) { + if (!options.layout) { msg_gerr("Error: A flash layout must be specified to use --wp-region.\n"); ret = 1; goto out_release; } - ret = flashrom_layout_get_region_range(layout, wp_region, &wp_start, &wp_len); + ret = flashrom_layout_get_region_range(options.layout, options.wp_region, &options.wp_start, &options.wp_len); if (ret) { - msg_gerr("Error: Region %s not found in flash layout.\n", wp_region); + msg_gerr("Error: Region %s not found in flash layout.\n", options.wp_region); goto out_release; } - set_wp_range = true; + options.set_wp_range = true; } ret = wp_cli( fill_flash, - enable_wp, - disable_wp, - print_wp_status, - print_wp_ranges, - set_wp_range, - wp_start, - wp_len + options.enable_wp, + options.disable_wp, + options.print_wp_status, + options.print_wp_ranges, + options.set_wp_range, + options.wp_start, + options.wp_len ); if (ret) goto out_release; } - flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, force); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE, options.force); #if CONFIG_INTERNAL == 1 flashrom_flag_set(fill_flash, FLASHROM_FLAG_FORCE_BOARDMISMATCH, force_boardmismatch); #endif - flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); - flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !options.dont_verify_it); + flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !options.dont_verify_all); /* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. */ programmer_delay(fill_flash, 100000); - if (read_it) - ret = do_read(fill_flash, filename); - else if (extract_it) + if (options.read_it) + ret = do_read(fill_flash, options.filename); + else if (options.extract_it) ret = do_extract(fill_flash); - else if (erase_it) { + else if (options.erase_it) { ret = flashrom_flash_erase(fill_flash); /* * FIXME: Do we really want the scary warning if erase failed? @@ -1215,13 +1250,13 @@ if (ret) emergency_help_message(); } - else if (write_it) - ret = do_write(fill_flash, filename, referencefile); - else if (verify_it) - ret = do_verify(fill_flash, filename); + else if (options.write_it) + ret = do_write(fill_flash, options.filename, options.referencefile); + else if (options.verify_it) + ret = do_verify(fill_flash, options.filename); out_release: - flashrom_layout_release(layout); + flashrom_layout_release(options.layout); out_shutdown: flashrom_programmer_shutdown(NULL); out: @@ -1230,17 +1265,7 @@ free(flashes[i].chip); } - cleanup_include_args(&include_args); - free(filename); - free(fmapfile); - free(referencefile); - free(layoutfile); - free(pparam); - free(wp_region); - /* clean up global variables */ - free((char *)chip_to_probe); /* Silence! Freeing is not modifying contents. */ - chip_to_probe = NULL; - free(logfile); + free_options(&options); ret |= close_logfile(); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/66343 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id573bc74e3bb46b7dc42f1452fff6394d4f091bc Gerrit-Change-Number: 66343 Gerrit-PatchSet: 9 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: flashrom: rewrite flashbuses_to_text()
by Anastasia Klimchuk (Code Review) 13 Apr '23

13 Apr '23

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/73039 ) Change subject: flashrom: rewrite flashbuses_to_text() ...................................................................... Patch Set 6: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/73039/comment/26e3b064_bafb86ca PS6, Line 17: `printf()` I thought that's undefined behaviour, handling null in printf? File flashrom.c: https://review.coreboot.org/c/flashrom/+/73039/comment/56ab58f8_e8a21938 PS5, Line 948: if (bustype & (1 << i)) > Came back with fresh ideas, I see that it's possible to use a struct: […] Looks good! I was a bit confused where is the code, but realised it's only in the comment for now, not in the latest patchset yet :) -- To view, visit https://review.coreboot.org/c/flashrom/+/73039 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c Gerrit-Change-Number: 73039 Gerrit-PatchSet: 6 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 13 Apr 2023 09:05:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: programmers: use vendor ID macro
by Alexander Goncharov (Code Review) 12 Apr '23

12 Apr '23

Alexander Goncharov has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/66859 ) Change subject: programmers: use vendor ID macro ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/66859 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifd8edc7c79cf46ae8acb0abd03764687a4a2bc8c Gerrit-Change-Number: 66859 Gerrit-PatchSet: 5 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> 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: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit