Hello Edward O'Callaghan, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33931 to look at the new patch set (#3). Change subject: flashchips.c: Sort file by vendor and model ...................................................................... flashchips.c: Sort file by vendor and model For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time. This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Post-merge we can explore options such as breaking apart flashchips.c into per-vendor files or directories. Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,510 insertions(+), 5,510 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 3 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 3: Code-Review-1 (1 comment) This doesn't seem to adhere to the currently defined order. I wouldn't mind to change that definition, though. Also, the post-merge plans are better discussed on the mailing list. There are other people that have plans too :) For instance during the OSF Hackathon in June [1], we discussed making it an easier to handle ASCII file, that can (optionally) be compiled into C. [1] http://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFsK... https://review.coreboot.org/#/c/33931/3/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/33931/3/flashchips.c@29 PS3, Line 29: Within families keep them in order of density. I suppose this is not true anymore after this patch? -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 3 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 02 Jul 2019 10:35:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 3:

Patch Set 3: Code-Review-1

(1 comment)

This doesn't seem to adhere to the currently defined order. I wouldn't mind to change that definition, though. Also, the post-merge plans are better discussed on the mailing list. There are other people that have plans too :) For instance during the OSF Hackathon in June [1], we discussed making it an easier to handle ASCII file, that can (optionally) be compiled into C.

[1] http://docs.google.com/document/d/18qKvEbfPszjsJJGJhwi8kRVDUG3GZkADzQSH6WFsK...

Hi Nico, Long time, hope you have been well? I actually had a similar idea of a mini DSL that describes chips and breaking up the configs into a per-vendor thing. It would be good to discuss that more as I should be back around more again these days! Alan worked on this in collaboration with me. I am trying to get rid of the Chromium flashrom fork and get all the extra chip (+chipset) support from that into upstream. Unfortunately they have diverged *significantly* [9+ years] so it has been a real battle. Alan was able to contrive a tool that allowed us to reconcile many of the differences in this specific file however there are some conditions to be able to reach the point of convergence, this obviously being one of them. More precisely, I think if we can come up with a ordering function that produces reproducible results of ordering that would also work well however this was Alan's best first go at getting a ordering that works enough to make progress merging the two flashchips.c implementations. Once we have all the chip definitions in one place again we can move forward with greater and better plans on a nicer representation. I hadn't seen that doc until now however I don't see a problem with this specific blocking that plan from happening, in fact this could help aid in that since the ordering and cleanups make the file consistent enough now to be parsed up into a data structure and written out to ASCII or whatever representation we decide to choice. Cheers, Edward. -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 3 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 02 Jul 2019 14:08:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Hello Edward O'Callaghan, David Hendricks, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/33931 to look at the new patch set (#5). Change subject: flashchips.c: Sort file by vendor and model ...................................................................... flashchips.c: Sort file by vendor and model For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time. This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/33931/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset

Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 5: (1 comment) https://review.coreboot.org/#/c/33931/3/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/33931/3/flashchips.c@29 PS3, Line 29: Within families keep them in order of density.

You're right. Thank you for pointing it out. I update the comment to match the new reality. Done

-- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 5 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 03 Jul 2019 05:29:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alan Green <avg@google.com> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 7: Code-Review+1 Hi Nico, are we good to go on this particular one? -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 7 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 04 Jul 2019 14:32:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 7:

Patch Set 7:

...

Patch Set 7: Code-Review+1

Hi Nico, are we good to go on this particular one?

Absolutely not. Did you review the 5k lines patch? I would do some testing on my own, probably add a few lines of C that prints the database to compare before/after the patch. Hi Nico,

Here are some notes about how this patch was made, and how I came to be confident that it only moves entries in the flashchips array rather than duplicating or omitting any. The patch was generated from python script that parses entries out of flashchips.c. The script is currently out for review at https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+.... It may or may not be submitted. I first tested that the script could read all the entries from flashchips.c and then rewrite the file without differences. I then used "rewrite --sort" to generate a version of flashchips.c with the entries sorted. I've run a number of checks on the resulting file to ensure there are no differences in the flashchips array apart from the order of the entries. The most comprehensive check involved a small awk script which constructs one line per entry in the flashchips array. I ran this script against the original flashchips.c and the sorted flashchips.c, to produce two files of 532 lines each: $ awk '/^.{/ {x=$0; inside = 1} {if (inside) {x=x $0}} /^.}/ {inside = 0; print x}' orig_flashchips.c > orig_entries $ awk '/^.{/ {x=$0; inside = 1} {if (inside) {x=x $0}} /^.}/ {inside = 0; print x}' sorted_flashchips.c > sorted_entries $ wc -l {orig,sorted}_entries 532 orig_entries 532 sorted_entries 1064 total 532 is the expected number. flashrom -L reports 519 chips, and there are also 11 entries for chips with known vendors but unknown names, 1 entry for "Unknown" "SFDP-capable chip" and 1 entry for "Programmer" "Opaque flash chip". (flashrom -L's counting takes place in print.c in print_supported_chips().) By sorting and diffing each of {orig,sorted}_entries files, I was able to verify that for every line in the orig_entries file there was an identical line in the sorted_entries file. This shows that the entries in the before and after versions of flashchips.c are textually identical, apart from their order.

It's good that you ask btw. Your sudden appearance in the flashrom developers group in Gerrit already caused some friction. I don't know if any other member knows about that? I know that you are up to the task, it just caught me by surprise. And technically, with zero contributions to flashrom proper, you wouldn't even be in a reviewer group if we had one separate from coreboot's ;)

-- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 7 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 05 Jul 2019 04:22:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... flashchips.c: Sort file by vendor and model For self-consistency, and to allow tools to assist with merging the chromium fork of flashrom, sort the entries of flashchips.c. The file is already largely sorted, though deviations have crept in over time. This is a non-clever mostly ASCII-order sorting. It is not intended to be permanent. Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Reviewed-on: https://review.coreboot.org/c/flashrom/+/33931 Reviewed-by: Edward O'Callaghan <quasisec@chromium.org> Reviewed-by: Nico Huber <nico.h@gmx.de> Tested-by: build bot (Jenkins) <no-reply@coreboot.org> --- M flashchips.c 1 file changed, 5,515 insertions(+), 5,512 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, but someone else must approve -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 8 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged

Alan Green has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33931 ) Change subject: flashchips.c: Sort file by vendor and model ...................................................................... Patch Set 8:

Patch Set 8:

Oops, this introduced a bug: the SFDP entry is no longer at the end of flashchips.c, so probing on a SFDP-capable Winbond chip results in added noise (flashrom says things about an unknown chip, and then has two definitions for the same chip).

It's trivial to fix, but it might affect the codebase merging.

Oops indeed. I'll send a patch to put it back at the end. -- To view, visit https://review.coreboot.org/c/flashrom/+/33931 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I75a99583592526f60ba5264e92391bf8b1213b20 Gerrit-Change-Number: 33931 Gerrit-PatchSet: 8 Gerrit-Owner: Alan Green <avg@google.com> Gerrit-Reviewer: Alan Green <avg@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Angel Pons <th3fanbus@gmail.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Thu, 22 Aug 2019 23:59:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment