- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: 4BA: Basic support for 4-bytes addressing mode extensions
by Nico Huber (Code Review) 11 Aug '17

11 Aug '17

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) 10 Aug '17

10 Aug '17

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) 10 Aug '17

10 Aug '17

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) 10 Aug '17

10 Aug '17

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

Change in flashrom[staging]: ich_descriptors: Fix off-by-one error
by Nico Huber (Code Review) 10 Aug '17

10 Aug '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20937 ) Change subject: ich_descriptors: Fix off-by-one error ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/20937/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20937/1//COMMIT_MSG@9 PS1, Line 9: The "number of masters" (NM) reflects the value that begins at 1. Seems that this is right for Lewisburg, awesome! more mess for us to handle. I've never found a definition if it starts at 0 or 1 and past practice was to count from 0 (== 1 master)! All descriptors in the wild that I've encountered were for platforms with 3 masters and always had NM set to 2 ;) Now the SPI guide for Lewisburg states it should be 6 (and there are only 6 masters mentioned), hmmm. I guess the lack of an explicit definition confused Intel's developers even more than us and now they disagree. No prob- lem for them, as they make one (binary) program for each platform anyway (I suppose). Two solutions come to mind: Take the guessed platform into account (Lewisburg would need its own constant). Or we could ignore a too high NM and just return MAX_NUM_MASTERS (and drop a lot of sanity checks as we could return an unsigned value). https://review.coreboot.org/#/c/20937/1//COMMIT_MSG@17 PS1, Line 17: even more wrong than it was before. I don't see any loop doing it wrong, nor any caller adding 1 to the return value. -- To view, visit https://review.coreboot.org/20937 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I92627332265cf79720b98241d73bc36b1f6a7167 Gerrit-Change-Number: 20937 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 11:55:24 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: ich_descriptors: Modify limits for C620/Lewisburg PCH
by Nico Huber (Code Review) 10 Aug '17

10 Aug '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20936 ) Change subject: ich_descriptors: Modify limits for C620/Lewisburg PCH ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/20936 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Gerrit-Change-Number: 20936 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 11:34:49 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ich_descriptors: Fix off-by-one error
by Paul Menzel (Code Review) 10 Aug '17

10 Aug '17

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/20937 ) Change subject: ich_descriptors: Fix off-by-one error ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/20937 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I92627332265cf79720b98241d73bc36b1f6a7167 Gerrit-Change-Number: 20937 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 09:43:38 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ich_descriptors: Modify limits for C620/Lewisburg PCH
by Paul Menzel (Code Review) 10 Aug '17

10 Aug '17

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/20936 ) Change subject: ich_descriptors: Modify limits for C620/Lewisburg PCH ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://review.coreboot.org/20936 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Gerrit-Change-Number: 20936 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 09:43:03 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ich_descriptors: Fix off-by-one error
by build bot (Jenkins) (Code Review) 10 Aug '17

10 Aug '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20937 ) Change subject: ich_descriptors: Fix off-by-one error ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/505/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/467/ : SUCCESS -- To view, visit https://review.coreboot.org/20937 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I92627332265cf79720b98241d73bc36b1f6a7167 Gerrit-Change-Number: 20937 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 05:44:18 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: ich_descriptors: Modify limits for C620/Lewisburg PCH
by build bot (Jenkins) (Code Review) 10 Aug '17

10 Aug '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20936 ) Change subject: ich_descriptors: Modify limits for C620/Lewisburg PCH ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/504/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/466/ : SUCCESS -- To view, visit https://review.coreboot.org/20936 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic8adc4b87993e65096166fa6d665432697070b4c Gerrit-Change-Number: 20936 Gerrit-PatchSet: 1 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 10 Aug 2017 05:44:13 +0000 Gerrit-HasComments: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit