Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 )
Change subject: hwaccess: replace macros by C code
......................................................................
Patch Set 6:
(1 comment)
File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/62898/comment/a52e4282_d57d1419
PS6, Line 34: inline
> Why to force inline? The GCC should do a proper job for optimizing and I would trust it to do the ri […]
Okay, I thought you wanted it to always be inline to match what the macro was doing. If you don't care whether it's actually inlined, then I agree the attribute isn't needed.
GCC will decide what to put inline based on the optimization level. Currently, the Makefile sets the optimization to -Os, which should probably put this inline even without the inline statement since it enables inline-small-functions.
If we're trusting the compiler to do the "right" thing here, why even mark it inline at all?
--
To view, visit https://review.coreboot.org/c/flashrom/+/62898
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I86d38d816b37c283279c485fac8027f8fb94364a
Gerrit-Change-Number: 62898
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 22 Mar 2022 03:29:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59280 )
Change subject: pcidev: Move pci_dev_find() from internal to canonical place
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59280/comment/cf50e500_b2da7eae
PS6, Line 9: Squash in,
> Done
Thanks
--
To view, visit https://review.coreboot.org/c/flashrom/+/59280
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191
Gerrit-Change-Number: 59280
Gerrit-PatchSet: 7
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 22 Mar 2022 01:25:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Martin Roth, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 )
Change subject: hwaccess: replace macros by C code
......................................................................
Patch Set 6:
(1 comment)
File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/62898/comment/b133881f_ef80fdaf
PS6, Line 34: inline
> Do these need __attribute__((always_inline)) to actually force it to be inline? My understanding is […]
Why to force inline? The GCC should do a proper job for optimizing and I would trust it to do the right thing. In this case I wouldn't dictate the exact behavior to GCC
--
To view, visit https://review.coreboot.org/c/flashrom/+/62898
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I86d38d816b37c283279c485fac8027f8fb94364a
Gerrit-Change-Number: 62898
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 22 Mar 2022 01:19:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59280 )
Change subject: pcidev: Move pci_dev_find() from internal to canonical place
......................................................................
Patch Set 7:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/59280/comment/f89ff2cf_3e3116f6
PS6, Line 9: Squash in,
> Nit, align text with previous commit in chain. They do the same thing, only for different functions.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/59280
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191
Gerrit-Change-Number: 59280
Gerrit-PatchSet: 7
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Tue, 22 Mar 2022 01:03:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan.
Hello build bot (Jenkins), Nico Huber, Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/59280
to look at the new patch set (#7).
Change subject: pcidev: Move pci_dev_find() from internal to canonical place
......................................................................
pcidev: Move pci_dev_find() from internal to canonical place
Also rename to `pcidev_find()` in fitting with pcidev.c helpers.
BUG=b:220950271
TEST=```sudo ./flashrom -p internal -r /tmp/bios
<snip>
Found Programmer flash chip "Opaque flash chip" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000.
Reading flash... done.
```
Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M board_enable.c
M chipset_enable.c
M internal.c
M pcidev.c
M programmer.h
M sb600spi.c
6 files changed, 32 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/59280/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/59280
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191
Gerrit-Change-Number: 59280
Gerrit-PatchSet: 7
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 )
Change subject: hwaccess: replace macros by C code
......................................................................
Patch Set 6:
(1 comment)
File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/62898/comment/eec624ee_b5b0c1e7
PS6, Line 34: inline
Do these need __attribute__((always_inline)) to actually force it to be inline? My understanding is that without the attribute, the compiler (GCC at least) can choose what to do based on optimization for size/speed.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62898
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I86d38d816b37c283279c485fac8027f8fb94364a
Gerrit-Change-Number: 62898
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Martin Roth <martinroth(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 22 Mar 2022 01:00:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62898 )
Change subject: hwaccess: replace macros by C code
......................................................................
Patch Set 6:
(1 comment)
File hwaccess.h:
https://review.coreboot.org/c/flashrom/+/62898/comment/6ff50ac5_b5d52d50
PS6, Line 28: swap
> It used to be "swab" and now it is "swap" (one char difference in the name). […]
I thought to swap (with the meaning of exchange) is a better wording. I'm not fixed with it.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62898
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I86d38d816b37c283279c485fac8027f8fb94364a
Gerrit-Change-Number: 62898
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Tue, 22 Mar 2022 00:56:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment