January 2023 - flashrom-gerrit

[S] Change in flashrom[master]: layout: Rename find_romentry() -> romentry_exists()
by Edward O'Callaghan (Code Review) 11 Jan '23

11 Jan '23

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/70893 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: layout: Rename find_romentry() -> romentry_exists() ...................................................................... layout: Rename find_romentry() -> romentry_exists() The functions purpose is to test for existence not to actually return the entry, therefore rename accordingly. Change-Id: Ibf14357c00717d1a7b6bc9c83e797fac125559c4 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/70893 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Felix Singer <felixsinger(a)posteo.net> --- M layout.c 1 file changed, 18 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved diff --git a/layout.c b/layout.c index 6af0e41..4f02be6 100644 --- a/layout.c +++ b/layout.c @@ -209,7 +209,7 @@ } /* returns -1 if an entry is not found, 0 if found. */ -static int find_romentry(struct flashrom_layout *const l, char *name, char *file) +static int romentry_exists(struct flashrom_layout *const l, char *name, char *file) { if (!l->head) return -1; @@ -244,7 +244,7 @@ tmp = args; while (tmp) { - if (find_romentry(l, tmp->name, tmp->file) < 0) { + if (romentry_exists(l, tmp->name, tmp->file) < 0) { msg_gerr("Invalid region specified: \"%s\".\n", tmp->name); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/70893 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibf14357c00717d1a7b6bc9c83e797fac125559c4 Gerrit-Change-Number: 70893 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> 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-MessageType: merged

1 0

[M] Change in flashrom[master]: flashrom_tester: Remove --output log redirect option
by Edward O'Callaghan (Code Review) 11 Jan '23

11 Jan '23

Attention is currently required from: Evan Benn, Peter Marheine. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71773 ) Change subject: flashrom_tester: Remove --output log redirect option ...................................................................... Patch Set 2: Code-Review-2 (1 comment) Patchset: PS2: This has been a pretty useful facility during AVL -- To view, visit https://review.coreboot.org/c/flashrom/+/71773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5eab8169644a16ba31b203e8607853c459f92978 Gerrit-Change-Number: 71773 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 11 Jan 2023 12:28:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: rust: Add license and other metadata to Cargo.toml
by Edward O'Callaghan (Code Review) 11 Jan '23

11 Jan '23

Attention is currently required from: Evan Benn. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71618 ) Change subject: rust: Add license and other metadata to Cargo.toml ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/71618 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibdab16713395509be511e45c5eae946496020429 Gerrit-Change-Number: 71618 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(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-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 11 Jan 2023 12:27:14 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c:Add function to get a flattened view of the chip erase blocks
by Edward O'Callaghan (Code Review) 11 Jan '23

11 Jan '23

Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Paul Menzel, Aarya. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65879 ) Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks ...................................................................... Patch Set 56: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/65879 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iafe78de00daa55f7114bd4ce09465dd88074ece4 Gerrit-Change-Number: 65879 Gerrit-PatchSet: 56 Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Wed, 11 Jan 2023 11:57:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Remove --output log redirect option
by Evan Benn (Code Review) 11 Jan '23

11 Jan '23

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71773 ) Change subject: flashrom_tester: Remove --output log redirect option ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/71773 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5eab8169644a16ba31b203e8607853c459f92978 Gerrit-Change-Number: 71773 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 11 Jan 2023 03:26:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: programmer: Add bitbanging programmer driver for Linux libgpiod
by Steve Markgraf (Code Review) 10 Jan '23

10 Jan '23

Steve Markgraf has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71801 ) Change subject: programmer: Add bitbanging programmer driver for Linux libgpiod ...................................................................... programmer: Add bitbanging programmer driver for Linux libgpiod With this driver, any single board computer, old smartphone, etc. with a few spare GPIOs can be used for flashrom. An example invocation looks like this: flashrom -p linux_gpiod:gpiochip=0,cs=18,sck=19,mosi=13,miso=56 This uses /dev/gpiochip0 with the specified GPIO numbers for the SPI lines. All arguments are required. Reading of a 2048 kB flash chip on a Qualcomm MSM8916 SoC @800 MHz: Found GigaDevice flash chip "GD25LQ16" (2048 kB, SPI) on linux_gpiod. real 1m 33.96s Change-Id: Icad3eb7764f28feaea51bda3a7893da724c86d06 Signed-off-by: Steve Markgraf <steve(a)steve-m.de> --- M Makefile M include/programmer.h A linux_gpiod.c M programmer_table.c 4 files changed, 225 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/71801/1 diff --git a/Makefile b/Makefile index 425b58c..33de024 100644 --- a/Makefile +++ b/Makefile @@ -179,6 +179,9 @@ CONFIG_REALTEK_MST_I2C_SPI \ CONFIG_MEDIATEK_I2C_SPI \ +DEPENDS_ON_LIBGPIOD := \ + CONFIG_LINUX_GPIOD \ + ifeq ($(CONFIG_ENABLE_LIBUSB1_PROGRAMMERS), no) $(call disable_all,$(DEPENDS_ON_LIBUSB1)) endif @@ -233,6 +236,10 @@ CONFIG_LIBPCI_CFLAGS := $(call dependency_cflags, libpci) CONFIG_LIBPCI_LDFLAGS := $(call dependency_ldflags, libpci) +CONFIG_LIBGPIOD_VERSION := $(call dependency_version, libgpiod) +CONFIG_LIBGPIOD_CFLAGS := $(call dependency_cflags, libgpiod) +CONFIG_LIBGPIOD_LDFLAGS := $(call dependency_ldflags, libgpiod) + # Determine the destination OS, architecture and endian # IMPORTANT: The following lines must be placed before TARGET_OS, ARCH or ENDIAN # is ever used (of course), but should come after any lines setting CC because @@ -247,6 +254,7 @@ HAS_LIBJAYLINK := $(call find_dependency, libjaylink) HAS_LIBUSB1 := $(call find_dependency, libusb-1.0) HAS_LIBPCI := $(call find_dependency, libpci) +HAS_LIBGPIOD := $(call find_dependency, libgpiod) HAS_PCI_OLD_GET_DEV := $(call c_compile_test, Makefile.d/pci_old_get_dev_test.c, $(CONFIG_LIBPCI_CFLAGS)) HAS_FT232H := $(call c_compile_test, Makefile.d/ft232h_test.c, $(CONFIG_LIBFTDI1_CFLAGS)) @@ -350,6 +358,10 @@ $(call mark_unsupported,$(DEPENDS_ON_LIBUSB1)) endif +ifeq ($(HAS_LIBGPIOD), no) +$(call mark_unsupported,$(DEPENDS_ON_LIBGPIOD)) +endif + ifeq ($(HAS_SERIAL), no) $(call mark_unsupported, $(DEPENDS_ON_SERIAL)) endif @@ -506,9 +518,10 @@ # Always enable Marvell SATA controllers for now. CONFIG_SATAMV ?= yes -# Enable Linux spidev and MTD interfaces by default. We disable them on non-Linux targets. +# Enable Linux spidev, MTD and gpiod interfaces by default. We disable them on non-Linux targets. CONFIG_LINUX_MTD ?= yes CONFIG_LINUX_SPI ?= yes +CONFIG_LINUX_GPIOD ?= yes # Always enable ITE IT8212F PATA controllers for now. CONFIG_IT8212 ?= yes @@ -757,6 +770,11 @@ PROGRAMMER_OBJS += linux_spi.o endif +ifeq ($(CONFIG_LINUX_GPIOD), yes) +FEATURE_FLAGS += -D'CONFIG_LINUX_GPIOD=1' +PROGRAMMER_OBJS += linux_gpiod.o +endif + ifeq ($(CONFIG_MSTARDDC_SPI), yes) FEATURE_FLAGS += -D'CONFIG_MSTARDDC_SPI=1' PROGRAMMER_OBJS += mstarddc_spi.o @@ -865,6 +883,12 @@ endif endif +USE_LIBGPIOD := $(if $(call filter_deps,$(DEPENDS_ON_LIBGPIOD)),yes,no) +ifeq ($(USE_LIBGPIOD), yes) +override CFLAGS += $(CONFIG_LIBGPIOD_CFLAGS) +override LDFLAGS += $(CONFIG_LIBGPIOD_LDFLAGS) +endif + USE_LIB_NI845X := $(if $(call filter_deps,$(DEPENDS_ON_LIB_NI845X)),yes,no) ifeq ($(USE_LIB_NI845X), yes) override CFLAGS += $(CONFIG_LIB_NI845X_CFLAGS) @@ -943,6 +967,11 @@ echo " CFLAGS: $(CONFIG_LIBFTDI1_CFLAGS)"; \ echo " LDFLAGS: $(CONFIG_LIBFTDI1_LDFLAGS)"; \ fi + @echo Dependency libgpiod found: $(HAS_LIBGPIOD) $(CONFIG_LIBGPIOD_VERSION) + @if [ $(HAS_LIBGPIOD) = yes ]; then \ + echo " CFLAGS: $(CONFIG_LIBGPIOD_CFLAGS)"; \ + echo " LDFLAGS: $(CONFIG_LIBGPIOD_LDFLAGS)"; \ + fi @echo "Checking for header \"mtd/mtd-user.h\": $(HAS_LINUX_MTD)" @echo "Checking for header \"linux/spi/spidev.h\": $(HAS_LINUX_SPI)" @echo "Checking for header \"linux/i2c-dev.h\": $(HAS_LINUX_I2C)" diff --git a/include/programmer.h b/include/programmer.h index 67e7b4a..e364c63 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -72,6 +72,7 @@ extern const struct programmer_entry programmer_jlink_spi; extern const struct programmer_entry programmer_linux_mtd; extern const struct programmer_entry programmer_linux_spi; +extern const struct programmer_entry programmer_linux_gpiod; extern const struct programmer_entry programmer_parade_lspcon; extern const struct programmer_entry programmer_mediatek_i2c_spi; extern const struct programmer_entry programmer_mstarddc_spi; diff --git a/linux_gpiod.c b/linux_gpiod.c new file mode 100644 index 0000000..1e0fa67 --- /dev/null +++ b/linux_gpiod.c @@ -0,0 +1,165 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2023 Steve Markgraf <steve(a)steve-m.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <gpiod.h> +#include "programmer.h" +#include "spi.h" +#include "flash.h" + +#define CONSUMER "flashrom" + +struct gpiod_spi_data { + struct gpiod_chip *chip; + struct gpiod_line_bulk bulk; + struct gpiod_line *cs_line, *sck_line, *mosi_line, *miso_line; +}; + +static void linux_gpiod_bitbang_set_cs(int val, void *spi_data) +{ + struct gpiod_spi_data *data = spi_data; + if (gpiod_line_set_value(data->cs_line, val) < 0) + msg_perr("Setting cs line failed\n"); +} + +static void linux_gpiod_bitbang_set_sck(int val, void *spi_data) +{ + struct gpiod_spi_data *data = spi_data; + if (gpiod_line_set_value(data->sck_line, val) < 0) + msg_perr("Setting sck line failed\n"); +} + +static void linux_gpiod_bitbang_set_mosi(int val, void *spi_data) +{ + struct gpiod_spi_data *data = spi_data; + if (gpiod_line_set_value(data->mosi_line, val) < 0) + msg_perr("Setting sck line failed\n"); +} + +static int linux_gpiod_bitbang_get_miso(void *spi_data) +{ + struct gpiod_spi_data *data = spi_data; + int r = gpiod_line_get_value(data->miso_line); + if (r < 0) + msg_perr("Getting miso line failed\n"); + return r; +} + +static const struct bitbang_spi_master bitbang_spi_master_gpiod = { + .set_cs = linux_gpiod_bitbang_set_cs, + .set_sck = linux_gpiod_bitbang_set_sck, + .set_mosi = linux_gpiod_bitbang_set_mosi, + .get_miso = linux_gpiod_bitbang_get_miso, +}; + +static int linux_gpiod_spi_shutdown(void *spi_data) +{ + struct gpiod_spi_data *data = spi_data; + + if (gpiod_line_bulk_num_lines(&data->bulk) > 0) + gpiod_line_release_bulk(&data->bulk); + + if (data->chip) + gpiod_chip_close(data->chip); + + free(data); + + return 0; +} + +static int linux_gpiod_spi_init(const struct programmer_cfg *cfg) +{ + struct gpiod_spi_data *data = NULL; + struct gpiod_chip *chip = NULL; + const char *param_str[] = { "cs", "sck", "mosi", "miso", "gpiochip" }; + unsigned int param_int[ARRAY_SIZE(param_str)]; + unsigned int i; + int r; + + for (i = 0; i < ARRAY_SIZE(param_str); i++) { + char *param = extract_programmer_param_str(cfg, param_str[i]); + if (param) { + param_int[i] = atoi(param); + free(param); + } else { + msg_perr("Missing required programmer parameter %s=<n>\n", param_str[i]); + goto err_exit; + } + } + + data = calloc(1, sizeof(*data)); + if (!data) { + msg_perr("Unable to allocate space for SPI master data\n"); + goto err_exit; + } + + chip = gpiod_chip_open_by_number(param_int[4]); + if (!chip) { + msg_perr("Failed to open gpiochip: %s\n", strerror(errno)); + goto err_exit; + } + + data->chip = chip; + + if (gpiod_chip_get_lines(chip, param_int, 4, &data->bulk)) { + msg_perr("Error getting GPIO lines\n"); + goto err_exit; + } + + data->cs_line = gpiod_line_bulk_get_line(&data->bulk, 0); + data->sck_line = gpiod_line_bulk_get_line(&data->bulk, 1); + data->mosi_line = gpiod_line_bulk_get_line(&data->bulk, 2); + data->miso_line = gpiod_line_bulk_get_line(&data->bulk, 3); + + r = gpiod_line_request_output(data->cs_line, CONSUMER, 1); + r |= gpiod_line_request_output(data->sck_line, CONSUMER, 1); + r |= gpiod_line_request_output(data->mosi_line, CONSUMER, 1); + r |= gpiod_line_request_input(data->miso_line, CONSUMER); + + if (r < 0) { + msg_perr("Requesting GPIO lines failed\n"); + goto err_exit; + } + + if (register_shutdown(linux_gpiod_spi_shutdown, data)) + goto err_exit; + + if (register_spi_bitbang_master(&bitbang_spi_master_gpiod, data)) + return 1; /* shutdown function does the cleanup */ + + return 0; + +err_exit: + if (data) { + if (gpiod_line_bulk_num_lines(&data->bulk) > 0) + gpiod_line_release_bulk(&data->bulk); + + free(data); + } + + if (chip) + gpiod_chip_close(chip); + return 1; +} + +const struct programmer_entry programmer_linux_gpiod = { + .name = "linux_gpiod", + .type = OTHER, + .devs.note = "Device file /dev/gpiochip<n>\n", + .init = linux_gpiod_spi_init, +}; diff --git a/programmer_table.c b/programmer_table.c index d58a155..848cd4e 100644 --- a/programmer_table.c +++ b/programmer_table.c @@ -124,6 +124,10 @@ &programmer_linux_spi, #endif +#if CONFIG_LINUX_GPIOD == 1 + &programmer_linux_gpiod, +#endif + #if CONFIG_PARADE_LSPCON == 1 &programmer_parade_lspcon, #endif -- To view, visit https://review.coreboot.org/c/flashrom/+/71801 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icad3eb7764f28feaea51bda3a7893da724c86d06 Gerrit-Change-Number: 71801 Gerrit-PatchSet: 1 Gerrit-Owner: Steve Markgraf <steve(a)steve-m.de> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: sfdp.c : Initializing hbuf to avoid warnings generated by scan-build
by Name of user not set (Code Review) 10 Jan '23

10 Jan '23

Attention is currently required from: eshankelkar(a)galorithm.com. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/71791 to look at the new patch set (#3). Change subject: sfdp.c : Initializing hbuf to avoid warnings generated by scan-build ...................................................................... sfdp.c : Initializing hbuf to avoid warnings generated by scan-build scan-build uses the clang analyzer which is giving warnings about garbage/undefined values in hbuf assigned to member of hdrs[0] if hbuf is not initialized. Though the path of the control flow shown by it in its html output cannot occur in reality(since it assumes that (nph+1)*8 is <= 0 i.e nph<=-1 but later assumes 0 <= nph ) hence its a false positive. Still initializing all bytes of hbuf to 0 leads to the two warnings for sfdp.c (one for hbuf, one for tbuf) to go away. Signed-off-by : Eshan Kelkar <eshankelkar(a)galorithm.com> Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b --- M sfdp.c 1 file changed, 28 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/71791/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/71791 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b Gerrit-Change-Number: 71791 Gerrit-PatchSet: 3 Gerrit-Owner: eshankelkar(a)galorithm.com Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: eshankelkar(a)galorithm.com Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: sfdp.c : Initializing hbuf to avoid warnings generated by scan-build
by Name of user not set (Code Review) 10 Jan '23

10 Jan '23

eshankelkar(a)galorithm.com has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71791 ) Change subject: sfdp.c : Initializing hbuf to avoid warnings generated by scan-build ...................................................................... sfdp.c : Initializing hbuf to avoid warnings generated by scan-build scan-build uses the clang analyzer which is giving warnings about garbage/undefined values in hbuf assigned to member of hdrs[0] if hbuf is not initialized. Though the path of the control flow shown by it in its html output cannot occur in reality(since it assumes that (nph+1)*8 is <= 0 i.e nph<=-1 but later assumes 0 <= nph ) hence its a false positive. Still initializing all bytes of hbuf to 0 leads to the two warnings for sfdp.c (one for hbuf, one for tbuf) to go away. Signed-off by : Eshan Kelkar <eshankelkar(a)galorithm.com> Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b --- M sfdp.c 1 file changed, 27 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/71791/1 diff --git a/sfdp.c b/sfdp.c index 5eddb21..b5010d6 100644 --- a/sfdp.c +++ b/sfdp.c @@ -296,6 +296,12 @@ /* Fetch all parameter headers, even if we don't use them all (yet). */ hbuf = malloc((nph + 1) * 8); + uint8_t k; + for (i=0; i<=nph; i++){ + for (k=0 ; k<=7 ; k++) + hbuf[i*8+k]=(uint8_t)0; + } + hdrs = malloc((nph + 1) * sizeof(*hdrs)); if (hbuf == NULL || hdrs == NULL ) { msg_gerr("Out of memory!\n"); -- To view, visit https://review.coreboot.org/c/flashrom/+/71791 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b Gerrit-Change-Number: 71791 Gerrit-PatchSet: 1 Gerrit-Owner: eshankelkar(a)galorithm.com Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: internal.c: Move sio register to own object
by Edward O'Callaghan (Code Review) 10 Jan '23

10 Jan '23

Attention is currently required from: Thomas Heijligen, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71577 ) Change subject: internal.c: Move sio register to own object ...................................................................... Patch Set 2: (1 comment) File superio.c: https://review.coreboot.org/c/flashrom/+/71577/comment/a75f571a_637f144b PS1, Line 40: //probe_superio_smsc(); > Wow, this is dead code Well let's get this in 'as-is' as it just moves around code. Cleanups can be separate commits. -- To view, visit https://review.coreboot.org/c/flashrom/+/71577 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9a4c3e12bce5d22492c8d1b8f4a3f49d736dcf31 Gerrit-Change-Number: 71577 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 10 Jan 2023 10:03:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests/: Add non-aligned write within a region unit-test
by Edward O'Callaghan (Code Review) 10 Jan '23

