September 2021 - flashrom-gerrit

Change in flashrom[master]: Add Tiger Lake U Premium support
by Michał Żygowski (Code Review) 20 Sep '21

20 Sep '21

Attention is currently required from: Michał Żygowski, Paul Menzel. Hello build bot (Jenkins), Tim Crawford, Nico Huber, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55578 to look at the new patch set (#12). Change subject: Add Tiger Lake U Premium support ...................................................................... Add Tiger Lake U Premium support Tiger Lake has very low ICCRIBA (TGL=0x11, CNL=0x34 and CML=0x34) and detects as unknown chipset compatible with 300 series chipset. Add a new enum CHIPSET_500_SERIES_TIGER_POINT and treat it identically to CHIPSET_400_SERIES_COMET_POINT. There are some exceptions though, ICCRIBA is no longer present n descriptor content so a new union has been defined for new fields and used in descriptor guessing. freq_read field is not present on Tiger Lake, moreover in CannonPoint and Comet Point this field is used as eSPI/EC frequency, so a new function to print read frequency has ben added. Finally Tiger lake boot straps include eSPI, so a new bus has been added for the new straps. TEST=Flash BIOS region on Intel i5-1135G7 Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd --- M chipset_enable.c M ich_descriptors.c M ich_descriptors.h M ichspi.c M programmer.h M util/ich_descriptors_tool/ich_descriptors_tool.c 6 files changed, 93 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/55578/12 -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 12 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Michał Żygowski (Code Review) 20 Sep '21

20 Sep '21

Attention is currently required from: Nico Huber, Angel Pons, Michael Niewöhner, Sergii Dmytruk. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 ) Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... Patch Set 9: (1 comment) File acpi_ec.h: https://review.coreboot.org/c/flashrom/+/55714/comment/0a8a0fef_ab24eb40 PS1, Line 41: bool ec_wait_for_ibuf(uint8_t control_port); : bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks); > Unexported EC_MAX_STATUS_CHECKS and replaced all timeouts with zero where appropriate. […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 9 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> 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: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Mon, 20 Sep 2021 08:20:45 +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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Anastasia Klimchuk (Code Review) 20 Sep '21

20 Sep '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57437 ) Change subject: tests: Revise mock chip definition and usage ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/57437/comment/c1900fdb_96d8f5ba PS4, Line 24: fresh copy on the stack. > This made me chuckle a little :) What I always tell people when […] Nico I appreciate your reviews so much! I know it's a big effort to review, it takes time and energy. And I produce heaps of stuff for review. Thank you! File tests/chip.c: https://review.coreboot.org/c/flashrom/+/57437/comment/e013f650_a312f468 PS3, Line 128: static struct flashchip chip_8MiB = { > Looks good. […] Good to know! I just wasn't sure. wp is write-protect, I think. yes I think I can ignore it for now. -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 Gerrit-PatchSet: 4 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-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 20 Sep 2021 00:54:56 +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]: Add support for Intel Emmitsburg PCH
by Nico Huber (Code Review) 19 Sep '21

19 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, David Hendricks, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57635 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/57635/comment/047f7148_346a5080 PS3, Line 952: Ibex Peak is historically used as the default > > I wouldn't say "should" but it definitely could. > > Exactly, and that's why we're in this huge debate ;-) This ambiguity of intentions is exactly why we should match values (or ranges) to a specific chipset whenever possible, and document why the default value was chosen, especially if it's not the newest platform anymore. David, it's not about intentions at all. It's about what silicon vendors will provide (in the future) and what will match the situation best. Hence it will always be a "could", because it depends on decisions made outside of this project. > > > > > Personally, as this branch seems complex enough already, I'd > > first check if there is no easier way to distinguish server > > descriptors from client ones. > > I actually did spend some time looking at this, and unfortunately there does not appear to be a reliable way. Intel does not version the IFD (AFAIK), and for the most part the layout is similar. > > The most obvious difference I saw is that server chips tend to have a large ISL value (more straps) compared to non-server chips. > > However, the SPI IP block seems common among client and server chips. We even call the chip enable function `enable_flash_pch100_or_c620()`. > > > > > Also, please note that everything done about Emmitsburg so far > > (including what I wrote above) makes the assumption that it's > > right to return _LEWISBURG for it. I don't know if that is > > correct. > > Yes, LBG and EBG are very similar for flashrom's purposes just like Lynx Point and Wildcat Point. Read, write, and erase using layouts has been done a lot at this point. Well, see my other comment on the original patch. At least wrt. to the code in ichspi.c, EBG seems more closer to the newer client chips. I can't say if that is also the case descriptor-wise. But maybe, to correctly reflect the hardware situation, it might even be better to add a new enum value. I don't know if. But if you really care about maintainability as you just stated on the normalization patch, you should look into it, IMHO. Flashrom is much easier to maintain when the patch that adds support for a new platform actually matches the hardware. -- To view, visit https://review.coreboot.org/c/flashrom/+/57635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Gerrit-Change-Number: 57635 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Sun, 19 Sep 2021 10:03:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add Tiger Lake U Premium support
by Nico Huber (Code Review) 19 Sep '21

19 Sep '21

Attention is currently required from: Michał Żygowski, Paul Menzel. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 ) Change subject: Add Tiger Lake U Premium support ...................................................................... Patch Set 11: (2 comments) Patchset: PS11: Sorry, meant to comment this earlier. I guess if we don't touch `enum chipbustype` now, it's ready? File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/55578/comment/a292eb7c_6656faaf PS3, Line 659: { "eSPI", BUS_LPC | BUS_FWH } }; > https://downloadmirror.intel.com/27055/eng/329957-001_eSPI_Spec_Server_Adde… […] I would simply put `{ "eSPI", 0 }` here. "eSPI" is what the datasheet says, so why shouldn't flashrom print it. There is nothing we want to do right now wrt. the return value in this case, hence the `0`. This way, we wouldn't have to touch `enum chipbustype` and could avoid any implications there. -- To view, visit https://review.coreboot.org/c/flashrom/+/55578 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd Gerrit-Change-Number: 55578 Gerrit-PatchSet: 11 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Tim Crawford <tcrawford(a)system76.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Sun, 19 Sep 2021 09:21:09 +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]: ich_descriptors: Normalize chipset detection
by Nico Huber (Code Review) 19 Sep '21

19 Sep '21

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... Patch Set 5: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55651/comment/dd079a64_cad8dfd7 PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK; > > I see the same side effects and conclude that for these numbers > > this change does not add assumptions. > > You didn't factor in the addition of a new chipset with a large ISL value. That was the whole point. Neither did I factor in the addition of a new chipset with a low ISL value or one without an ISL field. I'm not sure if the list of things I didn't factor in is finite. Hence, I'm not taking any of it into account. The purpose of this patch was not to predict the future. > > > I'm going to assume that what you call "the default case" is the > > return statement on line 943 (after this change). In line 942, we > > check `content->ISL != 16` always before the default case. So the > > information you are missing is still there, it's just not in the > > current state of the code inside an if statement. > > The check you are referring to is for a warning that just tells us that the descriptor was not recognized. It does not enforce anything, unlike the original code which put Ibex Peak detection in an if-statement to enforce proper behavior when Ibex Peak is detected while making the intentions for default behavior obvious. Well, beside the "It does not enforce anything" part, you are right. You just put it in different words. > > > NB. The purpose of the code is to make flashrom work, not to provide > > a manual for future development. > > Code needs to be maintainable. If it's not obvious how to do future development with this code then that is a problem that we should definitely fix. You are assuming that there is a fix. We can't predict the future. We can make educated guesses, though. For instance, when I wrote this patch, I would not have guessed that a server platform will land on this ICCRIBA == 0 branch. Trying to predict such details can lead to walking in circles patch wise. For instance, I assume that it's easier to follow the code when it's more condensed, like after this patch. Before the entire patch train it was a forest of if's and nobody knew where to put anything. We should avoid that this happens again. Anyway, I already agreed that you should try to go ahead days ago. So I don't understand why you have to keep arguing and wasting everybody's time. > > Unless there is an actual bug that is fixed with this patch we should revert it. The original code is more explicit in each of its codepaths which makes it easier for future developers to avoid problems. Only one thing I have to ask you: As the purpose of this patch was to normalize, please do so with explicit if's for all chipsets then. warn_peculiar_desc() probably shouldn't have to check anything then. -- 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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Sun, 19 Sep 2021 09:10:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: ich_descriptors: Normalize chipset detection
by David Hendricks (Code Review) 19 Sep '21

