- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: ichspi: Add support for Intel Skylake
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18962 ) Change subject: ichspi: Add support for Intel Skylake ...................................................................... Patch Set 10: Code-Review+1 I did a quick review, it all looks good, but I didn't verify with the datasheet if every offset is correct but I don't see any obvious errors in logic at least. -- To view, visit https://review.coreboot.org/18962 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I0f4565a3c39f5fe3aec4fc8863605cebed1ad4ee Gerrit-Change-Number: 18962 Gerrit-PatchSet: 10 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: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 27 Jul 2017 20:09:02 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake / Kabylake
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Youness Alaoui has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake / Kabylake ...................................................................... Patch Set 8: Code-Review+2 > > The RPCI fix is pushed here : https://review.coreboot.org/#/c/20784/ > > Now all we need is for this patch to set the 'pacc' variable and > > restore it at the end of the function. > > Um, I don't see why I have to wait for a seven years old bug to get > fixed before I add another chipset... also, if we fix it here and > not > in your patch, we'll lose the explicit information that the changes > are related (e.g. if anyone will ever fix the rpci interface he'll > more likely miss to remove the workaround here). Humm... you're right, the rpci bug is unrelated to skylake, I just see it only with skylake because that's the one that I build manually, I also associated it with this patch until I figured out the bug was from rpci. The patch looks good so I +2. I've half reviewed the ichspi one, but I'm not done yet. -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-Change-Number: 18925 Gerrit-PatchSet: 8 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 Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 27 Jul 2017 19:56:04 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by build bot (Jenkins) (Code Review) 27 Jul '17

27 Jul '17

build bot (Jenkins) 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 3: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/449/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/479/ : SUCCESS -- 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: 3 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, 27 Jul 2017 19:49:09 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Youness Alaoui 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 1: (2 comments) https://review.coreboot.org/#/c/20784/1/pcidev.c File pcidev.c: https://review.coreboot.org/#/c/20784/1/pcidev.c@297 PS1, Line 297: > Yes, the code was and is still checking the `pacc` pointer in It's not really nuts, it makes sense because the pci_access gets referenced in the pci_dev, and is used, so if the pacc is cleaned up and destroyed, then calling any pci_dev API will cause an error. The mistake here is that 'pacc' is not set to NULL after cleanup in its shutdown routine. Of course, this makes no more sense if the pacc variable is different like in the case of pch100, that check becomes wrong. https://review.coreboot.org/#/c/20784/2/programmer.h File programmer.h: https://review.coreboot.org/#/c/20784/2/programmer.h@200 PS2, Line 200: int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data); > That's not accurate... how about: Sounds good to me. Thanks -- 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: 1 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, 27 Jul 2017 19:48:44 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Hello David Hendricks, Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20784 to look at the new patch set (#3). 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> --- M pcidev.c M programmer.h 2 files changed, 20 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/20784/3 -- To view, visit https://review.coreboot.org/20784 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Gerrit-Change-Number: 20784 Gerrit-PatchSet: 3 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) 27 Jul '17

27 Jul '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 2: (2 comments) Please revise the comment in the header file. Looks good, otherwise. https://review.coreboot.org/#/c/20784/1/pcidev.c File pcidev.c: https://review.coreboot.org/#/c/20784/1/pcidev.c@297 PS1, Line 297: } > Added in the header file. The code was already checking for validity of 'pa Yes, the code was and is still checking the `pacc` pointer in the shutdown handler, which is nuts IMHO. It has nothing to say about the sanity of the program as the pointer is not used anymore. `pacc` is only interesting for the pci_get_dev() call. https://review.coreboot.org/#/c/20784/2/programmer.h File programmer.h: https://review.coreboot.org/#/c/20784/2/programmer.h@200 PS2, Line 200: */ That's not accurate... how about: 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. -- 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: 2 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, 27 Jul 2017 19:02:20 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Youness Alaoui 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 1: (3 comments) https://review.coreboot.org/#/c/20784/1/pcidev.c File pcidev.c: https://review.coreboot.org/#/c/20784/1/pcidev.c@262 PS1, Line 262: struct pci_dev * dev; > No whitespace after the asterisk, please. oups, I noticed it but forgot to fix it. https://review.coreboot.org/#/c/20784/1/pcidev.c@297 PS1, Line 297: > Add a comment that `dev` passed to rpci_* has to be compatible Added in the header file. The code was already checking for validity of 'pacc', so it was already a requirement before my change, but I've changed things to make it more robust. https://review.coreboot.org/#/c/20784/1/pcidev.c@307 PS1, Line 307: a->domain, a->bus, a->dev, a->func); \ > Bail out if `undo_pci_write_data->dev` is NULL or check it Done -- 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: 1 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, 27 Jul 2017 18:42:39 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by build bot (Jenkins) (Code Review) 27 Jul '17

27 Jul '17

build bot (Jenkins) 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 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/448/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/478/ : SUCCESS -- 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: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 27 Jul 2017 18:42:17 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks
by Youness Alaoui (Code Review) 27 Jul '17

27 Jul '17

Hello David Hendricks, Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20784 to look at the new patch set (#2). 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> --- M pcidev.c M programmer.h 2 files changed, 16 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/20784/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: newpatchset Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298 Gerrit-Change-Number: 20784 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: chipset_enable: Add support for Intel Skylake / Kabylake
by Nico Huber (Code Review) 27 Jul '17

27 Jul '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/18925 ) Change subject: chipset_enable: Add support for Intel Skylake / Kabylake ...................................................................... Patch Set 8: > The RPCI fix is pushed here : https://review.coreboot.org/#/c/20784/ > Now all we need is for this patch to set the 'pacc' variable and > restore it at the end of the function. Um, I don't see why I have to wait for a seven years old bug to get fixed before I add another chipset... also, if we fix it here and not in your patch, we'll lose the explicit information that the changes are related (e.g. if anyone will ever fix the rpci interface he'll more likely miss to remove the workaround here). -- To view, visit https://review.coreboot.org/18925 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I000819aff25fbe9764f33df85f040093b82cd948 Gerrit-Change-Number: 18925 Gerrit-PatchSet: 8 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 Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Youness Alaoui <snifikino(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 27 Jul 2017 11:32:26 +0000 Gerrit-HasComments: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit