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

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by David Hendricks (Code Review) Aug. 15, 2017

Aug. 15, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 Aug 2017 00:39:07 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Convert flashrom to git
by Stefan Tauner (Code Review) Aug. 15, 2017

Aug. 15, 2017

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 11: (15 comments) FWIW, these are my replies - mostly to Nico's excellent review - that also try to show rationales where my current patch diverges from what eventually landed in staging as of now. I still need to figure out how to integrate gerrit (and its hooks) into the scheme and will publish it then (with the intention to make it the first stable commit after 0.9.9). https://review.coreboot.org/#/c/19206/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/19206/1//COMMIT_MSG@28 PS1, Line 28: here are a few major differences: : - This uses coreboot's commit-msg hook which includes support for : generating and appending Change-Id. : - djgpp-dos target removal is moved to a > This seems way over-complicated with no visible use case. It also makes the Of course it is over-complicated - it's what Carl-Daniel and I came up with when I convinced him to switch to git by fulfilling his requirements - namely having version strings with some kind of monotonicity that can easily be referred to by humans. Programmatically it is trivial to just use the git hash and derive some other string like that of git describe if that's needed by anybody. I don't see why this would be needed though? The "visible use case" of the implemented scheme is simply... any log file posted that any developer needs to read? Apart from the additional complexity of the get_revision script I don't really see any downsides. I put some substantial effort into making the high-quality transition to git possible, which includes this borderline-insane scheme to create somewhat monotonic version strings out of git without too much hassle for anybody. Of course I would like to keep it and there need to be better arguments then "meh this looks too complicated and I don't need it" to convince me otherwise. https://review.coreboot.org/#/c/19206/1//COMMIT_MSG@63 PS1, Line 63: : > Honestly, who cares? If somebody gets on the mailing list and presents this It still gives a relatively good indication of what is contained in the code for those familiar with the commit history *without* checking a VCS. https://review.coreboot.org/#/c/19206/1/Makefile File Makefile: https://review.coreboot.org/#/c/19206/1/Makefile@1357 PS1, Line 1357: > This overwrites the default system-management heading (center of Basically, because I think it is a good idea to give the user the version on the top instead of the rather useless manual section name. :) And I would have never thought anybody would review this change in such detail so I simply slipped it in. If there is no objection I will simply note it in the commit message of the stable branch... https://review.coreboot.org/#/c/19206/1/Makefile@1369 PS1, Line 1369: @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \ : ( echo "found."; echo "UTSNAME := yes" >> .features.tmp ) || \ : ( echo "not found."; echo "UTSNAME := no" >> .features.tmp ) } 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) : @printf "Checking for clock_gettime support... " | tee -a The makefile does not modify itself. It generates a modified version of itself in the export directory. :P But I see your point. My implementation for your idea is slightly different to David's but essentially also creating and using a Makefile.version file. https://review.coreboot.org/#/c/19206/1/Makefile@1377 PS1, Line 1377: t found."; echo "CLOCK > I think 'x;n;n;/^set$$/{b};x;p' would be more clear. Is it? 'b' is documented in POSIX and sed's info pages as unconditional. In the case above this is kind of moot because there is a condition that guards the unconditional branch: the address using a regex to match the respective lines. Took me a while to grasp because I never really used addresses so far. When knowing this, the construct is nicer, true. I'll change it and also improve the comments. One question: why the '{}' around 'b'? https://review.coreboot.org/#/c/19206/1/Makefile@1381 PS1, Line 1381: > ...could be simply fixed by adding a dummy after the command like Good catch! Thank you. https://review.coreboot.org/#/c/19206/4/Makefile File Makefile: https://review.coreboot.org/#/c/19206/4/Makefile@a1374 PS4, Line 1374: > Why didn't I bikeshed this for being totally unrelated? It should have been removed a very long time ago, and like other minor things I did not expect anybody to notice its removal in a review at all... https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@44 PS1, Line 44: } : : git_is_file_tracked() { > Do we break lines only at full stops now? This looks like a single No, we break at the latest at a 112 character limit because that's what was agreed upon when the matter of line breaking was last discussed on the mailing list in 2012. In this case I broke it early but I cannot say why and this is indeed a bug. Possibly because breaking after every full stop is the style used when I write text documents (Latex etc). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@229 PS1, Line 229: query_ > So we never synchronize in the upstream-repo-registered case but The idea behind this is simply to optimize performance for the common case. If the user uses upstream repos the code assumes that the user updates them in case some commits are merged upstream. It is rather improbable that a user continuously implements changes on top of a single (aging) upstream version (unless upstream is not updated for years but that's another story ;) without fetching any updates. I have added this explanation to the source comment (the user output explains the updates of the upcache already and i dont want to complicate it ever further). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@257 PS1, Line 257: : : : : : : > Nit, looks like `staging` would override `stable` here even if they AFAICT the order is determined by that in 'upstream_branches' but I have not re-tested this now. Why do you think otherwise? https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269 PS1, Line 269: > always true on this path? No, there might be upstream branches that are neither stable nor staging (e.g. a release branch such as 0.9.9 for bug fixes). There is also an example in the commit message. https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh File util/git-hooks/install.sh: https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh@17 PS1, Line 17: $(git > This assumption looks rather odd given the `git rev-parse` above. No, IIRC, the idea is to make these relative links because else they get invalid when someone moves the checked out directory. https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@1 PS1, Line 1: #!/bin/sh > Since we probably want to avoid bashisms in general for git hooks, I got ri What David(?) said... https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53 PS1, Line 53: ommit $local_sha i > I reworded this to try and make it less confusing. LMK what you think. Much clearer, thank you. https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@57 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 | head -1) : if [ -n "$nonreachable" ]; then : echo "Only fast-forward pushes are allowed on $lbranch." >&2 : echo "$nonreachable is not included in $remote_sha while pushing to $remote_ref" >&2 : exit 1 : fi : fi > Isn't this configurable in the upstream repo? The patch was written when I still hoped we could avoid Gerrit... and plain git (as used on flashrom.org/git) does not support that. I have not thought about how to deal with the two repositories in the hooks yet... just catching up with the changes to this patch atm. -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-Change-Number: 19206 Gerrit-PatchSet: 11 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: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Tue, 15 Aug 2017 23:09:17 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by build bot (Jenkins) (Code Review) Aug. 15, 2017

