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

Change in flashrom[master]: ft2232_spi.c: change the chunksize to 280
by Simon Buhrow (Code Review) 27 Sep '21

27 Sep '21

Simon Buhrow has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43282 ) Change subject: ft2232_spi.c: change the chunksize to 280 ...................................................................... ft2232_spi.c: change the chunksize to 280 Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a --- M ft2232_spi.c 1 file changed, 12 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/43282/1 diff --git a/ft2232_spi.c b/ft2232_spi.c index 0799af4..0a8d631 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -398,7 +398,18 @@ msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(ftdic)); } - if (ftdi_write_data_set_chunksize(ftdic, 270)) { + if (ftdi_write_data_set_chunksize(ftdic, 280)) { + /* + * 280 bytes = + * + 9 B (CMD) + * + 1 B (WREN) + * + 9 B (CMD) + * + 1 B (op) + * + 4 B (addr) + * + 256 B (page data) + * + * With op: PageProgram or Erase; CMD: FTDI-Chip commands + */ msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic)); } -- To view, visit https://review.coreboot.org/c/flashrom/+/43282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5e096937834fc8fc8623c8c59ade529de081b72a Gerrit-Change-Number: 43282 Gerrit-PatchSet: 1 Gerrit-Owner: Simon Buhrow Gerrit-MessageType: newchange

4 17

Change in ...flashrom[master]: layout: Introduce layout_next()
by Nico Huber (Code Review) 01 Jul '21

01 Jul '21

Hello Angel Pons, Arthur Heymans, David Hendricks, Thomas Heijligen, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/33542 to review the following change. Change subject: layout: Introduce layout_next() ...................................................................... layout: Introduce layout_next() And use it to compare the layouts in flashrom_layout_read_from_ifd() in depth. Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa Signed-off-by: Nico Huber <nico.h(a)gmx.de> --- M layout.c M layout.h M libflashrom.c 3 files changed, 20 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/33542/1 diff --git a/layout.c b/layout.c index e49189e..684615c 100644 --- a/layout.c +++ b/layout.c @@ -252,7 +252,7 @@ return lowest; } -const struct romentry *layout_next_included( +const struct romentry *layout_next( const struct flashrom_layout *const layout, const struct romentry *iterator) { const struct romentry *const end = layout->entries + layout->num_entries; @@ -262,14 +262,21 @@ else iterator = &layout->entries[0]; - for (; iterator < end; ++iterator) { - if (!iterator->included) - continue; + if (iterator < end) return iterator; - } return NULL; } +const struct romentry *layout_next_included( + const struct flashrom_layout *const layout, const struct romentry *iterator) +{ + while ((iterator = layout_next(layout, iterator))) { + if (iterator->included) + break; + } + return iterator; +} + /** * @addtogroup flashrom-layout * @{ diff --git a/layout.h b/layout.h index 53a20d6..f5444e2 100644 --- a/layout.h +++ b/layout.h @@ -66,5 +66,6 @@ int process_include_args(struct flashrom_layout *l, const struct layout_include_args *const args); const struct romentry *layout_next_included_region(const struct flashrom_layout *, chipoff_t); const struct romentry *layout_next_included(const struct flashrom_layout *, const struct romentry *); +const struct romentry *layout_next(const struct flashrom_layout *, const struct romentry *); #endif /* !__LAYOUT_H__ */ diff --git a/libflashrom.c b/libflashrom.c index af62002..6743d50 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -345,8 +345,13 @@ goto _finalize_ret; } - if (chip_layout->base.num_entries != dump_layout.base.num_entries || - memcmp(chip_layout->entries, dump_layout.entries, sizeof(dump_layout.entries))) { + const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL); + const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL); + while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) { + chip_entry = layout_next(&chip_layout->base, chip_entry); + dump_entry = layout_next(&dump_layout.base, dump_entry); + } + if (chip_entry || dump_entry) { msg_cerr("Descriptors don't match!\n"); ret = 5; goto _finalize_ret; -- To view, visit https://review.coreboot.org/c/flashrom/+/33542 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I284958471c61344d29d92c95d88475065a9ca9aa Gerrit-Change-Number: 33542 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-MessageType: newchange

5 25

Change in flashrom[master]: [RFC] spi_master: Add shutdown function in spi_master struct
by Anastasia Klimchuk (Code Review) 30 Jun '21

30 Jun '21

Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55932 ) Change subject: [RFC] spi_master: Add shutdown function in spi_master struct ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > The "few" wasn't meant literally :) […] Oh and I also can see, for the masters without programmer entry, the entry does not even exist in programmer_table.c. Maybe then my question doesn't make sense, but initially it was, why two structs are separated, and there is no way to get programmer entry from spi_master. There is for example programmer name and type in programmer_entry, but no way to access this from an instance of spi_master. Is the answer "it should never be needed" or "it was never needed"? -- To view, visit https://review.coreboot.org/c/flashrom/+/55932 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2dc80dceca2f8204bcd0dad1f51753d7e79f1af5 Gerrit-Change-Number: 55932 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: Thomas Heijligen <src(a)posteo.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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 30 Jun 2021 23:25:59 +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

Change in flashrom[master]: programmer: Introduce default shutdown function
by Anastasia Klimchuk (Code Review) 30 Jun '21

30 Jun '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/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: > I wanted to say: let's not bikeshed, we won't need it for long. […] There are two reasons why I thought of default shutdown function. First reason is illustrated in this patch: instead of adding the simplest (and identical code) shutdown function into individual masters, have it added once and reuse. There are ~10 more par masters in the same situation. I am afraid we may need it for longer, the more I think about it. Strictly speaking, it matters when freeing the data (not allocation) moves to common code. But looking at how it currently works, shutdown function is needed to be registered (even if it does nothing), as a signal to free data. It all can be re-written, but that's what I mean "we may need it for longer". Second reason, completely independent is that some time ago when we were discussing CB:51676, there was an idea that shutdown function should be required. I think, if it is required, maybe to have default function available? There is `default_spi_send_multicommand` for example. -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 21:57:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Normalize chipset detection
by Nico Huber (Code Review) 30 Jun '21

30 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... ich_descriptors: Normalize chipset detection Normalize the fallback paths: * Always end with the newest, assumed compatible chipset. * Perform tighter checks when it's about warnings only. * If two chipsets seem compatible, always return the same (this is currently the case for 8/9 series and 300/400 series which we can't distinguish). Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55651 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ich_descriptors.c 1 file changed, 7 insertions(+), 11 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index 10355d9..aba7528 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -933,15 +933,13 @@ return CHIPSET_ICH9; if (content->ISL <= 10) return CHIPSET_ICH10; - if (content->ISL <= 16) - return CHIPSET_5_SERIES_IBEX_PEAK; if (content->FLMAP2 == 0) { - if (content->ISL == 23) - return CHIPSET_GEMINI_LAKE; - warn_peculiar_desc(content->ISL != 19, "Apollo Lake"); - return CHIPSET_APOLLO_LAKE; + if (content->ISL == 19) + return CHIPSET_APOLLO_LAKE; + warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); + return CHIPSET_GEMINI_LAKE; } - warn_peculiar_desc(true, "Ibex Peak"); + warn_peculiar_desc(content->ISL != 16, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; } else if (upper->MDTBA == 0x00) { if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { @@ -949,10 +947,8 @@ return CHIPSET_BAYTRAIL; if (content->MSL <= 1 && content->ISL <= 18) return CHIPSET_6_SERIES_COUGAR_POINT; - if (content->MSL <= 1 && content->ISL <= 21) - return CHIPSET_8_SERIES_LYNX_POINT; - warn_peculiar_desc(true, "Wildcat Point"); - return CHIPSET_9_SERIES_WILDCAT_POINT; + warn_peculiar_desc(content->MSL != 1 || content->ISL != 21, "Lynx Point"); + return CHIPSET_8_SERIES_LYNX_POINT; } if (content->NM == 6) { warn_peculiar_desc(content->ICCRIBA > 0x34, "C620 series"); -- To view, visit https://review.coreboot.org/c/flashrom/+/55651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5a5fee870202173b3a9813b03ec261e8ee45155 Gerrit-Change-Number: 55651 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.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-MessageType: merged

1 0

Change in flashrom[master]: ich_descriptors: Don't base chipset detection on `freq_read`
by Nico Huber (Code Review) 30 Jun '21

30 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55650 ) Change subject: ich_descriptors: Don't base chipset detection on `freq_read` ...................................................................... ich_descriptors: Don't base chipset detection on `freq_read` Only warn if the `freq_read` setting looks odd but don't override our previous guess. The `freq_read` check was taken from `ifdtool` but seems less reliable than our own detection scheme. Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55650 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> --- M ich_descriptors.c 1 file changed, 3 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index dbc2da4..10355d9 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -984,7 +984,7 @@ case CHIPSET_400_SERIES_COMET_POINT: case CHIPSET_GEMINI_LAKE: /* `freq_read` was repurposed, so can't check on it any more. */ - return guess; + break; case CHIPSET_100_SERIES_SUNRISE_POINT: case CHIPSET_C620_SERIES_LEWISBURG: case CHIPSET_APOLLO_LAKE: @@ -993,19 +993,17 @@ "However, the read frequency isn't set to 17MHz (the only valid value).\n" "Please report this message, the output of `ich_descriptors_tool` for\n" "your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n"); - return CHIPSET_9_SERIES_WILDCAT_POINT; } - return guess; + break; default: if (component->modes.freq_read == 6) { msg_pwarn("\nThe flash descriptor has the read frequency set to 17MHz. However,\n" "it doesn't look like a Skylake/Sunrise Point compatible descriptor.\n" "Please report this message, the output of `ich_descriptors_tool` for\n" "your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n"); - return CHIPSET_100_SERIES_SUNRISE_POINT; } - return guess; } + return guess; } /* len is the length of dump in bytes */ -- To view, visit https://review.coreboot.org/c/flashrom/+/55650 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f Gerrit-Change-Number: 55650 Gerrit-PatchSet: 5 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.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-MessageType: merged

