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

Change in flashrom[master]: libflashrom: clear layout on flashrom_layout_release
by Daniel Campello (Code Review) 12 May '21

12 May '21

Attention is currently required from: Peter Marheine. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54147 ) Change subject: libflashrom: clear layout on flashrom_layout_release ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/54147/comment/71778377_b2b461b6 PS2, Line 10: (as is currently always : the case) Not always the case... See --layout --fmap --ifd https://review.coreboot.org/c/flashrom/+/54147/comment/6353da3c_4032ee8f PS2, Line 14: it's always the global layout how can user do this on libflashrom? I think the global layout is only used by cli_classic -- To view, visit https://review.coreboot.org/c/flashrom/+/54147 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Gerrit-Change-Number: 54147 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 02:35:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: clear layout on flashrom_layout_release
by Daniel Campello (Code Review) 12 May '21

12 May '21

Attention is currently required from: Peter Marheine. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54147 ) Change subject: libflashrom: clear layout on flashrom_layout_release ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54147 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Gerrit-Change-Number: 54147 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 02:32:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer: Smoothen register_spi_master() API
by Anastasia Klimchuk (Code Review) 12 May '21

12 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54066 ) Change subject: programmer: Smoothen register_spi_master() API ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54066 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c753b3db050fb87d4bbe2301a7ead854f28456f Gerrit-Change-Number: 54066 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 02:31:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: clear layout on flashrom_layout_release
by Anastasia Klimchuk (Code Review) 12 May '21

12 May '21

Attention is currently required from: Daniel Campello, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54147 ) Change subject: libflashrom: clear layout on flashrom_layout_release ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54147 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Gerrit-Change-Number: 54147 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 01:57:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: stlinkv3_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 12 May '21

12 May '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54042 ) Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: (1 comment) File stlinkv3_spi.c: https://review.coreboot.org/c/flashrom/+/54042/comment/616cea9f_5f4625a3 PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle > It's just the one more `goto err_exit` that you added (in case calloc […] Not to derail but Nico no one is "And blaming the existing code for it seems odd." please don't jump to conclusions. Thanks for noticing that path though the shutdown fn, I was thinking of a different that is all. -- To view, visit https://review.coreboot.org/c/flashrom/+/54042 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id044661b864b506028720ea809bc524f0640469f Gerrit-Change-Number: 54042 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 01:42:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in ...flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
by Nico Huber (Code Review) 12 May '21

12 May '21

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/30979 Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ...................................................................... [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter In case the user only wants to specify files for specific layout regions, we need a way to set the operation without specifying a file. Instead of making the <filename> argument optional, we can add a new syntax for this particular purpose. This way, we avoid complex command line parsing and can do more sanity checks and provide better error messages. TODO: Update manpage in case this gets accepted. Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M cli_classic.c 1 file changed, 42 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/30979/1 diff --git a/cli_classic.c b/cli_classic.c index 324e145..d09a8d1 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -39,7 +39,8 @@ "-z|" #endif "-p <programmername>[:<parameters>] [-c <chipname>]\n" - "[-E|(-r|-w|-v) <file>] [(-l <layoutfile>|--ifd) [-i <imagename>]...] [-n] [-N] [-f]]\n" + "[(-r|-w|-v|-E) <file>|-m (r|w|v|e)] [(-l <layoutfile>|--ifd) [-i <region>]...]\n" + "[-n] [-N] [-f]]\n" "[-V[V[V]]] [-o <logfile>]\n\n", name); printf(" -h | --help print this help text\n" @@ -47,6 +48,8 @@ " -r | --read <file> read flash and save to <file>\n" " -w | --write <file> write <file> to flash\n" " -v | --verify <file> verify flash against <file>\n" + " -m | --mode <operation> for use with --include <region>:<file>,\n" + " specify the mode of operation (r|w|v|e)\n" " -E | --erase erase flash memory\n" " -V | --verbose more verbose output\n" " -c | --chip <chipname> probe only for specified flash chip\n" @@ -123,6 +126,7 @@ {"write", 1, NULL, 'w'}, {"erase", 0, NULL, 'E'}, {"verify", 1, NULL, 'v'}, + {"mode", 1, NULL, 'm'}, {"noverify", 0, NULL, 'n'}, {"noverify-all", 0, NULL, 'N'}, {"chip", 1, NULL, 'c'}, @@ -200,6 +204,25 @@ filename = strdup(optarg); verify_it = 1; break; + case 'm': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation specified. Aborting.\n"); + cli_classic_abort_usage(); + } + /* check that `optarg` is a prefix of a known operation */ + if (strncmp(optarg, "read", strlen(optarg)) == 0) { + read_it = 1; + } else if (strncmp(optarg, "write", strlen(optarg)) == 0) { + write_it = 1; + } else if (strncmp(optarg, "verify", strlen(optarg)) == 0) { + verify_it = 1; + } else if (strncmp(optarg, "erase", strlen(optarg)) == 0) { + erase_it = 1; + } else { + fprintf(stderr, "Unknown operation specified: '%s'\n", optarg); + cli_classic_abort_usage(); + } + break; case 'n': if (verify_it) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); @@ -404,7 +427,7 @@ cli_classic_abort_usage(); } - if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if (filename && check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -654,6 +677,23 @@ 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); + if (!filename && (read_it | write_it | verify_it)) { + if (layout->num_entries == 0) { + msg_gerr("Error: The specified operation requires a file.\n"); + ret = 1; + goto out_shutdown; + } + for (i = 0; i < layout->num_entries; i++) { + if (!layout->entries[i].file) { + msg_gerr("Error: Region \"%s\" requires a file argument.\n", + layout->entries[i].name); + ret = 1; + } + if (ret) + goto out_shutdown; + } + } + /* 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. -- To view, visit https://review.coreboot.org/c/flashrom/+/30979 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b Gerrit-Change-Number: 30979 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: newchange

5 18

Change in flashrom[master]: libflashrom: clear layout on flashrom_layout_release
by Peter Marheine (Code Review) 12 May '21

12 May '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54147 ) Change subject: libflashrom: clear layout on flashrom_layout_release ...................................................................... Patch Set 2: (2 comments) File layout.c: https://review.coreboot.org/c/flashrom/+/54147/comment/3924dcb1_2f9d5926 PS1, Line 296: layout->entries[i].included = false; > Should this line also move into flashrom_layout_release() ? Even better, zero the whole entry. https://review.coreboot.org/c/flashrom/+/54147/comment/df8a188a_bab782e5 PS1, Line 279: layout_cleanup_args > This function is not cleaning up the layout anymore, only args. […] It's still layout args specifically so I'd like to keep 'layout', but layout_args_cleanup seems a little better. -- To view, visit https://review.coreboot.org/c/flashrom/+/54147 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Gerrit-Change-Number: 54147 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 13 May 2021 01:06:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: clear layout on flashrom_layout_release
by Peter Marheine (Code Review) 12 May '21

12 May '21

Attention is currently required from: Edward O'Callaghan, Peter Marheine. Hello build bot (Jenkins), Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/54147 to look at the new patch set (#2). Change subject: libflashrom: clear layout on flashrom_layout_release ...................................................................... libflashrom: clear layout on flashrom_layout_release This changes the behavior of flashrom_layout_release to always clear the provided layout, even if it is the global layout (as is currently always the case). Not clearing it causes confusing behavior where a user may release a layout and create a new one, but because it's always the global layout which wasn't cleared there will be stale information in the returned layout. This can easily lead to writing unexpected flash regions because the API provides no way to stop including regions, and makes it effectively impossible to stop including a region without completely tearing down the library. Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M cli_classic.c M flash.h M layout.c 3 files changed, 8 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/54147/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54147 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8ba4b356f2a40ccbfd4e91f81af01d5f6a6de515 Gerrit-Change-Number: 54147 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: linux_mtd: drop 'mtd_' prefix from variable/field names
by Edward O'Callaghan (Code Review) 12 May '21

12 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/53953 ) Change subject: linux_mtd: drop 'mtd_' prefix from variable/field names ...................................................................... linux_mtd: drop 'mtd_' prefix from variable/field names BUG=b:161951062 BRANCH=none TEST=builds Change-Id: I2503c98e9111d1fecd911473f65eeea7031cfdc3 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/53953 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M linux_mtd.c 1 file changed, 27 insertions(+), 27 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/linux_mtd.c b/linux_mtd.c index 7cdba6c..0f7ac7e 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -33,13 +33,13 @@ struct linux_mtd_data { FILE *dev_fp; - int mtd_device_is_writeable; - int mtd_no_erase; + int device_is_writeable; + int no_erase; /* Size info is presented in bytes in sysfs. */ - unsigned long int mtd_total_size; - unsigned long int mtd_numeraseregions; + unsigned long int total_size; + unsigned long int numeraseregions; /* only valid if numeraseregions is 0 */ - unsigned long int mtd_erasesize; + unsigned long int erasesize; }; /* read a string from a sysfs file and sanitize it */ @@ -123,51 +123,51 @@ static int get_mtd_info(const char *sysfs_path, struct linux_mtd_data *data) { unsigned long int tmp; - char mtd_device_name[32]; + char device_name[32]; /* Flags */ if (read_sysfs_int(sysfs_path, "flags", &tmp)) return 1; if (tmp & MTD_WRITEABLE) { /* cache for later use by write function */ - data->mtd_device_is_writeable = 1; + data->device_is_writeable = 1; } if (tmp & MTD_NO_ERASE) { - data->mtd_no_erase = 1; + data->no_erase = 1; } /* Device name */ - if (read_sysfs_string(sysfs_path, "name", mtd_device_name, sizeof(mtd_device_name))) + if (read_sysfs_string(sysfs_path, "name", device_name, sizeof(device_name))) return 1; /* Total size */ - if (read_sysfs_int(sysfs_path, "size", &data->mtd_total_size)) + if (read_sysfs_int(sysfs_path, "size", &data->total_size)) return 1; - if (popcnt(data->mtd_total_size) != 1) { + if (popcnt(data->total_size) != 1) { msg_perr("MTD size is not a power of 2\n"); return 1; } /* Erase size */ - if (read_sysfs_int(sysfs_path, "erasesize", &data->mtd_erasesize)) + if (read_sysfs_int(sysfs_path, "erasesize", &data->erasesize)) return 1; - if (popcnt(data->mtd_erasesize) != 1) { + if (popcnt(data->erasesize) != 1) { msg_perr("MTD erase size is not a power of 2\n"); return 1; } /* Erase regions */ - if (read_sysfs_int(sysfs_path, "numeraseregions", &data->mtd_numeraseregions)) + if (read_sysfs_int(sysfs_path, "numeraseregions", &data->numeraseregions)) return 1; - if (data->mtd_numeraseregions != 0) { + if (data->numeraseregions != 0) { msg_perr("Non-uniform eraseblock size is unsupported.\n"); return 1; } msg_pdbg("%s: device_name: \"%s\", is_writeable: %d, " "numeraseregions: %lu, total_size: %lu, erasesize: %lu\n", - __func__, mtd_device_name, data->mtd_device_is_writeable, - data->mtd_numeraseregions, data->mtd_total_size, data->mtd_erasesize); + __func__, device_name, data->device_is_writeable, + data->numeraseregions, data->total_size, data->erasesize); return 0; } @@ -176,13 +176,13 @@ { struct linux_mtd_data *data = flash->mst->opaque.data; - if (data->mtd_no_erase) + if (data->no_erase) flash->chip->feature_bits |= FEATURE_NO_ERASE; flash->chip->tested = TEST_OK_PREW; - flash->chip->total_size = data->mtd_total_size / 1024; /* bytes -> kB */ - flash->chip->block_erasers[0].eraseblocks[0].size = data->mtd_erasesize; + flash->chip->total_size = data->total_size / 1024; /* bytes -> kB */ + flash->chip->block_erasers[0].eraseblocks[0].size = data->erasesize; flash->chip->block_erasers[0].eraseblocks[0].count = - data->mtd_total_size / data->mtd_erasesize; + data->total_size / data->erasesize; return 1; } @@ -227,7 +227,7 @@ unsigned int chunksize = flash->chip->block_erasers[0].eraseblocks[0].size; unsigned int i; - if (!data->mtd_device_is_writeable) + if (!data->device_is_writeable) return 1; if (fseek(data->dev_fp, start, SEEK_SET) != 0) { @@ -267,23 +267,23 @@ struct linux_mtd_data *data = flash->mst->opaque.data; uint32_t u; - if (data->mtd_no_erase) { + if (data->no_erase) { msg_perr("%s: device does not support erasing. Please file a " "bug report at flashrom(a)flashrom.org\n", __func__); return 1; } - if (data->mtd_numeraseregions != 0) { + if (data->numeraseregions != 0) { /* TODO: Support non-uniform eraseblock size using use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */ - msg_perr("%s: mtd_numeraseregions must be 0\n", __func__); + msg_perr("%s: numeraseregions must be 0\n", __func__); return 1; } - for (u = 0; u < len; u += data->mtd_erasesize) { + for (u = 0; u < len; u += data->erasesize) { struct erase_info_user erase_info = { .start = start + u, - .length = data->mtd_erasesize, + .length = data->erasesize, }; if (ioctl(fileno(data->dev_fp), MEMERASE, &erase_info) == -1) { -- To view, visit https://review.coreboot.org/c/flashrom/+/53953 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2503c98e9111d1fecd911473f65eeea7031cfdc3 Gerrit-Change-Number: 53953 Gerrit-PatchSet: 5 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> 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]: linux_mtd: move global state into programmer data field
by Edward O'Callaghan (Code Review) 12 May '21

12 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/53947 ) Change subject: linux_mtd: move global state into programmer data field ...................................................................... linux_mtd: move global state into programmer data field BUG=b:161951062 BRANCH=none TEST=builds, reading /dev/mtd0 on Oak succeeds Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/53947 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M linux_mtd.c 1 file changed, 65 insertions(+), 45 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/linux_mtd.c b/linux_mtd.c index 22702e9..7cdba6c 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -31,16 +31,16 @@ #define LINUX_DEV_ROOT "/dev" #define LINUX_MTD_SYSFS_ROOT "/sys/class/mtd" -static FILE *dev_fp = NULL; - -static int mtd_device_is_writeable; - -static int mtd_no_erase; - -/* Size info is presented in bytes in sysfs. */ -static unsigned long int mtd_total_size; -static unsigned long int mtd_numeraseregions; -static unsigned long int mtd_erasesize; /* only valid if numeraseregions is 0 */ +struct linux_mtd_data { + FILE *dev_fp; + int mtd_device_is_writeable; + int mtd_no_erase; + /* Size info is presented in bytes in sysfs. */ + unsigned long int mtd_total_size; + unsigned long int mtd_numeraseregions; + /* only valid if numeraseregions is 0 */ + unsigned long int mtd_erasesize; +}; /* read a string from a sysfs file and sanitize it */ static int read_sysfs_string(const char *sysfs_path, const char *filename, char *buf, int len) @@ -120,7 +120,7 @@ } /* returns 0 to indicate success, non-zero to indicate error */ -static int get_mtd_info(const char *sysfs_path) +static int get_mtd_info(const char *sysfs_path, struct linux_mtd_data *data) { unsigned long int tmp; char mtd_device_name[32]; @@ -130,10 +130,10 @@ return 1; if (tmp & MTD_WRITEABLE) { /* cache for later use by write function */ - mtd_device_is_writeable = 1; + data->mtd_device_is_writeable = 1; } if (tmp & MTD_NO_ERASE) { - mtd_no_erase = 1; + data->mtd_no_erase = 1; } /* Device name */ @@ -141,55 +141,59 @@ return 1; /* Total size */ - if (read_sysfs_int(sysfs_path, "size", &mtd_total_size)) + if (read_sysfs_int(sysfs_path, "size", &data->mtd_total_size)) return 1; - if (popcnt(mtd_total_size) != 1) { + if (popcnt(data->mtd_total_size) != 1) { msg_perr("MTD size is not a power of 2\n"); return 1; } /* Erase size */ - if (read_sysfs_int(sysfs_path, "erasesize", &mtd_erasesize)) + if (read_sysfs_int(sysfs_path, "erasesize", &data->mtd_erasesize)) return 1; - if (popcnt(mtd_erasesize) != 1) { + if (popcnt(data->mtd_erasesize) != 1) { msg_perr("MTD erase size is not a power of 2\n"); return 1; } /* Erase regions */ - if (read_sysfs_int(sysfs_path, "numeraseregions", &mtd_numeraseregions)) + if (read_sysfs_int(sysfs_path, "numeraseregions", &data->mtd_numeraseregions)) return 1; - if (mtd_numeraseregions != 0) { + if (data->mtd_numeraseregions != 0) { msg_perr("Non-uniform eraseblock size is unsupported.\n"); return 1; } msg_pdbg("%s: device_name: \"%s\", is_writeable: %d, " "numeraseregions: %lu, total_size: %lu, erasesize: %lu\n", - __func__, mtd_device_name, mtd_device_is_writeable, - mtd_numeraseregions, mtd_total_size, mtd_erasesize); + __func__, mtd_device_name, data->mtd_device_is_writeable, + data->mtd_numeraseregions, data->mtd_total_size, data->mtd_erasesize); return 0; } static int linux_mtd_probe(struct flashctx *flash) { - if (mtd_no_erase) + struct linux_mtd_data *data = flash->mst->opaque.data; + + if (data->mtd_no_erase) flash->chip->feature_bits |= FEATURE_NO_ERASE; flash->chip->tested = TEST_OK_PREW; - flash->chip->total_size = mtd_total_size / 1024; /* bytes -> kB */ - flash->chip->block_erasers[0].eraseblocks[0].size = mtd_erasesize; - flash->chip->block_erasers[0].eraseblocks[0].count = mtd_total_size / mtd_erasesize; + flash->chip->total_size = data->mtd_total_size / 1024; /* bytes -> kB */ + flash->chip->block_erasers[0].eraseblocks[0].size = data->mtd_erasesize; + flash->chip->block_erasers[0].eraseblocks[0].count = + data->mtd_total_size / data->mtd_erasesize; return 1; } static int linux_mtd_read(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; unsigned int eb_size = flash->chip->block_erasers[0].eraseblocks[0].size; unsigned int i; - if (fseek(dev_fp, start, SEEK_SET) != 0) { + if (fseek(data->dev_fp, start, SEEK_SET) != 0) { msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); return 1; } @@ -203,7 +207,7 @@ unsigned int step = eb_size - ((start + i) % eb_size); step = min(step, len - i); - if (fread(buf + i, step, 1, dev_fp) != 1) { + if (fread(buf + i, step, 1, data->dev_fp) != 1) { msg_perr("Cannot read 0x%06x bytes at 0x%06x: %s\n", step, start + i, strerror(errno)); return 1; @@ -219,13 +223,14 @@ static int linux_mtd_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; unsigned int chunksize = flash->chip->block_erasers[0].eraseblocks[0].size; unsigned int i; - if (!mtd_device_is_writeable) + if (!data->mtd_device_is_writeable) return 1; - if (fseek(dev_fp, start, SEEK_SET) != 0) { + if (fseek(data->dev_fp, start, SEEK_SET) != 0) { msg_perr("Cannot seek to 0x%06x: %s\n", start, strerror(errno)); return 1; } @@ -240,12 +245,12 @@ unsigned int step = chunksize - ((start + i) % chunksize); step = min(step, len - i); - if (fwrite(buf + i, step, 1, dev_fp) != 1) { + if (fwrite(buf + i, step, 1, data->dev_fp) != 1) { msg_perr("Cannot write 0x%06x bytes at 0x%06x\n", step, start + i); return 1; } - if (fflush(dev_fp) == EOF) { + if (fflush(data->dev_fp) == EOF) { msg_perr("Failed to flush buffer: %s\n", strerror(errno)); return 1; } @@ -259,28 +264,29 @@ static int linux_mtd_erase(struct flashctx *flash, unsigned int start, unsigned int len) { + struct linux_mtd_data *data = flash->mst->opaque.data; uint32_t u; - if (mtd_no_erase) { + if (data->mtd_no_erase) { msg_perr("%s: device does not support erasing. Please file a " "bug report at flashrom(a)flashrom.org\n", __func__); return 1; } - if (mtd_numeraseregions != 0) { + if (data->mtd_numeraseregions != 0) { /* TODO: Support non-uniform eraseblock size using use MEMGETREGIONCOUNT/MEMGETREGIONINFO ioctls */ msg_perr("%s: mtd_numeraseregions must be 0\n", __func__); return 1; } - for (u = 0; u < len; u += mtd_erasesize) { + for (u = 0; u < len; u += data->mtd_erasesize) { struct erase_info_user erase_info = { .start = start + u, - .length = mtd_erasesize, + .length = data->mtd_erasesize, }; - if (ioctl(fileno(dev_fp), MEMERASE, &erase_info) == -1) { + if (ioctl(fileno(data->dev_fp), MEMERASE, &erase_info) == -1) { msg_perr("%s: ioctl: %s\n", __func__, strerror(errno)); return 1; } @@ -300,7 +306,7 @@ }; /* Returns 0 if setup is successful, non-zero to indicate error */ -static int linux_mtd_setup(int dev_num) +static int linux_mtd_setup(int dev_num, struct linux_mtd_data *data) { char sysfs_path[32]; int ret = 1; @@ -332,15 +338,15 @@ /* so far so good, get more info from other files in this dir */ if (snprintf(sysfs_path, sizeof(sysfs_path), "%s/mtd%d/", LINUX_MTD_SYSFS_ROOT, dev_num) < 0) goto linux_mtd_setup_exit; - if (get_mtd_info(sysfs_path)) + if (get_mtd_info(sysfs_path, data)) goto linux_mtd_setup_exit; /* open file stream and go! */ - if ((dev_fp = fopen(dev_path, "r+")) == NULL) { + if ((data->dev_fp = fopen(dev_path, "r+")) == NULL) { msg_perr("Cannot open file stream for %s\n", dev_path); goto linux_mtd_setup_exit; } - ret = setvbuf(dev_fp, NULL, _IONBF, 0); + ret = setvbuf(data->dev_fp, NULL, _IONBF, 0); if (ret) msg_pwarn("Failed to set MTD device to unbuffered: %d\n", ret); @@ -353,10 +359,11 @@ static int linux_mtd_shutdown(void *data) { - if (dev_fp != NULL) { - fclose(dev_fp); - dev_fp = NULL; + struct linux_mtd_data *mtd_data = data; + if (mtd_data->dev_fp != NULL) { + fclose(mtd_data->dev_fp); } + free(data); return 0; } @@ -366,6 +373,7 @@ char *param; int dev_num = 0; int ret = 1; + struct linux_mtd_data *data = NULL; param = extract_programmer_param("dev"); if (param) { @@ -398,11 +406,23 @@ goto linux_mtd_init_exit; } - if (linux_mtd_setup(dev_num)) + data = calloc(1, sizeof(*data)); + if (!data) { + msg_perr("Unable to allocate memory for linux_mtd_data\n"); goto linux_mtd_init_exit; + } - if (register_shutdown(linux_mtd_shutdown, NULL)) + /* Get MTD info and store it in `data` */ + if (linux_mtd_setup(dev_num, data)) { + free(data); goto linux_mtd_init_exit; + } + + programmer_linux_mtd.data = data; + if (register_shutdown(linux_mtd_shutdown, (void *)data)) { + free(data); + goto linux_mtd_init_exit; + } register_opaque_master(&programmer_linux_mtd); -- To view, visit https://review.coreboot.org/c/flashrom/+/53947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Gerrit-Change-Number: 53947 Gerrit-PatchSet: 5 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021