Aug. 15, 2017

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/508/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/468/ : SUCCESS -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 15 Aug 2017 09:32:51 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by Nico Huber (Code Review) Aug. 15, 2017

Aug. 15, 2017

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21015 Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Include <sys/types.h> wherever ssize_t is used `ssize_t` is a POSIX type (cf. IEEE Std 1003.1). Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reported-by: Urja Rannikko <urjaman(a)gmail.com> --- M dediprog.c M ich_descriptors.c M ich_descriptors.h 3 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/21015/1 diff --git a/dediprog.c b/dediprog.c index 6f82772..2f5b441 100644 --- a/dediprog.c +++ b/dediprog.c @@ -21,6 +21,7 @@ #include "platform.h" +#include <sys/types.h> #include <stdlib.h> #include <stdio.h> #include <string.h> diff --git a/ich_descriptors.c b/ich_descriptors.c index a2f8edf..7916152 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -34,6 +34,7 @@ #define UPPER_MAP_OFFSET (4096 - 256 - 4) #define getVTBA(flumap) (((flumap)->FLUMAP1 << 4) & 0x00000ff0) +#include <sys/types.h> #include <string.h> #include "flash.h" /* for msg_* */ #include "programmer.h" diff --git a/ich_descriptors.h b/ich_descriptors.h index 8c313d1..a3af8c2 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -22,6 +22,7 @@ #ifndef __ICH_DESCRIPTORS_H__ #define __ICH_DESCRIPTORS_H__ 1 +#include <sys/types.h> #include <stdint.h> #include "programmer.h" /* for enum ich_chipset */ -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newchange Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[staging]: 4BA: Basic support for 4-bytes addressing mode extensions
by David Hendricks (Code Review) Aug. 13, 2017

