Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from flashrom to flashprog #1769

Merged
merged 22 commits into from
Oct 29, 2024
Merged

Conversation

tlaurion
Copy link
Collaborator

@tlaurion tlaurion commented Aug 31, 2024

Flashrom is unfit for a while:

  • No progress bar, leaving users in the dark, Heads gave up on his homemade progress bar because changes in flashrom verbose output, parsed for Heads to draw output constantly changes. Its not for Heads to maintain this. As of master, users left in the dark while erasing/writing to SPI. not cool
  • Bricking with newer flashrom version for internal flashing for nv41... Probably also buggy with newer platforms. Not sure what's up with flashrom
  • Commits missing in flashrom releases for WP on time of release
  • Getting things merged under flashrom is problematic for a while.

See #1546 for more details leading for this work.


Unknows @i-c-o-n:

  • xx20: hwseq automatic or still needed on command line? yes. Added. ok
  • talos II: linux_mtd working? ok.
  • legacy boards: internal flashing on --ifd specified bios region: ok? (deprecating soon legacy boards): deprecated, but should work.

Tested with amazing progress bar output:
PXL_20240902_002217653.MP.jpg

Needs double flashing: reflashing within Heads to see if flashprog is able to flash again the firmware you had

Quick test for other boards untested:

  • download master zip artifcat from circleci for board, this pr zip.
  • flash this pr zip
  • flash master zip back. If successful, report success.

@tlaurion tlaurion marked this pull request as draft August 31, 2024 13:48
@tlaurion tlaurion force-pushed the flashprog branch 4 times, most recently from af6e9ff to a4ef189 Compare September 1, 2024 22:38
@tlaurion tlaurion changed the title PoC WiP flashprog Switch from flashrom to flashprog Sep 1, 2024
@tlaurion tlaurion linked an issue Sep 1, 2024 that may be closed by this pull request
@tlaurion tlaurion marked this pull request as ready for review September 2, 2024 00:26
@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 2, 2024

@JonathonHall-Purism need review and testing on Librems.
Can be tested under #1773

@JonathonHall-Purism
Copy link
Collaborator