1 0

Change in flashrom[master]: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
by Nico Huber (Code Review) 30 Jun '21

30 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55647 ) Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA ...................................................................... ich_descriptors: Revise detection for chipsets w/ ICCRIBA Detection based on ICCRIBA and FMSBA became a little messy lately. However, there's a new static difference: Since 300 series (Cannon Point), there is an MDTBA field in FLUMAP1 that has always been 0 (reserved) before. Taking this into account, we can relax the checks on ICCRIBA. Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55647 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> --- M ich_descriptors.c M ich_descriptors.h 2 files changed, 26 insertions(+), 22 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Michał Żygowski: Looks good to me, approved diff --git a/ich_descriptors.c b/ich_descriptors.c index 21fc40b..dbc2da4 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -921,9 +921,10 @@ /* * Guesses a minimum chipset version based on the maximum number of - * soft straps per generation. + * soft straps per generation and presence of the MIP base (MDTBA). */ -static enum ich_chipset guess_ich_chipset_from_content(const struct ich_desc_content *const content) +static enum ich_chipset guess_ich_chipset_from_content(const struct ich_desc_content *const content, + const struct ich_desc_upper_map *const upper) { if (content->ICCRIBA == 0x00) { if (content->MSL == 0 && content->ISL <= 2) @@ -942,25 +943,26 @@ } warn_peculiar_desc(true, "Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; - } else if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { - if (content->MSL == 0 && content->ISL <= 17) - return CHIPSET_BAYTRAIL; - if (content->MSL <= 1 && content->ISL <= 18) - return CHIPSET_6_SERIES_COUGAR_POINT; - if (content->MSL <= 1 && content->ISL <= 21) - return CHIPSET_8_SERIES_LYNX_POINT; - warn_peculiar_desc(true, "Wildcat Point"); - return CHIPSET_9_SERIES_WILDCAT_POINT; - } else if (content->ICCRIBA < 0x34) { - if (content->NM == 6) + } else if (upper->MDTBA == 0x00) { + if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { + if (content->MSL == 0 && content->ISL <= 17) + return CHIPSET_BAYTRAIL; + if (content->MSL <= 1 && content->ISL <= 18) + return CHIPSET_6_SERIES_COUGAR_POINT; + if (content->MSL <= 1 && content->ISL <= 21) + return CHIPSET_8_SERIES_LYNX_POINT; + warn_peculiar_desc(true, "Wildcat Point"); + return CHIPSET_9_SERIES_WILDCAT_POINT; + } + if (content->NM == 6) { + warn_peculiar_desc(content->ICCRIBA > 0x34, "C620 series"); return CHIPSET_C620_SERIES_LEWISBURG; + } + warn_peculiar_desc(content->ICCRIBA != 0x31, "100 series"); return CHIPSET_100_SERIES_SUNRISE_POINT; - } else if (content->ICCRIBA == 0x34) { - if (content->NM == 6) - return CHIPSET_C620_SERIES_LEWISBURG; - return CHIPSET_300_SERIES_CANNON_POINT; } else { - msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); + if (content->ICCRIBA != 0x34) + msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); return CHIPSET_300_SERIES_CANNON_POINT; } } @@ -972,9 +974,10 @@ * tinction because of the dropped number of regions field (NR). */ static enum ich_chipset guess_ich_chipset(const struct ich_desc_content *const content, - const struct ich_desc_component *const component) + const struct ich_desc_component *const component, + const struct ich_desc_upper_map *const upper) { - const enum ich_chipset guess = guess_ich_chipset_from_content(content); + const enum ich_chipset guess = guess_ich_chipset_from_content(content, upper); switch (guess) { case CHIPSET_300_SERIES_CANNON_POINT: @@ -1055,7 +1058,7 @@ } if (*cs == CHIPSET_ICH_UNKNOWN) { - *cs = guess_ich_chipset(&desc->content, &desc->component); + *cs = guess_ich_chipset(&desc->content, &desc->component, &desc->upper); prettyprint_ich_chipset(*cs); } diff --git a/ich_descriptors.h b/ich_descriptors.h index 7683ed0..d00af63 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -521,7 +521,8 @@ struct { uint32_t VTBA :8, /* ME VSCC Table Base Address */ VTL :8, /* ME VSCC Table Length */ - :16; + :8, + MDTBA :8; /* MIP Descr. Table Base Addr. (new since Cannon Point/300) */ }; }; struct { -- To view, visit https://review.coreboot.org/c/flashrom/+/55647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Gerrit-Change-Number: 55647 Gerrit-PatchSet: 6 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.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-MessageType: merged