Aug. 13, 2017

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20513 ) Change subject: 4BA: Basic support for 4-bytes addressing mode extensions ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Either way is fine with me. If you have time and can edit this patch, I'll be happy to re-review and ack. There might be style issues in the follow-up patches, so perhaps doing a single !fixup would be the easiest/quickest thing to do here. https://review.coreboot.org/#/c/20513/1/spi4ba.c File spi4ba.c: https://review.coreboot.org/#/c/20513/1/spi4ba.c@127 PS1, Line 127: + 1) - 1 > > I'd eventually like to switch to using dynamically allocated memory That sounds good to me as well. -- To view, visit https://review.coreboot.org/20513 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ie72e2a89cd75fb4d09f48e81c4c1d927c317b7a7 Gerrit-Change-Number: 20513 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> 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) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Aug 2017 06:50:13 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ich_descriptors: Modify limits for C620/Lewisburg PCH
by David Hendricks (Code Review) Aug. 12, 2017

Aug. 12, 2017

David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/20936 ) Change subject: ich_descriptors: Modify limits for C620/Lewisburg PCH ...................................................................... ich_descriptors: Modify limits for C620/Lewisburg PCH Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Signed-off-by: David Hendricks <dhendricks(a)fb.com> Reviewed-on: https://review.coreboot.org/20936 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Paul Menzel <paulepanter(a)users.sourceforge.net> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M ich_descriptors.h 1 file changed, 3 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved diff --git a/ich_descriptors.h b/ich_descriptors.h index 8c313d1..574bc5f 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -165,7 +165,7 @@ }; }; -#define MAX_NUM_FLREGS 10 /* 16 on unsupported Lewisburg PCH */ +#define MAX_NUM_FLREGS 16 struct ich_desc_region { /* * Number of entries and width differ on various generations: @@ -174,6 +174,7 @@ * ICH8 .. Panther Point/7 5 13 * Lynx Point/8 .. Wildcat Point/9 7 15 * Sunrise Point/100 .. 10 15 + * Lewisburg/100 .. 16 15 */ union { uint32_t FLREGs[MAX_NUM_FLREGS]; /* Flash Descriptor Regions */ @@ -188,7 +189,7 @@ }; }; -#define MAX_NUM_MASTERS 5 /* 6 on unsupported Lewisburg PCH */ +#define MAX_NUM_MASTERS 6 /* 5 prior to C620/Lewisburg PCH */ struct ich_desc_master { union { uint32_t FLMSTRs[MAX_NUM_MASTERS]; /* Flash Masters */ -- To view, visit https://review.coreboot.org/20936 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Gerrit-Change-Number: 20936 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> 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: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: 4BA: Basic support for 4-bytes addressing mode extensions
by Nico Huber (Code Review) Aug. 10, 2017