19 Sep '21

Attention is currently required from: Nico Huber. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... Patch Set 5: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55651/comment/d1f17932_93b6f27e PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK; > I see the same side effects and conclude that for these numbers > this change does not add assumptions. You didn't factor in the addition of a new chipset with a large ISL value. That was the whole point. > I'm going to assume that what you call "the default case" is the > return statement on line 943 (after this change). In line 942, we > check `content->ISL != 16` always before the default case. So the > information you are missing is still there, it's just not in the > current state of the code inside an if statement. The check you are referring to is for a warning that just tells us that the descriptor was not recognized. It does not enforce anything, unlike the original code which put Ibex Peak detection in an if-statement to enforce proper behavior when Ibex Peak is detected while making the intentions for default behavior obvious. > NB. The purpose of the code is to make flashrom work, not to provide > a manual for future development. Code needs to be maintainable. If it's not obvious how to do future development with this code then that is a problem that we should definitely fix. Unless there is an actual bug that is fixed with this patch we should revert it. The original code is more explicit in each of its codepaths which makes it easier for future developers to avoid problems. -- 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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Sun, 19 Sep 2021 06:08:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for Intel Emmitsburg PCH
by David Hendricks (Code Review) 19 Sep '21

19 Sep '21

Attention is currently required from: Arthur Heymans, Nico Huber, Jonathan Zhang, David Hendricks, Edward O'Callaghan, Angel Pons. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57635 ) Change subject: Add support for Intel Emmitsburg PCH ...................................................................... Patch Set 3: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/57635/comment/6437a28a_d4021088 PS3, Line 952: Ibex Peak is historically used as the default > I wouldn't say "should" but it definitely could. Exactly, and that's why we're in this huge debate ;-) This ambiguity of intentions is exactly why we should match values (or ranges) to a specific chipset whenever possible, and document why the default value was chosen, especially if it's not the newest platform anymore. > > Personally, as this branch seems complex enough already, I'd > first check if there is no easier way to distinguish server > descriptors from client ones. I actually did spend some time looking at this, and unfortunately there does not appear to be a reliable way. Intel does not version the IFD (AFAIK), and for the most part the layout is similar. The most obvious difference I saw is that server chips tend to have a large ISL value (more straps) compared to non-server chips. However, the SPI IP block seems common among client and server chips. We even call the chip enable function `enable_flash_pch100_or_c620()`. > > Also, please note that everything done about Emmitsburg so far > (including what I wrote above) makes the assumption that it's > right to return _LEWISBURG for it. I don't know if that is > correct. Yes, LBG and EBG are very similar for flashrom's purposes just like Lynx Point and Wildcat Point. Read, write, and erase using layouts has been done a lot at this point. -- To view, visit https://review.coreboot.org/c/flashrom/+/57635 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1edae9163b910c3831adc82c90840fa965479451 Gerrit-Change-Number: 57635 Gerrit-PatchSet: 3 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: David Hendricks <david.hendricks(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: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Sun, 19 Sep 2021 05:24:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Revert "Add support for Intel Emmitsburg PCH"
by Nico Huber (Code Review) 18 Sep '21

18 Sep '21

Attention is currently required from: Arthur Heymans, Jonathan Zhang, Johnny Lin, David Hendricks, Stefan Reinauer, Christian Walter, Deomid "rojer" Ryabkov, Tim Chu. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/57589 ) Change subject: Revert "Add support for Intel Emmitsburg PCH" ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: Hey Stefan, I thought it's time to open a new thread for this. There's just so much to write it seems. > > > From an outside perspective without having skin in the game of this specific issue: Forking is not always bad. If there are two opposing trains of thought that can not be unified properly, or if there is a lack in will to make that happen, allowing everybody to move on and make their own version better for themselves and their customer base can even help to overcome differences in the long run (see egcs vs gcc a long time ago). > > I agree and this is why I bring it up from time to time without meaning > > it as a threat. > At some point it is better to try this out and see if it is what you think it is. > For your own peace and that of everybody else. I for one would be excited to see different directions take place at the same time. That's much more energizing than yet another community where people don't get along. Now I'm completely confused. In this project, flashrom, I'm not aware of any doubt about my involvement until we started to try to bring a fork back into the fold. That this effort was started shows to me that people were not very excited to see different directions. So, the exact opposite of what you just wrote. Can you elaborate on that thought? Do you think it's a bad idea to merge the chromium fork? > > > Nico, you are unhappy with about everybody trying to work with you here, > > "everybody": About 1 out of 10 people and only 1 in total long-term. > Admittedly I am bad at keeping lists. It seemed to have been everybody that I have worked with, at some point, but that may very well only be a small portion of the flashrom contributors. I apologize if I managed to become your long-term foe. No-no-no, not my foe at all! :) This wasn't about you and I don't see anybody as mine generally. And I hope of course that I'm nobodies foe, but I don't know that. My only long-term unhappiness is about trust issues I have with Edward's work. Lost trust takes time to build again, I wouldn't be surprised if it takes a decade in this case. About the "list". Are you sure you mean it as stated "Nico, you are unhappy..."? Or do you mean that everybody is unhappy with me? Because I could only name three to four people in the whole coreboot community that I've really been unhappy about. And if everybody that you have worked with was unhappy with me, that might be good to know for me. > > > and you are offering to fork the project fairly frequently. > > About once per year since Google tried to converge their fork with brute force. > Look, "Google" is not trying to do anything with brute force here. David, for one (who has not been at Google for many years), never really bothered if upstream would like the code contributions that he managed to integrate from the Chrome OS team. He was criticized for letting things diverge, and so another person came along, and another, and none of them were seemingly able to make much progress here, even decade long contributors to coreboot were in this list. That is not a sign of agressive convergence or brute force. Argh, sorry for putting it like this without explanation. Stefan, I honestly appreciate that you keep talking to me even when I badmouth things. It was meant roughly about the last 2.5 years. Back when things diverged, I wasn't even involved with flashrom. I guess I just started to work on coreboot at that time. It was during these last 2.5 years, that upstream flashrom was con- fronted with commits that were a mere diff of a part of the master branches of upstream and chromium, with made up commit messages. This is what I call brute force. Once such a commit is merged, it deceives anybody who wants to look up the reasons for the current code state. And this stays in the Git history. If the committers did this on there own or if they were tasked by Google with upstrea- ming, I don't know, I admit it. But at that time I assumed the latter. Please Stefan, imagine for a moment a commit that is actually a revert of a fix but the commit message says it makes things better. I remember one of such commits that made it in and one that didn't. But not knowing how many made it in is a huge issue. Ah, one more thing that I consider brute force; this was not Google's doing, though: Edward got submit rights to the repository without any prior involvement with the upstream project. Without saying hello even. > Now, Edward came along, and Edward is an assertive guy that follows up and follows through, and that can be both a blessing and a curse. I myself have been a complete jerk to Edward when he tried to merge LLVM support into coreboot at the time many years ago (and I never appropriately apologized), because I felt that he was diverging the direction of the project away from the stable construct that we had at the time to something that is too in love with the tools and programming languages used. I felt at times the same about Rust, go, and Ada. Today I feel that I would have been better off seeing Edward's work as the investment that it was, instead of a destructive intrusion. Luckily by the time Ada came around, I had already learned a lesson there. > > > It is unhealthy for you and everybody else involved in this project to keep the threat of breakup over everybody's head. I would encourage you to think about whether that is what you want and if so, I will gladly help you with resources (hosting, etc) to get your fork started and off the ground. I will also gladly pay for a domain for you for the first couple of years. > > That is a nice offer. But what is holding me back is not the lack of > > resources, but the people who ask me to continue and get another > > release done eventually. > You can of course do another release also from a new project. Life does not stop at any single moment but the last one. I hope you will be releasing software for decades to come, no matter what the project. My understanding was also that you have officially resigned from the flashrom project a while ago? Yep, I resigned. I've started to look into it again after a year. Right now I'm trying to keep it to things that nobody else does. I guess that's what we all do equally, as the project has no lead. You said "new project" above. You may not expect this, but if I'd fork flashrom, I'd do it to continue the project and I'd also ask you for the domain name oO > > > We have all started out as friends in this project, and it is good to remember that in the end we share 95% of the same goals, even if we like to fight to the last breath over the remaining 5%. Let's not use those 5% disagreement as a reason to create an environment in which none of us wants to be. It's not worth anybody's life time. > > This is one of the misunderstandings in this project. We don't share 95% > > of the same goals. And I don't think it's wise to ignore that. On one hand, > > there is flashrom, the thing that is packaged in long-term stable OS distri- > > butions that is used by humble users and needs to be 100% reliable so they > > don't brick their machine (or at least reliable enough so we are not buried > > under support requests). On the other hand, there is flashrom, a development > > tool. For most developers flashrom is the latter, I think. > > Please don't ignore diversity. > As grown-up people we can often hold conflicting goals in mind at the same time. I would still hold up that 95% of our developers are interested in creating a tool that is both capable of stable OS distribution as well as use as a development tool. The degree to which one lets the project encompass both of those goals and juggles the objectives successfully is leadership. That's why I quit back then. I'd like to think David and I did a good job at that and that I also did well when David spent less time with the project. But then what I called brute force happened and it was impossible to keep the stable goal in sight without having to argue about it constantly. You were involved too, AFAIR, you showed up on Gerrit in one of such arguments and I don't think you were juggling with more than one objective at that time. > I don't think that I ignore diversity, although your line of thought is compelling there and I would like to know more about how you get to that conclusion. Because you said 5% disagreement. That sounds like something one could ignore. If one assumes that little disagreement and maybe even communicates it to their superiors, it's more likely that people are caught by surprise when things escalate. People won't schedule time to settle disputes for 5%. You said these 5% should not be used to create a bad environment. But that doesn't mean we can ignore them. Also what should one leader do if developers do exactly that, create a bad environment and then the leader of the neighbour project backs them up doing so? Now that I re-read your original 5% comment, I think that is exactly what people did when trying to upstream things from the chromium fork. > > > I am open to suggestions but this way of treating each other needs to stop. > > Ok, then we need to talk about how people are treated. Please, Stefan, advice > > us what we can do better. > I think - and this goes for all of us - that we will be better off putting us into the other person's shoes before alleging that they are only out to hurt us, Hmmm, that's a good idea. I tried, though maybe not with the best strategy. I asked David how it is possible to move on here without being forced to work on his patches. But I didn't get an answer. So I assumed it's all intended. Maybe it wasn't. What would you suggest how I figure it out? > or even worse "the project" in a religious assumption that only one side can determine what is good for "the project". Well, call me a zealot, but I'm open for the idea to call forged commit messages and forcing people to work as not good for any project like this. > > I've been working my ass off last sunday for the project. Partially trying to > > identify regressions and to fix them. I tried to revert one change that was > > logically wrong and introduced a regression. I got a -2 instantly which was > > (allegedly) only set to give priority to the author's own unwritten patches. There's a typo there /o\ I meant the "reviewer's own patches", author of the -2 kind of. Sorry if this caused confusion. > I really appreciate that you are working hard to accomplish a more stable flashrom. Thanks! > While aggravating, a -2 is not a personal attack, I didn't see it like this and never meant to make the impression. I believe there are situations where a -2 is justified and situations where it's bad for everyone. When it's used to force people to work, I think that's in the category of "this way of treating each other needs to stop". > and in this case I am pretty sure that it was an attempt to spark a conversation. I am fairly certain that I have seen similar behavior from you in the past (and I'm sure the other side was upset too ;). Can you give an example? Well, obviously there was the -2 I gave in reaction to the one I got here. But was there a case where I gave one without advice or an alternative solution? Is there a way to search Gerrit for this? Would really help me to reflect about it. > Nico, I also don't think that your code or reviews are the only ones that are infallible or that your work is worth significantly more than those of other folks in the community - and that applies as well to Edward and David and the others that are trying to create a working solution for Emmitsburg here. > > Do you already see anything there that needs to stop? Or shall I continue to > > describe what happened? > Trying to figure out your nuance here, but I'm listening. Well, I have to admit this, giving priority to ones own patches, was my strongest point and the question was rhetorical. There is a lot of hidden work for reviewers on David's path and I felt forced to do it simply to fix a regression. David keeps calling things simple, but evidently that alone doesn't get things done. Now I decided to just not do any work for him. If somebody else does it, we'll see if it gets master to a sound state again; a gamble. If not, master stays broken for the moment I guess. -- To view, visit https://review.coreboot.org/c/flashrom/+/57589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4ebb7a931cfa66276df2f762c63e6d092d6b3d5a Gerrit-Change-Number: 57589 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: David Hendricks Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: David Hendricks Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Sat, 18 Sep 2021 17:08:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

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

18 Sep '21

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 ) Change subject: ich_descriptors: Normalize chipset detection ...................................................................... Patch Set 5: (1 comment) File ich_descriptors.c: https://review.coreboot.org/c/flashrom/+/55651/comment/3dc76f6e_24c6be6a PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK; Ok, your number example is FLMAP2 == 0 and ISL == 80. But your rea- soning seems to assume FLMAP2 != 0, so I'm going to assume FLMAP2 == 1 for this example, maybe that helps. We can also derive ICCRIBA == 0 because this comment thread is on that path. So before this change: ICCRIBA = 0, FLMAP2 = 1, ISL = 80 if on line 929 => true if on line 930 => false if on line 932 => false if on line 934 => false if on line 936 => false if on line 938 => false warning on line 944 => printed return on line 945 CHIPSET_5_SERIES_IBEX_PEAK and after this change: ICCRIBA = 0, FLMAP2 = 1, ISL = 80 if on line 929 => true if on line 930 => false if on line 932 => false if on line 934 => false if on line 936 => false warning on line 942 => printed return on line 943 CHIPSET_5_SERIES_IBEX_PEAK I see the same side effects and conclude that for these numbers this change does not add assumptions. > With this patch (CB:55651) applied it's unclear if the intent of the default case is to positively detect Ibex Peak, or to simply return Ibex Peak as a reasonable default when nothing else is explicitly detected. I'm going to assume that what you call "the default case" is the return statement on line 943 (after this change). In line 942, we check `content->ISL != 16` always before the default case. So the information you are missing is still there, it's just not in the current state of the code inside an if statement. NB. The purpose of the code is to make flashrom work, not to provide a manual for future development. And it's open-source, it can be changed to anything reasonable. It's also possible to change the code to something unreasonable. What happened by accident. This thread here also provides a plausible explanation: A bad rebase on top of this change. But no matter what you pretend, I did not review CB:54965, so I don't know if that is a possibility. -- 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: David Hendricks <david.hendricks(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Sat, 18 Sep 2021 14:11:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2021