- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: Add a convenient libflashrom interface
by Nico Huber (Code Review) 12 Apr '17

12 Apr '17

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/17946 to look at the new patch set (#3). Change subject: Add a convenient libflashrom interface ...................................................................... Add a convenient libflashrom interface This adds a minimal libflashrom interface based on the draft in the wiki. While the glue code in libflashrom.c is build on top of the existing code instead on overhauling it, the interface in libflashrom.h is supposed to be stable. So we can keep the interface and adapt internals later if favoured, without breaking clients. A new make target, libinstall, is also added. It installs libflashrom.a and libflashrom.h in lib/ and include/ dirs respectively. This commit breaks build of the cli. It will be fixed with a follow-up commit. v2: Rebase and fixes by Anton Kochkov. v3: o fl_image_*() rewritten with layout support (touch only included regions). o Moved read/erase/write/verify operations to flashrom.c. o Added layout pointer and flags to the flash context. Change-Id: I00f169990830aa17b7dfae5eb74010d40c476181 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> --- M Makefile M flash.h M flashrom.c A libflashrom.c A libflashrom.h 5 files changed, 694 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/17946/3 -- To view, visit https://review.coreboot.org/17946 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00f169990830aa17b7dfae5eb74010d40c476181 Gerrit-PatchSet: 3 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins)

1 0

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

11 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: (1 comment) 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 only, it might simplify things... but not for the verify-all case :-/ -- 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 Nico Huber (Code Review) 11 Apr '17

11 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: (10 comments) Mostly commented for myself for the next iteration. Feel free to look at it anyway. https://review.coreboot.org/#/c/17945/2/flashrom.c File flashrom.c: PS2, Line 1647: struct fl_layout_single *const fallback > Since the fallback is always the entire chip, would it make sense to omit t I'm tempted but I'd rather see less global variables than more. At some point, somebody will ask why he can't use libflashrom with his weird programmer array in a multi-threaded application... and I can point a finger at others. :) We could make the fallback structure part of the flashctx though. How about that? PS2, Line 1672: void * should be `uint8_t *` for pointer arithmetic PS2, Line 1695: unsigned int wondering why I didn't use chipoff_t for consistency PS2, Line 1705: int size_t PS2, Line 1708: int size_t PS2, Line 1721: info->region_end < info->erase_start could just break (both loops) in this case PS2, Line 1759: ret rather `error` PS2, Line 1844: newcontents better `info->newcontents` PS2, Line 1847: region_unaligned > Has to update info->curcontents too for `-A` case. Ok, trying to sort this out. In case of `-A` (noverify-all) we don't read the full flash chip in advance but only included regions. So `info->curcontents` may be undefined for the parts outside the unaligned region. Reading per erase block might be slow, so we don't want to fill `curcontents` after selecting the block eraser but want to read as much as possible in advance. However, `curcontents` is always valid for the part that we want to change and `newcontents` is already patched correctly. So in the worst case, we'd unnecessarily erase the block but write the correct data. Solutions: * Write the above into a comment and ignore the unnecessary erase * Patch curcontents too: Copy what we just read into `newc` * Patch the layout used to read in advance to read complete erase blocks (wouldn't have to read() here then but could always rely on `curcontents`). Preferred solution but doesn't play with our block-eraser fallback. In any case: Document the contract explicitly, that `curcontents` either guarantees to contain the layout regions only or every touched eraseblock. PS2, Line 1862: newc - info->erase_start + info->region_end + 1 > Hmmm, somehow I think this can be made clearer to the reader. We're offsett ack -- 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]: Convert flashrom to git
by Nico Huber (Code Review) 11 Apr '17

11 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: Code-Review-1 (8 comments) I still object to the complex version generation. It's very far from being bullet-proof and just wastes time atm. Please split it out into another commit. A simple `<tag>-<count>-<sha>` would suffice, IMO. If `<sha>` is on an upstream branch, a developer can look that up easily. If it's not, anything could have been changed, no matter if the changes are based on a tag or a branch. It seems to be just user (and contributor) patronizing for the sake of a little upstream dev convenience. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh: PS1, Line 50: forced_update This is not about updating in general but updating the "upcache". The name should reflect that. PS1, Line 58: update This is not about updating in general but updating the "upcache". The name should reflect that. PS1, Line 224: -n "$up_remote" Just that there is a upstream remote registered doesn't imply that its branches are synchronized. PS1, Line 229: update So we never synchronize in the upstream-repo-registered case but always in the upcache case. That's scratchy. PS1, Line 224: if [ -n "$up_remote" ]; then : for b in $upstream_branches ; do : up_refs="$up_refs ${up_remote}/${b}" : done : else : update || { echo "offline" ; return ; } : up_refs=$upcache_refs : fi What we'd need here instead is * setting `up_refs` based on `-n "$up_remote"` (like already done) * check if all `$up_refs` exist * if not, update `$up_refs` whatever they are PS1, Line 257: for ref in $up_refs ; do : if git merge-base --is-ancestor ${merge_point} ${ref}; then : upstream_branch=${ref##*/} # remove everything till last / : cnt_tag2upstream_branch=$(git rev-list --count "${lasttag}..${merge_point}" 2>/dev/null) : break : fi : done Nit, looks like `staging` would override `stable` here even if they are equally close. PS1, Line 268: _tag2upstream_branch probably uninitialized PS1, Line 269: [ -n "$upstream_branch" ] always true on this path? -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.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]: Convert flashrom to git
by Nico Huber (Code Review) 10 Apr '17

10 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: (7 comments) Not quite through... https://review.coreboot.org/#/c/19206/1//COMMIT_MSG Commit Message: PS1, Line 84: Acked-by: Stefan Tauner <stefan.tauner(a)alumni.tuwien.ac.at> I'd drop this since it's not this revision he acked. https://review.coreboot.org/#/c/19206/1/Makefile File Makefile: PS1, Line 1357: "$(MAN_DATE)" This overwrites the default system-management heading (center of first line). Why? it's also not mentioned in the commit message. https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh: PS1, Line 41: upcache_refs uninitialized PS1, Line 44: # We need to update our upstream information sometimes so that we can create detailed version information. : # To that end the code below fetches parts of the upstream repository via https. : # This takes about one second or less under normal circumstances. Do we break lines only at full stops now? This looks like a single paragraph to me. Everything wider than about 70 chars limits reada- bility. Applies to many comments in this file. Line 66: upstream repositories as git remote to rely on that information. Is here supposed to start a new paragraph? Please separate them with an empty line. Also, please break the whole text somewhere between 60~70 chars. We want people to read it, don't we? PS1, Line 86: "$1" This is an empty string if $1 is not set. It still works, but git already complains that it won't much longer. PS1, Line 218: previse precise? -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.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]: Convert flashrom to git
by Nico Huber (Code Review) 10 Apr '17

10 Apr '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: (9 comments) Haven't looked at the commit message and getrevision.sh yet. https://review.coreboot.org/#/c/19206/1/Makefile File Makefile: PS1, Line 1369: @sed -e 's/^VERSION :=.*/VERSION := $(VERSION)/' \ : -e 's/^MAN_DATE :=.*/MAN_DATE := $(MAN_DATE)/' \ : -e 's#./util/getrevision.sh -c#false#' \ : Makefile >"$(EXPORTDIR)/flashrom-$(RELEASENAME)/Makefile" I'd rather not review a self-modifying Makefile... How about a Makefile.version that gets included if it exists? Plus providing weak overrides for these variables. Line 1374: # sed is required to filter out file names having the attribute set. Hmmm, fancy. One nit and one problem here: PS1, Line 1377: x;n;n;s/^set$$//;t;x;p I think 'x;n;n;/^set$$/{b};x;p' would be more clear. PS1, Line 1378: for f; This will ignore $0 which is the first argument after the command for `sh -c`... Line 1381: done' ...could be simply fixed by adding a dummy after the command like done' dummy_arg0 https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh File util/git-hooks/install.sh: PS1, Line 17: ../../ This assumption looks rather odd given the `git rev-parse` above. I think this is: "$(git rev-parse --prefix $(git rev-parse --git-path hooks/) --show-cdup)${src}wrapper.sh" https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push: PS1, Line 1: #!/bin/sh But it looks like a bash script to me. PS1, Line 53: No Signoff or Ack This reads abiguous. I think it wants to say "Either Signoff or Ack not found ...". PS1, Line 57: # Make _really_ sure we do not rewrite precious history : for lbranch in "${precious_branches[@]}"; do : if [ "$remote_ref" = "refs/heads/$lbranch" ]; then : nonreachable=$(git rev-list $remote_sha ^$local_sha) : if [ -n "$nonreachable" ]; then : echo "Only fast-forward pushes are allowed on $lbranch." >&2 : echo "$nonreachable is not included in $remote_sha while pusing to $remote_ref" >&2 : exit 1 : fi : fi : done Isn't this configurable in the upstream repo? -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.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]: Convert flashrom to git
by David Hendricks (Code Review) 08 Apr '17

08 Apr '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: Thanks, Martin. I'd still like to get the follow-up patch merged since it unbreaks things for people who check out flashrom from coreboot.org. -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Convert flashrom to git
by Martin Roth (Code Review) 07 Apr '17

07 Apr '17

Martin Roth has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: Since the jenkins builder always runs in offline mode, I updated it to add: git config flashrom.offline-builds true -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Convert flashrom to git
by build bot (Jenkins) (Code Review) 07 Apr '17

07 Apr '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/104/ : SUCCESS -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-PatchSet: 1 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) Gerrit-HasComments: No

1 0

Change in flashrom[staging]: This is a combination of 2 commits. The first commit's messa...
by David Hendricks (Code Review) 07 Apr '17

07 Apr '17

David Hendricks has abandoned this change. ( https://review.coreboot.org/19185 ) Change subject: This is a combination of 2 commits. The first commit's message is: ...................................................................... Abandoned Superceded by https://review.coreboot.org/#/c/19206/ -- To view, visit https://review.coreboot.org/19185 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I8d3f54c3e87d3875b73a6f24c15d2ae43b84819f Gerrit-PatchSet: 2 Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins)

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit