March 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[1.1.x]: tree: Make internal variables static
by Nico Huber (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Nico Huber, Angel Pons. Hello build bot (Jenkins), Angel Pons, Jacob Garber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/62612 to look at the new patch set (#2). Change subject: tree: Make internal variables static ...................................................................... tree: Make internal variables static All these variables are only used in the files they are defined in, so they can be made static. (Backported as it untangles aliased global objects.) Change-Id: I1e55138adef540e9d3a2237aa5b289cb338c0608 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Reviewed-on: https://review.coreboot.org/c/flashrom/+/33747 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c M developerbox_spi.c M dummyflasher.c M gfxnvidia.c M it85spi.c M layout.c M mcp6x_spi.c M nicintel.c M nicintel_spi.c M satamv.c 10 files changed, 18 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/62612/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62612 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I1e55138adef540e9d3a2237aa5b289cb338c0608 Gerrit-Change-Number: 62612 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[1.1.x]: dmi: Correctly check for ERROR_PTR
by Nico Huber (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Angel Pons. Hello Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/62613 to review the following change. Change subject: dmi: Correctly check for ERROR_PTR ...................................................................... dmi: Correctly check for ERROR_PTR For the physmap*() functions, NULL is considered valid return value. Fixes a segmentation fault when DMI tables can't be mapped. Tested on intel/eblake board with broken coreboot. Change-Id: Ic403c2940c2b91acbd113f0acfa3ce9ef6c6bb6c Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M dmi.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/62613/1 diff --git a/dmi.c b/dmi.c index 79321ce..cea07f8 100644 --- a/dmi.c +++ b/dmi.c @@ -164,7 +164,7 @@ int i = 0, j = 0; uint8_t *dmi_table_mem = physmap_ro("DMI Table", base, len); - if (dmi_table_mem == NULL) { + if (dmi_table_mem == ERROR_PTR) { msg_perr("Unable to access DMI Table\n"); return; } -- To view, visit https://review.coreboot.org/c/flashrom/+/62613 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Ic403c2940c2b91acbd113f0acfa3ce9ef6c6bb6c Gerrit-Change-Number: 62613 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[1.1.x]: tree: Make internal variables static
by Nico Huber (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Angel Pons, Jacob Garber. Hello build bot (Jenkins), Angel Pons, Jacob Garber, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/62612 to review the following change. Change subject: tree: Make internal variables static ...................................................................... tree: Make internal variables static All these variables are only used in the files they are defined in, so they can be made static. Change-Id: I1e55138adef540e9d3a2237aa5b289cb338c0608 Signed-off-by: Jacob Garber <jgarber1(a)ualberta.ca> Reviewed-on: https://review.coreboot.org/c/flashrom/+/33747 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M dediprog.c M developerbox_spi.c M dummyflasher.c M gfxnvidia.c M it85spi.c M layout.c M mcp6x_spi.c M nicintel.c M nicintel_spi.c M satamv.c 10 files changed, 18 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/12/62612/1 diff --git a/dediprog.c b/dediprog.c index c99c922..9379773 100644 --- a/dediprog.c +++ b/dediprog.c @@ -43,7 +43,7 @@ #define REQTYPE_OTHER_IN (LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_OTHER) /* 0xC3 */ #define REQTYPE_EP_OUT (LIBUSB_ENDPOINT_OUT | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_ENDPOINT) /* 0x42 */ #define REQTYPE_EP_IN (LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | LIBUSB_RECIPIENT_ENDPOINT) /* 0xC2 */ -struct libusb_context *usb_ctx; +static struct libusb_context *usb_ctx; static libusb_device_handle *dediprog_handle; static int dediprog_in_endpoint; static int dediprog_out_endpoint; @@ -159,7 +159,7 @@ }; static int dediprog_firmwareversion = FIRMWARE_VERSION(0, 0, 0); -enum dediprog_devtype dediprog_devicetype = DEV_UNKNOWN; +static enum dediprog_devtype dediprog_devicetype = DEV_UNKNOWN; #if defined(LIBUSB_MAJOR) && defined(LIBUSB_MINOR) && defined(LIBUSB_MICRO) && \ LIBUSB_MAJOR <= 1 && LIBUSB_MINOR == 0 && LIBUSB_MICRO < 9 diff --git a/developerbox_spi.c b/developerbox_spi.c index 4ad966e..46da80e 100644 --- a/developerbox_spi.c +++ b/developerbox_spi.c @@ -60,7 +60,7 @@ {0}, }; -struct libusb_context *usb_ctx; +static struct libusb_context *usb_ctx; static libusb_device_handle *cp210x_handle; static int cp210x_gpio_get(void) diff --git a/dummyflasher.c b/dummyflasher.c index 2929686..53801d9 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -55,10 +55,10 @@ static unsigned int emu_jedec_be_d8_size = 0; static unsigned int emu_jedec_ce_60_size = 0; static unsigned int emu_jedec_ce_c7_size = 0; -unsigned char spi_blacklist[256]; -unsigned char spi_ignorelist[256]; -int spi_blacklist_size = 0; -int spi_ignorelist_size = 0; +static unsigned char spi_blacklist[256]; +static unsigned char spi_ignorelist[256]; +static int spi_blacklist_size = 0; +static int spi_ignorelist_size = 0; static uint8_t emu_status = 0; /* A legit complete SFDP table based on the MX25L6436E (rev. 1.8) datasheet. */ @@ -128,7 +128,7 @@ .chip_writen = dummy_chip_writen, }; -enum chipbustype dummy_buses_supported = BUS_NONE; +static enum chipbustype dummy_buses_supported = BUS_NONE; static int dummy_shutdown(void *data) { diff --git a/gfxnvidia.c b/gfxnvidia.c index c6d752e..d8ea4d6 100644 --- a/gfxnvidia.c +++ b/gfxnvidia.c @@ -28,7 +28,7 @@ #define GFXNVIDIA_MEMMAP_MASK ((1 << 17) - 1) #define GFXNVIDIA_MEMMAP_SIZE (16 * 1024 * 1024) -uint8_t *nvidia_bar; +static uint8_t *nvidia_bar; const struct dev_entry gfx_nvidia[] = { {0x10de, 0x0010, NT, "NVIDIA", "Mutara V08 [NV2]" }, diff --git a/it85spi.c b/it85spi.c index e47cabb..b616a26 100644 --- a/it85spi.c +++ b/it85spi.c @@ -74,9 +74,9 @@ #endif /* LPC_IO */ #ifdef LPC_IO -unsigned int shm_io_base; +static unsigned int shm_io_base; #endif -unsigned char *ce_high, *ce_low; +static unsigned char *ce_high, *ce_low; static int it85xx_scratch_rom_reenter = 0; /* This function will poll the keyboard status register until either diff --git a/layout.c b/layout.c index 5080e3e..96434e6 100644 --- a/layout.c +++ b/layout.c @@ -23,7 +23,7 @@ #include "programmer.h" #include "layout.h" -struct romentry entries[MAX_ROMLAYOUT]; +static struct romentry entries[MAX_ROMLAYOUT]; static struct flashrom_layout layout = { entries, 0 }; /* include_args holds the arguments specified at the command line with -i. They must be processed at some point diff --git a/mcp6x_spi.c b/mcp6x_spi.c index 971780d..d7dbdf0 100644 --- a/mcp6x_spi.c +++ b/mcp6x_spi.c @@ -36,7 +36,7 @@ #define MCP6X_SPI_REQUEST 0 #define MCP6X_SPI_GRANT 8 -void *mcp6x_spibar = NULL; +static void *mcp6x_spibar = NULL; /* Cached value of last GPIO state. */ static uint8_t mcp_gpiostate; diff --git a/nicintel.c b/nicintel.c index a4424e7..4672890 100644 --- a/nicintel.c +++ b/nicintel.c @@ -20,8 +20,8 @@ #include "programmer.h" #include "hwaccess.h" -uint8_t *nicintel_bar; -uint8_t *nicintel_control_bar; +static uint8_t *nicintel_bar; +static uint8_t *nicintel_control_bar; const struct dev_entry nics_intel[] = { {PCI_VENDOR_ID_INTEL, 0x1209, NT, "Intel", "8255xER/82551IT Fast Ethernet Controller"}, diff --git a/nicintel_spi.c b/nicintel_spi.c index f31a2f9..f5c9ed1 100644 --- a/nicintel_spi.c +++ b/nicintel_spi.c @@ -75,7 +75,7 @@ #define BIT(x) (1<<(x)) -uint8_t *nicintel_spibar; +static uint8_t *nicintel_spibar; const struct dev_entry nics_intel_spi[] = { {PCI_VENDOR_ID_INTEL, 0x105e, OK, "Intel", "82571EB Gigabit Ethernet Controller"}, diff --git a/satamv.c b/satamv.c index 1d3734a..31265ea 100644 --- a/satamv.c +++ b/satamv.c @@ -22,8 +22,8 @@ #include "programmer.h" #include "hwaccess.h" -uint8_t *mv_bar; -uint16_t mv_iobar; +static uint8_t *mv_bar; +static uint16_t mv_iobar; const struct dev_entry satas_mv[] = { /* 88SX6041 and 88SX6042 are the same according to the datasheet. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/62612 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: I1e55138adef540e9d3a2237aa5b289cb338c0608 Gerrit-Change-Number: 62612 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Jacob Garber <jgarber1(a)ualberta.ca> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dmi: Correctly check for ERROR_PTR
by Nico Huber (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Angel Pons. Hello Angel Pons, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/62611 to review the following change. Change subject: dmi: Correctly check for ERROR_PTR ...................................................................... dmi: Correctly check for ERROR_PTR For the physmap*() functions, NULL is considered valid return value. Fixes a segmentation fault when DMI tables can't be mapped. Tested on intel/eblake board with broken coreboot. Change-Id: Ic403c2940c2b91acbd113f0acfa3ce9ef6c6bb6c Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M dmi.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/11/62611/1 diff --git a/dmi.c b/dmi.c index 16d350d..a7b300d 100644 --- a/dmi.c +++ b/dmi.c @@ -164,7 +164,7 @@ unsigned int i = 0, j = 0; uint8_t *dmi_table_mem = physmap_ro("DMI Table", base, len); - if (dmi_table_mem == NULL) { + if (dmi_table_mem == ERROR_PTR) { msg_perr("Unable to access DMI Table\n"); return; } -- To view, visit https://review.coreboot.org/c/flashrom/+/62611 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic403c2940c2b91acbd113f0acfa3ce9ef6c6bb6c Gerrit-Change-Number: 62611 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: ichspi: Add Alder Lake support
by Subrata Banik (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Nico Huber, Rizwan Qureshi, Edward O'Callaghan, Angel Pons. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62251 ) Change subject: ichspi: Add Alder Lake support ...................................................................... Patch Set 14: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/62251 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie66cf519df13f3391c41f5016b16a81ef3dfd4bf Gerrit-Change-Number: 62251 Gerrit-PatchSet: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 05 Mar 2022 13:13:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Nico Huber (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Michael Niewöhner, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (1 comment) File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/ec3651a4_0504d635 PS29, Line 411: } > Yes, correct. […] I trust you that we need to do this. The thing is, we need to ensure flashrom actually does it. ;) Normally, there is no order enforced of the blocks to be written. But in case of `write_mode == 0` this function must be called with the last block last (and it mustn't be skipped) and before that it must be called with the first block (so we know the first KiB). Currently this happens by coincidence because the whole flash is erased, the way the core flashrom code is written, we expect that the last block of the contents to be flashed are not empty, and we expect that the changes in contents need an erase. We could leave comments everywhere about this (still with the risk that it breaks because of changes somewhere else). Or we could try to find a way to make it work by definition. A hunch tells me that the write granularity could help there. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 05 Mar 2022 13:06:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ichspi: Add Alder Lake support
by Edward O'Callaghan (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Nico Huber, Rizwan Qureshi, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62251 ) Change subject: ichspi: Add Alder Lake support ...................................................................... Patch Set 14: (1 comment) Patchset: PS12: > Sorry I think I didn't write well. We are saying the same thing. […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/62251 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie66cf519df13f3391c41f5016b16a81ef3dfd4bf Gerrit-Change-Number: 62251 Gerrit-PatchSet: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 05 Mar 2022 12:03:09 +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> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michael Niewöhner (Code Review) 05 Mar '22

05 Mar '22

Attention is currently required from: Felix Singer, Nico Huber, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (1 comment) File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/67f6d3e1_b65487b4 PS29, Line 411: } > When we think about flashrom as an application to simply replace flash content, these problems are o […] Yes, correct. Flashing the ec directly on hardware level is possible but not a good idea at runtime, since flashing would interfere with the currently running firmware. Thus, the ec firmware implements flashing with help of the scratch region, as Michal described. We have to comply with the logic implemented there. -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 05 Mar 2022 09:37:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
by Michał Żygowski (Code Review) 04 Mar '22

04 Mar '22

Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Sergii Dmytruk. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ...................................................................... Patch Set 29: (2 comments) Patchset: PS29: > I mean it should be fully included in `-p internal` eventually. The `internal` […] Understood. It may be a good idea to merge it with internal programmer indeed. File ite_ecfw.c: https://review.coreboot.org/c/flashrom/+/55715/comment/ffb5fb1a_704ee98d PS29, Line 411: } > AFAICS, if we'd allow a partial erase, flashrom could decide to only write […] When we think about flashrom as an application to simply replace flash content, these problems are only theoretical. But if we look at it hardware-wise, it is a different story. The main problem with this first kbyte is that it contains the code responsible for writing to the flash. This is a special scratch memory which is doublemapped to 8051 External Memory and ROM space. When the flashing process begins, the flash access routines are copied to the scratch ROM space 4KB where EC can safely execute them without worrying about the flash content (EC wouldn't be able to fetch code when the SPI flash is in WIP/busy state). The command that is sent when writing the first kilobyte probably indicates to the EC that: 1. The flash access is finished after it takes the first kilobyte to be flashed. 2. EC can switch back from scratch ROM to the flash ROM with the execution. That is why it must be done at the end of flashing process. I hope I didn't make any mistake here, Michael can confirm -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 29 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Fri, 04 Mar 2022 15:53:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: layout: Change signature for prepare_layout_for_extraction
by Nico Huber (Code Review) 04 Mar '22

04 Mar '22

Attention is currently required from: Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62554 ) Change subject: layout: Change signature for prepare_layout_for_extraction ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Thanks for reply! :) > > > I'm not sure if I mentioned this somewhere already, the topic seems familiar. I kind of see this extract feature as CLI code. > > Sorry if I missed that! I don't remember, but maybe because extract feature was upstreamed earlier when I just started and I wasn't fully aware of all everything that going on. Don't worry, if we discussed this somewhere else already, I lost track of it anyway. > My reasoning for this patch chain was to align with larger movement to make cli a libflashrom user, which means libflashrom is used from everywhere and needs to be tested. Yes, I got that :) we need to adapt libflashrom API for this. Making existing functions public isn't always the best way, though. > > > So far, we have kept libflashrom free from file i/o and some target environments just don't support it > > Maybe I am missing something, but I don't see `prepare_layout_for_extraction` doing file i/o at the moment? It is populating file names, which is prep step for i/o, but the i/o itself is done next in read operation. That's right. But for prepare_layout_for_extraction() to be useful in the libflashrom API, we'd need another function there that probably does what write_buf_to_include_args() currently does. And that's doing file i/o. Also, the description in your next patch says "Fill in file name". So I'd say it's part of a file i/o concept. It's why I generally prefer to discuss the coarse direction of a topic on the mailing list first. Here on Gerrit, people are often focused to bring patches in step-by-step, but sometimes the bigger picture is only visible at the end of a patch train. It can be frustrating for both authors and reviewers to see that the effort on prior reviews doesn't pan out. Or worse, they might want to merge something that they consider wrong just because they put all the effort into it. > Maybe `prepare_layout_for_extraction` can be renamed into something more specific? I would much prefer to discuss first if the whole topic belongs into libflashrom or the CLI. > I agree with keeping libflashrom free from file i/o. -- To view, visit https://review.coreboot.org/c/flashrom/+/62554 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7d3874d1097bb0d7bb8d9fa8e639cc1e71407627 Gerrit-Change-Number: 62554 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 04 Mar 2022 13:55:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit March 2022