April 2017 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: Add iMac5, 2 to board_enable
by Nico Huber (Code Review) 16 Apr '17

16 Apr '17

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/18943 ) Change subject: Add iMac5,2 to board_enable ...................................................................... Add iMac5,2 to board_enable When running coreboot everything works, while on vendor bios it fails to erase anything until the 3th erase function of a SST25VF016B is tried. Change-Id: I5e4daaa67f98a47e237cd73962776da369c8f0b5 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-on: https://review.coreboot.org/18943 Tested-by: build bot (Jenkins) Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M board_enable.c 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/board_enable.c b/board_enable.c index 2f0c1c0..c85bd15 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2325,6 +2325,7 @@ {0x1022, 0x2090, 0, 0, 0x1022, 0x2080, 0, 0, NULL, "artecgroup", "dbe62", P3, "Artec Group", "DBE62", 0, OK, board_artecgroup_dbe6x}, {0x8086, 0x27b9, 0xa0a0, 0x0632, 0x8086, 0x27da, 0xa0a0, 0x0632, NULL, NULL, NULL, P3, "AOpen", "i945GMx-VFX", 0, OK, intel_ich_gpio38_raise}, {0x8086, 0x277c, 0xa0a0, 0x060b, 0x8086, 0x27da, 0xa0a0, 0x060b, NULL, NULL, NULL, P3, "AOpen", "i975Xa-YDG", 0, OK, board_aopen_i975xa_ydg}, + {0x8086, 0x27A0, 0x8086, 0x7270, 0x8086, 0x27B9, 0x8086, 0x7270, "^iMac5,2$", NULL, NULL, P2, "Apple", "iMac5,2", 0, OK, p2_whitelist_laptop}, {0x8086, 0x27A0, 0x8086, 0x7270, 0x8086, 0x27B9, 0x8086, 0x7270, "^MacBook2,1$", NULL, NULL, P2, "Apple", "MacBook2,1", 0, OK, p2_whitelist_laptop}, {0x8086, 0x27b8, 0x1849, 0x27b8, 0x8086, 0x27da, 0x1849, 0x27da, "^ConRoeXFire-eSATA2", NULL, NULL, P3, "ASRock", "ConRoeXFire-eSATA2", 0, OK, intel_ich_gpio16_raise}, {0x1022, 0x1536, 0x1849, 0x1536, 0x1022, 0x780e, 0x1849, 0x780e, "^Kabini CRB$", NULL, NULL, P2, "ASRock", "IMB-A180(-H)", 0, OK, p2_not_a_laptop}, -- To view, visit https://review.coreboot.org/18943 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5e4daaa67f98a47e237cd73962776da369c8f0b5 Gerrit-PatchSet: 5 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins)

1 0

Change in flashrom[staging]: Add iMac5, 2 to board_enable
by Nico Huber (Code Review) 16 Apr '17

16 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18943 ) Change subject: Add iMac5,2 to board_enable ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/18943 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5e4daaa67f98a47e237cd73962776da369c8f0b5 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Free board_vendor and board_model vars before returning
by Nico Huber (Code Review) 16 Apr '17

16 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18207 ) Change subject: Free board_vendor and board_model vars before returning ...................................................................... Patch Set 3: (1 comment) > it seems like return 1 is the default except for a few cases. how > about initializing ret = 1, and drop the _1 label? Not that easy since `ret` might get overwritten in between. https://review.coreboot.org/#/c/18207/3/internal.c File internal.c: Line 163: #if defined __FLASHROM_LITTLE_ENDIAN__ Well, this is a problem... `ret` wouldn't be defined otherwise. Just remove the guard? -- To view, visit https://review.coreboot.org/18207 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I825f53702803292277b44dcb03fdc49b5dd410a8 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Add functions to read/erase/write/verify by layout
by David Hendricks (Code Review) 15 Apr '17

15 Apr '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/17945 ) Change subject: Add functions to read/erase/write/verify by layout ...................................................................... Patch Set 4: Code-Review+2 (3 comments) Better! I have a few cosmetic nits for you to consider, but overall I'd be fine with committing this as is. FWIW for testing I'm trying to bring in some patches to handle -i region[:<filename>] syntax (https://patchwork.coreboot.org/patch/4076/) and the new test script from chromium (https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/master/…). It sounds like you already did a fair amount of manual testing, too. https://review.coreboot.org/#/c/17945/4/flashrom.c File flashrom.c: PS4, Line 1764: size_t int to match num_entries? Line 1855: /* nit: add a newline above to separate the declarations and the body PS4, Line 1886: newc + start - info->erase_start Thanks for updating the offset calculation. However, it somehow didn't end up as clear as I had hoped, probably because "start" was added and so when re-reading this I had to mentally substitute "info->region_end + 1" for "start". How about creating another variable above, rel_start or something, to store "start - info->erase_start"? That makes it even more clear that we're adding a relative address to newc and also reduces arithmetic in the function calls (same rationale for creating "start" and "len" to begin with, I presume). -- To view, visit https://review.coreboot.org/17945 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic6194cea4c4c430e0cf9d586052508a865b09c86 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Add functions to read/erase/write/verify by layout
by build bot (Jenkins) (Code Review) 13 Apr '17

13 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/17945 ) Change subject: Add functions to read/erase/write/verify by layout ...................................................................... Patch Set 4: Build Failed https://qa.coreboot.org/job/flashrom_gerrit/137/ : FAILURE https://qa.coreboot.org/job/flashrom-customrules/100/ : SUCCESS -- To view, visit https://review.coreboot.org/17945 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic6194cea4c4c430e0cf9d586052508a865b09c86 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Add functions to read/erase/write/verify by layout
by Nico Huber (Code Review) 13 Apr '17