Wow! 😮 The progress bar shown in the video (in #1773) is extremely compelling to me. Thanks for this, I'm building the combined MR to test and will review now.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 5, 2024

@i-c-o-n comments in OP still needing feedback otherwise have to reach for community testing and more extensive testing from board owners.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 5, 2024

Wow! 😮 The progress bar shown in the video (in #1773) is extremely compelling to me. Thanks for this, I'm building the combined MR to test and will review now.

Quick reminder that CircleCi builds for quick testing. This #1773 was just a staging PR containing all the pending changes to ease testing to reduce delays in functional testing results :)

Copy link
Collaborator

@JonathonHall-Purism JonathonHall-Purism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work. The progress indicator is awesome. Thank you so much for this @tlaurion 🙇

I tested Librem L1UM, 13v2, 14, and 11 (representing our range of chipsets pretty well) and all work perfectly.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 6, 2024

Adding https://github.com/linuxboot/heads/blob/master/BOARD_TESTERS.md's needing testing to OP prior of merging this PR

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 6, 2024

Testing needed as per OP post:

@srgrint
Copy link
Contributor

srgrint commented Sep 7, 2024

Have tested x220.

Unfortunately, it needs

export CONFIG_FLASH_OPTIONS="flashprog memory --progress --programmer internal:ich_spi_mode=hwseq"

Otherwise I get:

flashprog is free software, get the source code at https://flashprog.org/
Calibrating delay loop... ОК.
coreboot table found at 0x7faf1000.
Found chipset "Intel QM67".
Enabling flash write... OK.
Found Macronix flash chip "MX25L6405" (8192 kB, SPI) on internal. Found Macronix flash chip "MX25L6405D" (8192 kB, SPI) on internal.
Found Macronix flash chip "MX25L6406E/MX25L6408E" (8192 kB, SPI) on internal. Found Macronix flash chip "MX25L6436E/MX25L6445E/MX25L6465E/MX25L6473E/MX25L6473F" (8192 kB, SPI) on internal.
Multiple flash chip definitions match the detected chip(s): "MX25L6405", "MX25L6405D", "MX25L6406E/MX25L6408E", "MX25L6436E/MX25L6445E/MX25L6465E/M
!!!!!
Please specify which chip definition to use with the c option.
/tmp/flash_gui/update_package/heads-x220-maximized-v0.2.0-2241-ge313c18-dirty.rom: Flash failed

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 7, 2024

Have tested x220.

Unfortunately, it needs

export CONFIG_FLASH_OPTIONS="flashprog memory --progress --programmer internal:ich_spi_mode=hwseq"

Otherwise I get:

flashprog is free software, get the source code at https://flashprog.org/ Calibrating delay loop... ОК. coreboot table found at 0x7faf1000. Found chipset "Intel QM67". Enabling flash write... OK. Found Macronix flash chip "MX25L6405" (8192 kB, SPI) on internal. Found Macronix flash chip "MX25L6405D" (8192 kB, SPI) on internal. Found Macronix flash chip "MX25L6406E/MX25L6408E" (8192 kB, SPI) on internal. Found Macronix flash chip "MX25L6436E/MX25L6445E/MX25L6465E/MX25L6473E/MX25L6473F" (8192 kB, SPI) on internal. Multiple flash chip definitions match the detected chip(s): "MX25L6405", "MX25L6405D", "MX25L6406E/MX25L6408E", "MX25L6436E/MX25L6445E/MX25L6465E/M !!!!! Please specify which chip definition to use with the c option. /tmp/flash_gui/update_package/heads-x220-maximized-v0.2.0-2241-ge313c18-dirty.rom: Flash failed

Thanks @srgrint will update PR ASAP, I will expect the same for all xx20.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 7, 2024

@srgrint bd98101 rebased on master with hwseq in! Can you post results?

endif

#Only enable AST1100 if requested per board configs
ifeq "$(CONFIG_FLASHPROG_AST1100)" "y"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch missing. Even i currently activated under server board and passed at make, doesn't o anything without https://github.com/linuxboot/heads/blob/master/patches/flashrom-b1f858f65b2abd276542650d8cb9e382da258967/0100-enable-kgpe-d16.patch missing

@i-c-o-n?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can have a look at it. AFAIR, nobody made a real effort to upstream this. If I rebase this, we'd still need somebody to test it thoroughly. We are quite pedantic when it comes to internal flashing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@i-c-o-n it is a problem of upstreaming indeed, keeping track of patches downstream never proved to do any good.

If not under flashprog/flashrom: this means end users need to flash bmc externally which is not a desirable thing. Also as you may know, that bmc port is old but still in charge of best fan control out there for the d16. Having heads flash through https://github.com/linuxboot/heads/pull/1769/files#diff-08a8a9080a4ca4377e0de4dbef6bd94c84209343b4e12a8982352a1c1b09b71b would be useful.

That would be ideal, but not a blocker for this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@i-c-o-n do you see anything else that should be improved in the flashprog makefile for size/functionalities inclusion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see anything that should be added in board config for flash options that should be passed? Something that would make speed of internal flashing faster?

Also not a requirement for this PR, but a nice thing to track in separate issue for improvements, let me know.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@i-c-o-n do you see anything else that should be improved in the flashprog makefile for size/functionalities inclusion?

No, the current selection is already the minimum AFAICT. Anything else would need some hacking on the code. One low hanging fruit: For targets that use Intel hwseq or linux_mtd, you could drop everything else from flashchips.c. Haven't tried, but it probably won't link the chip drivers then.

Do you see anything that should be added in board config for flash options that should be passed? Something that would make speed of internal flashing faster?

--noverify-all gives you a little reading-speed advantage. But assuming that you update most of the flash anyway, it won't make much of a difference.

Copy link
Collaborator Author

@tlaurion tlaurion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed unmaintained boards for hwseq, d16 bmc internal flashing must be wrong

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 8, 2024

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 8, 2024

Amazing work. The progress indicator is awesome. Thank you so much for this @tlaurion 🙇

I tested Librem L1UM, 13v2, 14, and 11 (representing our range of chipsets pretty well) and all work perfectly.

@i-c-o-n : thank you for your great work and maintainership of flashprog!!!

@srgrint
Copy link
Contributor

srgrint commented Sep 8, 2024

@srgrint bd98101 rebased on master with hwseq in! Can you post results?

Yes x220 works fine again with hwseq

By the way, I don't have access to my t440p for a few weeks. Hopefully one of your other testers will test haswell.

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 8, 2024

By the way, I don't have access to my t440p for a few weeks. Hopefully one of your other testers will test haswell.

Haswell board owners, time to shine promptly
@ThePlexus @akunterkontrolle @rbreslow
@resende-gustavo @gaspar-ilom

Otherwise they will land as untested boards in the next days.

@tlaurion
Copy link
Collaborator Author

I've just tested on w541 board, from the CircleCI job #53413, and everything is working fine. I apologize for the delay with the testing. I needed the board and was concerned about having enough time to address any issues if the changes didn't work.

I will make sure to manage and ensure that such delays do not occur again.

So this is one user (I think those 2 usernames are the same person, unsure).

We are not the same person.

@ResendeGHF 5cba23e commit message accurate?

@tlaurion
Copy link
Collaborator Author

@pietrushnic is Dasharo considering switching to flashprog?
Would need go/no go for this PR, all boards were tested working.

…c FLASH_OPTIONS instead, might cchange in the future.

Signed-off-by: Thierry Laurion <[email protected]>
@ResendeGHF
Copy link

@ResendeGHF 5cba23e commit message accurate?

Accurate.

@arhabd
Copy link
Contributor

arhabd commented Sep 11, 2024

Flashrom is unfit.

* No progress bar, leaving users in the dark, Heads gave up on his homemade progress bar because changes in flashrom verbose output, parsed for Heads to draw output constantly changes. Its not for Heads to maintain this. As of master, users left in the dark while erasing/writing to SPI. not cool

* Bricking with newer flashrom version for internal flashing for nv41... Probably also buggy with newer platforms. Not sure what's up with flashrom

* Commits missing in flashrom releases for WP on time of release

* Getting things merged under flashrom is problematic for a while.

See #1546 formore details leading for this work.

Unknows @i-c-o-n:

* xx20: ~hwseq automatic or still needed on command line?~ yes. Added.

* talos II: linux_mtd working? ok.

* ~legacy boards: internal flashing on --ifd specified bios region: ok? (deprecating soon legacy boards)~: ok.

Tested with amazing progress bar output: PXL_20240902_002217653.MP.jpg

Needs double flashing: reflashing within Heads to see if flashprog is able to flash again the firmware you had

* [x]  x230 : thanks @tlaurion

* [x]  nv41 : thanks @tlaurion

* [x]  one sandy result
  
  * [ ]  t420 (xx20): @alexmaloteaux @natterangell (iGPU) @akfhasodh @doob85
  * [x]  x220 (xx20): @Thrilleratplay @BlackMaria @srgrint : thanks @srgrint for testing once again [Switch from flashrom to flashprog #1769 (comment)](https://github.com/linuxboot/heads/pull/1769#issuecomment-2336744150)

* [x]  one haswell result
  
  * [x]  t440p: @ThePlexus @srgrint @akunterkontrolle @rbreslow : thanks to @fhvyhjriur [Switch from flashrom to flashprog #1769 (comment)](https://github.com/linuxboot/heads/pull/1769#issuecomment-2340479889)
  * [ ]  w541 (similar to t440p): @resende-gustavo @gaspar-ilom  : saved from becoming untested by @fhvyhjriur [Switch from flashrom to flashprog #1769 (comment)](https://github.com/linuxboot/heads/pull/1769#issuecomment-2341097328)

* [x]  talos-2 for linux_mtd : thankd @tlaurion

* [ ]  d16 (should work)
  
  * [ ]  kgpe-d16 (AMD fam15h) (dropped in coreboot 4.12): @Tonux599 @zifxify @arhabd

* [ ]  legacy-boards? If no answer, deprecate as part of this PR.

Quick test for other boards untested:

* download master zip artifcat from circleci for board, this pr zip.

* flash this pr zip

* flash master zip back. If successful, report success.

can confirm this works on kgpe-d16 and the #1778 patch works as intended aswell

@tlaurion
Copy link
Collaborator Author

Updated OP #1769 (comment)

Waiting for approval/comments/concerns.

Started a thread for vPub today: https://matrix.to/#/!HAonmNlisbeoADmnrN:matrix.org/$SmBpUcZDlvATbUruP1sMfV16gZjEDIwVmm6Yi98n5cw?via=matrix.org&via=kit.edu&via=invisiblethingslab.com

Starts in one hour, should talk on this after 14:00 EST.

@natterangell
Copy link
Contributor

Flashed heads-t420-hotp-maximized-v0.2.0-2343-gd5ddab5.zip from this PR and wrote back heads-t420-hotp-maximized-v0.2.0-2323-g3fef9e0.zip from master.

Works flawlessly!

PS! t420-hotp-maximized shows up as untested when flashing.

IMG_0023

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 12, 2024

Flashed heads-t420-hotp-maximized-v0.2.0-2343-gd5ddab5.zip from this PR and wrote back heads-t420-hotp-maximized-v0.2.0-2323-g3fef9e0.zip from master.

Works flawlessly!

IMG_0023

This is from old to new firmware flashing, from a firmware version still showing Heads internal flashing progress bar.
PXL_20240902_002217653.MP.jpg

The goal of this testing is double flashing, meaning you have to flash something FROM this PR. Flashing the same ROM is ok.

Please redo.

PS! t420-hotp-maximized shows up as untested when flashing.

Will check, meaning boardname is untested. With your update, can move to tested @natterangell

Output should look like OP's screenshot on second flash, from within that new firmware being flashed @natterangell .
Hopefully other testers reported success on this premise.

EDIT: you flashed from a rom that WAS tagging t420 as untested. :)

Copy link
Contributor

@gaspar-ilom gaspar-ilom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, for the late reply. I think you tagged me six days ago. I am always willing to test the W541, but I cannot guarantee to do it as promptly as per your requirement. It is your decision whether you want to keep me listed as a tester here.

For the record the last few rounds of board testing I did test the W541 eventually. Personally, I also do not mind if the board is moved to untested and I create a PR after I tested it like I did here: #1750

That said, I understand your frustration with slow testing holding the project back. You know best whether it is worth keeping the board in spite of slow testing or not.

@natterangell
Copy link
Contributor

EDIT: you flashed from a rom that WAS tagging t420 as untested. :)

Yes, that was it. I was on an old rom :)

Don’t worry, I enjoyed the improved progress bars flashing back to master 😎

@tlaurion
Copy link
Collaborator Author

tlaurion commented Sep 15, 2024

@JonathonHall-Purism do we merge and deal with ecosystem switching to flashprog /not switching/ reverting to flashrom later if needed?

Seems like we won't get feedback here. Feel free to merge.

@tlaurion
Copy link
Collaborator Author

Dasharo/dasharo-issues#1060 (comment)

Patch needed

@i-c-o-n
Copy link

i-c-o-n commented Sep 25, 2024

Dasharo/dasharo-issues#1060 (comment)

Patch needed

https://review.sourcearcade.org/c/flashprog/+/259

Should probably put more work into auto-detection... I'll try to get everything merged over the next weekend, then you can pull from the main branch 😄

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 22, 2024

This is where I don't know how to act. https://review.coreboot.org/c/flashrom/+/84102 is still unmerged while flashprog works for all platforms, producing needed progress output as well.

@macpijan @SergiiDmytruk what do we do? Blocker for #1753 as well.

@macpijan
Copy link
Contributor

macpijan commented Oct 23, 2024

We have used flashrom mostly so far, not flashprog, but if it works better for you, we do not mind.
Do you need us to test something with flashprog?

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 23, 2024

We have used flashrom mostly so far, not flashprog, but if it works better for you, we do not mind. Do you need us to test something with flashprog?

My personal preference for forks are for ones that are listening to downstream needs: here Heads is a downstream user of flashrom/flashprog that needs to be stable, functionally equivalent and provide proper UX not aimed at geeks but more and more non-technical end users under Heads umbrella.

@macpijan

Do you need us to test something with flashprog?

Not specifically: This PR proved its point where https://review.coreboot.org/c/flashrom/+/84102 and whole #1753 discussion with patches missing under flashrom, and versions not including some patches at time of release, releases breaking compatibility with older platforms and the lack of progress output consideration from upstream for 2+ years under https://ticket.coreboot.org/issues/390 are all reasons for me to want tot get away of flashrom ASAP if possible.

My question is more: what project do we want to use and promote. And then, if I switch to flashprog, does it generates a veto or not?


This is where I don't know how to act. https://review.coreboot.org/c/flashrom/+/84102 is still unmerged while flashprog works for all platforms, producing needed progress output as well.

I just need understanding of the situation, where @i-c-o-n clearly surpassed my expectations in the past and moves things forward and fast and stabler then flashrom. This is actually why the project was forked. Again not wanting to go political, but this is why projects fragments: right? When things don't advance as fast as expected downstream: this is why projects gets forked, amongst many other reasons of course. Eg: why Heads tends to rely on more forks of coreboot then upstream for recent platforms. I am reluctant to change from flashrom to flashprog for political reasons, not technical ones.

@macpijan If you are confortable, please approve PR. :)

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 23, 2024

TODO:

  • have confirmation with @i-c-o-n that the commit pointed under modules/flashprog is good for now/newer commit without regression expected for tested boards should be used (link to current review)
  • rebase on master
  • switch optiplex boards to flashprog, test
  • merge

@tlaurion
Copy link
Collaborator Author

tlaurion commented Oct 24, 2024

@macpijan change of mind since input from @SergiiDmytruk at #1753 (comment)? I really don't know what do do with this fragmented ecosystem and need advice, really.

@i-c-o-n
Copy link

i-c-o-n commented Oct 28, 2024

  • have confirmation with @i-c-o-n that the commit pointed under modules/flashprog is good for now/newer commit without regression expected for tested boards should be used (link to current review)

Current commit pointer looks good. I have plans to go over the patches on the wp_cli branch one more time this week and then merge to main. But I don't see a reason to wait for that.

@tlaurion tlaurion self-assigned this Oct 28, 2024
@tlaurion
Copy link
Collaborator Author

Tested on optiplex after rebasing on master: working.

@tlaurion tlaurion merged commit d128fa3 into linuxboot:master Oct 29, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider flashprog