Aug. 10, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20513 ) Change subject: 4BA: Basic support for 4-bytes addressing mode extensions ...................................................................... Patch Set 2: Code-Review+1 (12 comments) Just nits, style and non-functional issues we can also handle in a !fixup, I suppose. I can do that if nobody beats me to it. Or shall I edit this change and you ack it again? https://review.coreboot.org/#/c/20513/2/flash.h File flash.h: https://review.coreboot.org/#/c/20513/2/flash.h@124 PS2, Line 124: #define FEATURE_4BA_ONLY (1 << 11) nit, doesn't align well https://review.coreboot.org/#/c/20513/1/spi4ba.c File spi4ba.c: https://review.coreboot.org/#/c/20513/1/spi4ba.c@127 PS1, Line 127: + 1) - 1 > I'd eventually like to switch to using dynamically allocated memory > so we don't duplicate all this code between 3BA and 4BA versions. I > implemented that in a pending patch for the CrOS branch: > https://chromium-review.googlesource.com/c/337202/1 . But that can > happen later if desired. Actually we don't need dynamically allocated memory, just the offset for the memcpy() would have to be adjusted (and the buffer has to big enough ofc). But some abstraction like your new_spi_write_command() also crossed my mind during this review. https://review.coreboot.org/#/c/20513/2/spi4ba.c File spi4ba.c: https://review.coreboot.org/#/c/20513/2/spi4ba.c@46 PS2, Line 46: nit, spurious space https://review.coreboot.org/#/c/20513/2/spi4ba.c@74 PS2, Line 74: same here https://review.coreboot.org/#/c/20513/2/spi4ba.c@77 PS2, Line 77: msg_cerr("%s failed during command execution\n", __func__); Don't know the coding style very well, in coreboot a single- line body shouldn't have braces. https://review.coreboot.org/#/c/20513/2/spi4ba.c@84 PS2, Line 84: uint8_t databyte) Inconsistent alignment, sometimes two tabs are used, sometimes tabs+spaces, here tabs only. https://review.coreboot.org/#/c/20513/2/spi4ba.c@100 PS2, Line 100: (addr & 0xff), `>> 0` used in other places https://review.coreboot.org/#/c/20513/2/spi4ba.c@127 PS2, Line 127: + 1) - 1 I don't get what the +1-1 should tell the reader? Ok, guess now I understand it: +1 for the 4th address byte -1 for the single byte that is included in the 256 Would be more clear if we'd define something like JEDEC_4_BYTE_ADDR_BYTE_PROGRAM_OUTSIZE (6) and JEDEC_4_BYTE_ADDR_PROGRAM_OUTSIZE (5). (This is all ugly shit, IMHO. But not due to this patch but the spi25 code that obviously was used as a template. The size of the static parts of the command should be implicitly given by the data, IMO. Currently the code suffers from redundancy that makes it har- der to read, i.e. I trust the compiler to do the calculations but not developers.) https://review.coreboot.org/#/c/20513/2/spi4ba.c@175 PS2, Line 175: uint8_t *bytes, unsigned int len) alignment https://review.coreboot.org/#/c/20513/2/spi4ba.c@193 PS2, Line 193: unsigned int blocklen) weirdest alignment https://review.coreboot.org/#/c/20513/2/spi4ba.c@237 PS2, Line 237: 32 Is this specified somewhere? IIRC, for 3BA it was inconsistent across chips. https://review.coreboot.org/#/c/20513/2/spi4ba.c@327 PS2, Line 327: } flashrom the project of redundancy? I just reviewed the same function three times... -- To view, visit https://review.coreboot.org/20513 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ie72e2a89cd75fb4d09f48e81c4c1d927c317b7a7 Gerrit-Change-Number: 20513 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> 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) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 21:35:34 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Nico Huber (Code Review) Aug. 10, 2017