10 Jan '23

Attention is currently required from: Sam McNally, Anastasia Klimchuk, Evan Benn. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 ) Change subject: tests/: Add non-aligned write within a region unit-test ...................................................................... Patch Set 2: (16 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/71659/comment/0901f4f6_097bdf43 PS2, Line 7: tests/: Add subregion alignment unit-test > is a subregion a thing? […] Done https://review.coreboot.org/c/flashrom/+/71659/comment/6ca559fc_d50b4381 PS2, Line 9: A written region that is sized below that of the erasure granularity > I don't understand the bug from this description should make more sense now given all the other fixes give context. File tests/chip.c: https://review.coreboot.org/c/flashrom/+/71659/comment/2dbdad01_75b44fb5 PS2, Line 110: va_list logfile_args; > you dont need to va_copy because you only pass ap to vfprintf Ack https://review.coreboot.org/c/flashrom/+/71659/comment/f25d06f3_9a15fed8 PS2, Line 114: output_type = stderr; > I don't know the motivation for printing the messages, so I also dont know the motivation for splitt […] Ack https://review.coreboot.org/c/flashrom/+/71659/comment/96e19d18_041e83d5 PS2, Line 116: if (level <= verbose_screen) { > verbose_screen is part of `cli_output. […] Ack https://review.coreboot.org/c/flashrom/+/71659/comment/9f190440_a96060c8 PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly > why do we need to fflush at all? Ack https://review.coreboot.org/c/flashrom/+/71659/comment/04957694_341b54a0 PS2, Line 444: void write_chip_subregion_with_dummyflasher_test_success(void **state) // XXX > this is the first mention of a 'subregion' in flashrom, maybe there is an existing word you can choo […] Done https://review.coreboot.org/c/flashrom/+/71659/comment/20aeae7f_05b0d69b PS2, Line 458: struct flashchip mock_chip = chip_W25Q128_V; > do you want to assert something about the size of the erasers here? as much for documentation as en […] Well that should all be part of the W25Q128V chip spec this struct is defining. That sounds like another test to add to ensure the data structure is maintained or a comment above it. Either way, maybe out of scope here. https://review.coreboot.org/c/flashrom/+/71659/comment/4c742e8a_f82500af PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC > this can be a const int instead of a define well not quite in this case because reasons, however I added a comment. https://review.coreboot.org/c/flashrom/+/71659/comment/5b635c7d_837de663 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}. > im not sure what this is telling me Done https://review.coreboot.org/c/flashrom/+/71659/comment/2ba94eae_f9400a56 PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size); > memsetting so calloc could be malloc I prefer consistent use of calloc() that ensures heap is always zero. malloc nano optimisation for a uninit heap gains me nothing over the advantage of consistency. https://review.coreboot.org/c/flashrom/+/71659/comment/05d64979_94ba7308 PS2, Line 477: flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false); > suspect these flags are important what is being tested, but its not documented here or […] Documenting those flags better is outside the scope of this change. However I agree they need better doxygen comments in libflashrom.h https://review.coreboot.org/c/flashrom/+/71659/comment/e80af7bd_d27a532c PS2, Line 478: flashrom_set_log_callback((flashrom_log_callback *)&unittest_print_cb); > why are we setting up a print callback? We also dont seem to undo this at the end, which will be tri […] Ack https://review.coreboot.org/c/flashrom/+/71659/comment/9b3356db_91ef6bed PS2, Line 502: printf("Subregion chip W op..\n"); > write the word Done https://review.coreboot.org/c/flashrom/+/71659/comment/24fbe3ff_33366af9 PS2, Line 508: //flashrom_layout_set(&flashctx, NULL); // use default layout. > the layout you use is the same as the default or NULL layout, does that not work? No, it segfauls for some unknown reason outside the scope of the work here. https://review.coreboot.org/c/flashrom/+/71659/comment/692915fe_1814f6a8 PS2, Line 514: assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, mock_chip_size)); > maybe documenting right here what the predicted bug is would be useful. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Tue, 10 Jan 2023 00:43:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit January 2023