1 0

Change in flashrom[master]: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
by Michał Żygowski (Code Review) 30 Jun '21

30 Jun '21

Attention is currently required from: Nico Huber. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55647 ) Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: I guess we just need a rebase here to resolve conflicts -- To view, visit https://review.coreboot.org/c/flashrom/+/55647 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512 Gerrit-Change-Number: 55647 Gerrit-PatchSet: 4 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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-Comment-Date: Wed, 30 Jun 2021 12:19:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Nico Huber (Code Review) 30 Jun '21

30 Jun '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 5: Code-Review+2 (1 comment) Patchset: PS5: I wanted to say: let's not bikeshed, we won't need it for long. But then again, do we need it at all? All the candidates for this function will likely get a shutdown NULL pointer once the allocation moves into common code. No matter if we introduce default_shutdown() or not, right? -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 09:42:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dediprog: Init-shutdown test for dediprog
by Nico Huber (Code Review) 30 Jun '21

30 Jun '21

Attention is currently required from: Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55934 ) Change subject: dediprog: Init-shutdown test for dediprog ...................................................................... Patch Set 3: (2 comments) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/bca06c00_85043fb4 PS3, Line 34: /* Define libusb symbols to avoid dependency on libusb.h */ Now that I realized that we don't mock `libusb.h` for the code under test. I wonder why we should avoid it here? It doesn't hurt so I'm ok with it. Just wondering. (I guess it wouldn't be hard to build the tests without libusb, but that can still be done if anyone ever needs it.) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/55934/comment/3c357fa5_e8df9617 PS2, Line 52: */ > Done, it works! but I'm not sure I understand how it works... […] Oh, I missed earlier that you avoid the libusb dependency here while it's still needed for the tests to build (includes of the code under test). There are some explanations why this works. Here is why this is legal in C: * The typedef is somewhat transparent. You can repeat it whereever you want, as long as it's the same type. * Just saying `struct libusb_device_handle;` without a member list makes it an incomplete type. That's good enough to declare pointers as long as the compiler doesn't need to know the size of the type, i.e. we don't try to dereference anything. Strictly speaking, the type we use here is not `libusb_device_handle`, it is `pointer to libusb_device_handle`. * Naturally, if incomplete types are allowed, it must be allowed to mix them with the actual (complete) type in another compilation unit. Otherwise they would be rather useless. (NB. Incomplete types are a means of information hiding in C.) Here is my favorite explanation: It all doesn't matter ;) because the compiler only looks at compilation units individually. And the linker who stitches the compilation results of the various compilation units together doesn't know C and its types. It doesn't even have to know parameters. There's a call to a symbol, it replaces the symbol with an address. That's about it. To get a working program out of it, the compiler needs to know the size of the parameters, and the size needs to match across compilation units. All pointers have the same size in C which makes it work even if we'd use `int *` here. -- To view, visit https://review.coreboot.org/c/flashrom/+/55934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I38508dfb6d7c24d42522f22fcae0c5e410c5f7ea Gerrit-Change-Number: 55934 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 30 Jun 2021 08:53:21 +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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021