Aug. 10, 2017

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20784 ) Change subject: rpci: Use pci_dev struct pointer to avoid API breaks ...................................................................... rpci: Use pci_dev struct pointer to avoid API breaks The pci_dev structure is never meant to be used as is, but always as a pointer. By using the struct itself in undo_pci_write_data, we are risking data corruption, or buffer overflows if the structure size changes. This is especially apparent on my system where flashrom segfaults because I compile it with pciutils 3.3.0 and I run it on a system with pciutils 3.5.2. The struture size is different and causes a struct with the wrong size to be sent to the library, with invalid internal field values. This has been discovered and discussed in Change ID 18925 [1] [1] https://review.coreboot.org/#/c/18925/ Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Signed-off-by: Youness Alaoui <kakaroto(a)kakaroto.homelinux.net> Reviewed-on: https://review.coreboot.org/20784 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M chipset_enable.c M pcidev.c M programmer.h 3 files changed, 25 insertions(+), 9 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/chipset_enable.c b/chipset_enable.c index 20d2662..6a93d0d 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -843,6 +843,7 @@ * straints (e.g. on PCI domains, extended PCIe config space). */ struct pci_access *const pci_acc = pci_alloc(); + struct pci_access *const saved_pacc = pacc; if (!pci_acc) { msg_perr("Can't allocate PCI accessor.\n"); return ret; @@ -857,6 +858,9 @@ return ret; } + /* Modify pacc so the rpci_write can register the undo callback with a + * device using the correct pci_access */ + pacc = pci_acc; enable_flash_ich_report_gcs(spi_dev, pch_generation, NULL); const int ret_bc = enable_flash_ich_bios_cntl_config_space(spi_dev, pch_generation, 0xdc); @@ -880,6 +884,7 @@ _freepci_ret: pci_free_dev(spi_dev); + pacc = saved_pacc; return ret; } diff --git a/pcidev.c b/pcidev.c index 2c78063..f4e5542 100644 --- a/pcidev.c +++ b/pcidev.c @@ -160,6 +160,7 @@ return 1; } pci_cleanup(pacc); + pacc = NULL; return 0; } @@ -259,7 +260,7 @@ }; struct undo_pci_write_data { - struct pci_dev dev; + struct pci_dev *dev; int reg; enum pci_write_type type; union { @@ -272,22 +273,23 @@ int undo_pci_write(void *p) { struct undo_pci_write_data *data = p; - if (pacc == NULL) { - msg_perr("%s: Tried to undo PCI writes without a valid PCI context!\n" - "Please report a bug at flashrom(a)flashrom.org\n", __func__); + if (pacc == NULL || data->dev == NULL) { + msg_perr("%s: Tried to undo PCI writes without a valid PCI %s!\n" + "Please report a bug at flashrom(a)flashrom.org\n", + __func__, data->dev == NULL ? "device" : "context"); return 1; } msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n", - data->dev.bus, data->dev.dev, data->dev.func, data->reg); + data->dev->bus, data->dev->dev, data->dev->func, data->reg); switch (data->type) { case pci_write_type_byte: - pci_write_byte(&data->dev, data->reg, data->bytedata); + pci_write_byte(data->dev, data->reg, data->bytedata); break; case pci_write_type_word: - pci_write_word(&data->dev, data->reg, data->worddata); + pci_write_word(data->dev, data->reg, data->worddata); break; case pci_write_type_long: - pci_write_long(&data->dev, data->reg, data->longdata); + pci_write_long(data->dev, data->reg, data->longdata); break; } /* p was allocated in register_undo_pci_write. */ @@ -303,7 +305,11 @@ msg_gerr("Out of memory!\n"); \ exit(1); \ } \ - undo_pci_write_data->dev = *a; \ + if (pacc) \ + undo_pci_write_data->dev = pci_get_dev(pacc, \ + a->domain, a->bus, a->dev, a->func); \ + else \ + undo_pci_write_data->dev = NULL; \ undo_pci_write_data->reg = b; \ undo_pci_write_data->type = pci_write_type_##c; \ undo_pci_write_data->c##data = pci_read_##c(dev, reg); \ diff --git a/programmer.h b/programmer.h index ec00bd9..e58fd32 100644 --- a/programmer.h +++ b/programmer.h @@ -195,6 +195,11 @@ struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. + * To clone the pci_dev instances internally, the `pacc` global + * variable has to reference a pci_access method that is compatible + * with the given pci_dev handle. The referenced pci_access (not + * the variable) has to stay valid until the shutdown handlers are + * finished. */ int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data); int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data); -- To view, visit https://review.coreboot.org/20784 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Gerrit-Change-Number: 20784 Gerrit-PatchSet: 5 Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com> 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: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Nico Huber (Code Review) Aug. 10, 2017

Aug. 10, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20784 ) Change subject: rpci: Use pci_dev struct pointer to avoid API breaks ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/20784 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Gerrit-Change-Number: 20784 Gerrit-PatchSet: 4 Gerrit-Owner: Youness Alaoui <snifikino(a)gmail.com> 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: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 15:17:44 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chipset_enable: Mark Braswell as tested
by Nico Huber (Code Review) Aug. 10, 2017

Aug. 10, 2017

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20923 ) Change subject: chipset_enable: Mark Braswell as tested ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/20923 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iaf7558af8737af36401f577ca7aba9fd7114a3df Gerrit-Change-Number: 20923 Gerrit-PatchSet: 1 Gerrit-Owner: 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) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 13:12:51 +0000 Gerrit-HasComments: No

1 0