13 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/17945 ) Change subject: Add functions to read/erase/write/verify by layout ...................................................................... Patch Set 2: (3 comments) I'm not sure which path to go from here. I could leave it as is, in case tests are positive (I've fixed the curcontents patching in the latest patch set). Or try to perfect it (e.g. by making curcontents/newcontents completely transparent to the walk_* functions). But I guess the ugly parts would vanish anyway, if we tackle the erase function decision later. https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c: PS2, Line 1773: read_by_layout(flashctx, info->curcontents) > If we'd move this before the first try and read the current region Should move anyway, or else we'd read again after the last function failed. PS2, Line 1836: const unsigned int erase_len = info->erase_end + 1 - info->erase_start; > Are you sure info->erase_end and info->erase_start are valid here? The call We're called by walk_eraseblocks() which sets them... generally: something_by_layout() -- optionally sets curcontents/newcontents walk_by_layout() -- sets region_start/region_end walk_eraseblocks()-- sets erase_start/erase_end per_blockfn() -- does the job PS2, Line 1847: region_unaligned > I tried to patch the layout in advance in the chromiumos branch, but the re After reading your comment above, I too thought walk_eraseblocks() would be a better place for the patching, but... the "Note" in the comment here gives a hint that it would become more complex. We can't patch into info->newcontents without walking the layout again to decide which parts to patch. So I decided to introduce the `newc` buffer which only contains the changes that will be written for the current region. This is not the optimal solution but, IMO, the cleaner one: 1. the code is less complex. 2. we do things step by step. after walk_by_ layout() is done with one interation the flash chip is in the ob- vious state: we have updated the data for all regions up to the last iteration (without patching data for later regions in beforehand). 3. the caller of walk_by_layout() (who decides about the used per_ blockfn()) and the per_blockfn() share a contract how curcontents and newcontents are handled. this contract is transparent to walk_ by_layout() and walk_eraseblocks() (with the exception of the re- reading in the fallback case). If we'd keep the `newc` buffer but move the patching up into walk_ eraseblocks(), we'd have to mess with the per_blockfn() signature (adding a parameter for the newcontents). -- To view, visit https://review.coreboot.org/17945 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic6194cea4c4c430e0cf9d586052508a865b09c86 Gerrit-PatchSet: 2 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Add functions to read/erase/write/verify by layout
by David Hendricks (Code Review) 12 Apr '17

12 Apr '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/17945 ) Change subject: Add functions to read/erase/write/verify by layout ...................................................................... Patch Set 4: (5 comments) a few quick comments... I need to re-review this patch later on. https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c: PS2, Line 1647: cinfo("\nWarning: Chip content is ident > I'm tempted but I'd rather see less global variables than more. At SGTM. I'd also rename it to "default" or "default_layout" while we're at it, but that's your call. PS2, Line 1721: > could just break (both loops) in this case Actually, I think this will need to be expanded to patch the content buffers if eraseblocks[i].size doesn't align with the targeted region (more on that below). Not sure how it will look in the end, will think about it some more... PS2, Line 1836: * @param buffer Buffer of full chip size to read into. Are you sure info->erase_end and info->erase_start are valid here? The caller (walk_by_layout()) doesn't initialize them. PS2, Line 1847: c int read_erase > Ok, trying to sort this out. In case of `-A` (noverify-all) we I tried to patch the layout in advance in the chromiumos branch, but the result was kinda ugly and inflexible (https://chromium-review.googlesource.com/#/c/240176/). Unfortunately I never had time to go back and implement a solution that could work with different block sizes, so I ended up just assuming that the smallest size must work :-/ Like you say, it doesn't play well with the block-erase fallback. IIRC the issue was the way contents and offsets were passed around awkwardly (and inconsistently?) to the helper functions. Passing a walk_info struct all the way down the stack should help a lot. The innermost loop in walk_eraseblocks() can use that info to patch both curcontent and newcontent with the missing parts (patch newcontent for the update, curcontent for verification later on). Line 1929: ret = 0; The other members should probably be initialized to 0xdeadbeef (or something out of range of usual flash chips) to help debug when somebody re-factors all of this. -- To view, visit https://review.coreboot.org/17945 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic6194cea4c4c430e0cf9d586052508a865b09c86 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Add functions to read/erase/write/verify by layout
by Nico Huber (Code Review) 12 Apr '17

12 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/17945 ) Change subject: Add functions to read/erase/write/verify by layout ...................................................................... Patch Set 4: > Do you by chance have testing notes or a script handy? No. Most testing was done manually with patch set 1 (which made it into our products). The current revision is untested btw. -- To view, visit https://review.coreboot.org/17945 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic6194cea4c4c430e0cf9d586052508a865b09c86 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Enable writes with active ME
by build bot (Jenkins) (Code Review) 12 Apr '17

12 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/18740 ) Change subject: Enable writes with active ME ...................................................................... Patch Set 4: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/119/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/136/ : SUCCESS -- To view, visit https://review.coreboot.org/18740 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I94a5e7074b845c227e43d76d04dd1a71082a1cef Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Fix linking with libpayload
by build bot (Jenkins) (Code Review) 12 Apr '17

12 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/18738 ) Change subject: Fix linking with libpayload ...................................................................... Patch Set 4: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/117/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/134/ : SUCCESS -- To view, visit https://review.coreboot.org/18738 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I561135209b819361d125eeaeef9ff886d6bae987 Gerrit-PatchSet: 4 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2017