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:
> Actually it was correct in the earlier patch versions. I just
> forgot about
> the rpci_* when restructuring it. Should be fixed now, haven't
> retested yet.
Actually it wasn't correct in earlier patches. While I apparently commented on patchset 5, looking at my tree, I had actually tested patchset 1, so I already had the cleanup done in a shutdown handler.
Anyways, I tested the latest code and I still get the crash. I also moved the register for shutdown to the end of the function to make sure the handler gets registered *after* the rcpi_* handler gets registered, hoping it would mean the pci_access wouldn't get cleaned up until after the rcpi callback is called, but it didn't help.
Like I said before, the issue is that the accessor is set to -1 for some reason...
... and I just figured out why.. the machine I'm testing on is using pciutils 3.5.2, but the machine I'm building on is using pciutils 3.3.0. The structure changed (abi change but not api change?) and since the accessor is a 'used internally' its position in the structure changed.. when i print its value, it's actually printing the "numa_mode" field which is set to -1.
Anyways, the problem here is that all the libpci APIs take/return a pci_dev pointer, so whichever version of the library you use, the allocations and field accesses from within the library are safe, and the 'non-internal' fields are also safe to use. The issue here is that flashrom/pcidev.c does a copy of the structure as is, but not as a pointer, so the structure has the wrong size and when it's given to a libpci API function, it causes the segfault.
So basically, whatever you do, it will always crash because the pci_dev structure has the wrong size in pcidev.c. I'm going to assume that it would still crash regardless of the ich100 code. I think I didn't see it before because on BDW I was using flashrom from the apt repository instead of compiling it myself on a different computer/system.
This is the diff I had locally which fixed the crash for me : https://ghostbin.com/paste/6hd26 (this was against patchset 1)
Storing the pci_acc in dev->access is because trying to access it doesn't work (it's seen as -1) and without it, I get the error of can't find /sys/class/pci.../1f.5/...
I didn't want to show this patch before because it felt like an ugly hack, and that's why I said before "I don't know how it should be fixed", but I think it's correct, at least, partially, we need to store a pci_dev pointer instead of a structure in the rcpi callback data, if we use the pci_get_dev, then we lose the accessor, if we store the pointer as given to us, then we risk using a freed device when the callback is called.
My suggestion is, what if instead of creating a new pci_access and setting its method, what if in the case of ich100, we simply change the method of the global 'pacc' ? Will it affect anything? will pci devices get created or the accessor needed after we identify it as pch100 ? If not, then it would be safe to change the method of the global pci_access, and that would resolve the rcpi_ issue if we use pci_get_dev and simply use pacc.
--
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, 13 Jul 2017 19:12:46 +0000
Gerrit-HasComments: No
Hello Stefan Tauner, Youness Alaoui, David Hendricks, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/18962
to look at the new patch set (#9).
Change subject: ichspi: Add support for Intel Skylake
......................................................................
ichspi: Add support for Intel Skylake
The Sunrise Point PCH, paired with Skylake, has some minor changes
in the HW sequencing interface:
* Support for more flash regions moved PR* registers
* Only 4KiB erase blocks are supported by the primary erase command
* A second erase command for 64KiB pages was added
* More commands were added for status register access etc.
* A "Dedicated Lock Bits" register was added
No support for the new commands was added.
The SW sequencing interface seems to have moved register location and
is not supported any more officially. It's also untested.
Changes are loosely based on the Skylake support commit in Chromium OS
by Ramya Vijaykumar:
commit a9a64f9e4d52c39fcd3c5f7d7b88065baed189b1
Author: Ramya Vijaykumar <ramya.vijaykumar(a)intel.com>
flashrom: Add Skylake platform support
Change-Id: I0f4565a3c39f5fe3aec4fc8863605cebed1ad4ee
Signed-off-by: Nico Huber <nico.huber(a)secunet.com>
---
M ich_descriptors.c
M ich_descriptors.h
M ichspi.c
3 files changed, 227 insertions(+), 87 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/18962/9
--
To view, visit https://review.coreboot.org/18962
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f4565a3c39f5fe3aec4fc8863605cebed1ad4ee
Gerrit-Change-Number: 18962
Gerrit-PatchSet: 9
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>