Attention is currently required from: Thomas Heijligen, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61276 )
Change subject: hwaccess_x86_io: clean header concept
......................................................................
Patch Set 6:
(5 comments)
File hwaccess_x86_io.c:
https://review.coreboot.org/c/flashrom/+/61276/comment/414b9a22_9a4fdff1
PS6, Line 30: * - X86_IO_USE_LIBC
: * - X86_IO_USE_SWAPPED
How about more accurate names, e.g.
USE_LIBC_DESTINATION_LAST
USE_LIBC_DESTINATION_FIRST
https://review.coreboot.org/c/flashrom/+/61276/comment/828cffed_36ee389b
PS6, Line 33: * - X86_IO_USE_ASM
The whole file is about x86-I/O, so I would not repeat that in local
identifiers.
https://review.coreboot.org/c/flashrom/+/61276/comment/f5f4e91f_de8b9ecb
PS6, Line 51: #define X86_IO_USE_LIBC 1
To be honest, I don't feel good about the indentation. Is there a
precedent in flashrom?
https://review.coreboot.org/c/flashrom/+/61276/comment/e7aa34c0_73c1256d
PS6, Line 110: #define X86_IO_USE_DOS 1
We could also set aliases for the functions, like for iopl() above, e.g.
#define inb inportb
#define outb outportb
Maybe for another patch?
https://review.coreboot.org/c/flashrom/+/61276/comment/c290e359_e0f28732
PS6, Line 259: }
The input functions could be shared with USE_SWAPPED. Maybe for another
patch?
--
To view, visit https://review.coreboot.org/c/flashrom/+/61276
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1400704e9ac5fed00c096796536108d5bfb875e3
Gerrit-Change-Number: 61276
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 23:59:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Martin Roth, Subrata Banik, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> > > So, if this code change is considered a missed step in HW sequencing recommendation, then this could still be merged..correct?
> >
> > A version of this change that takes the applicable context into account
> > (e.g. other waiting loops in this driver, implications when multiple
> > instances would reach this point in the code simultaneously) would be
> > accepted immediately.
> >
> > > As for the failure that was noticed, it is when flashrom was being used in 2 modes
> > > 1. flashrom standalone
> > > 2. via futility which uses libflashrom implementation
> > >
> > > The standalone flashrom already has a fix for the issue, i.e, lockfile mechanism.
> > > libflashrom did not utilize/inherit the lockfile feature and did not acquire the lock. Which allowed other instances to run. This is already being fixed https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/…
> >
> > AFAIK, there is no such locking (yet) in this project. However, I fully
> > agree that file-based locking is the correct way. And it's interesting
> > to see that the problem is already solved for the fork. Makes me wonder
> > why Subrata asks how to solve such things here.
> >
> Isn't the HW based locking is much better than file based locking
Absolutely not. HW based locking would require hardware designed for such
locking. Even if we would have that (which we don't), it would still need
such a mechanism in every single driver (not everybody uses flashrom only
for internal flashing on Intel hardware). That would mean more work to
implement, more effort to maintain, to achieve little. But doesn't need
any argument about it anyway, because we don't have the hardware ;)
You're just wasting our time here.
> and how one can achieve file based locking even in low-level fw like coreboot spi driver or depthcharge ?
I don't know about depthcharge. But in coreboot there shouldn't be multiple
processes accessing the SPI controller. Even if there are, one could handle
it with a simple mutex because it's all one program.
> The reason, we opted for file based locking is to have a short term approach and share a fix rather waiting here in loop in code review :-). we are looking for a long term fix inside flashrom to have a `_wait_before` executing run execution.
Can you tell me more about the reasoning? The code is more than a decade old,
I don't see how that's a short term approach.
Again, these are discussions for the mailing list. By constraining the
discussion to Gerrit, you create a bottleneck in the review process for
a project that has close to zero review resources. It hurts the project.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 14:44:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Patrick Georgi, Martin Roth, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> > My recommendation was given in my very first comment on this change and was
> > repeated later. You choose to ignore it from the beginning. I can't help you
> > there any more.
>
> I've tried to respond to each questions being asked and shared the possible information that we are getting out from debug. Sometime early debug data was misleading (like, I suspected CSE FW is accessing the SPI bus when we are seeing 61849 is failing at Host side) and then have clarified saying (multiple processes might trying to access the same flashrom at host side is the culprit). Please help me know which one you felt is being ignored.
I don't know what's so hard to understand about "very first comment" this
was it:
```
The whole driver works synchronously, i.e. waits for any cycle to finish
that it started itself. So unless I miss something, checking SCIP would
only serve us in case of hardware failures or another program trying to
use the SPI controller at the same time (e.g. broken SMM). In both cases,
I'd say all bets are off anyway and we should definitely bail out.
Datasheets say to check the SCIP bit; we could just do so once (without
waiting) and if the bit is set bail out.
```
If you have questions about it just ask.
> > It will just take time to answer your questions
>
> Sure, take time, In either case I was not going to merge this CL with just one +2 from Intel (Rizwan) and waited for community for review time.
It seems you are confusing projects. This is flashrom. Nobody who hasn't
contributed to this project before can just merge patches here. And we
don't merge patches here hastily. The code your change is touching is
part of the `internal` programmer, hence a regression here could brick
machines. Changes to the code need many eyes and tests.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 14:33:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Patrick Georgi, Martin Roth, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> > So, if this code change is considered a missed step in HW sequencing recommendation, then this could still be merged..correct?
>
> A version of this change that takes the applicable context into account
> (e.g. other waiting loops in this driver, implications when multiple
> instances would reach this point in the code simultaneously) would be
> accepted immediately.
>
> > As for the failure that was noticed, it is when flashrom was being used in 2 modes
> > 1. flashrom standalone
> > 2. via futility which uses libflashrom implementation
> >
> > The standalone flashrom already has a fix for the issue, i.e, lockfile mechanism.
> > libflashrom did not utilize/inherit the lockfile feature and did not acquire the lock. Which allowed other instances to run. This is already being fixed https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/…
>
> AFAIK, there is no such locking (yet) in this project. However, I fully
> agree that file-based locking is the correct way. And it's interesting
> to see that the problem is already solved for the fork. Makes me wonder
> why Subrata asks how to solve such things here.
Isn't the HW based locking is much better than file based locking and how one can achieve file based locking even in low-level fw like coreboot spi driver or depthcharge ?
The reason, we opted for file based locking is to have a short term approach and share a fix rather waiting here in loop in code review :-). we are looking for a long term fix inside flashrom to have a `_wait_before` executing run execution.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 14:31:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Martin Roth, Subrata Banik, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> So, if this code change is considered a missed step in HW sequencing recommendation, then this could still be merged..correct?
A version of this change that takes the applicable context into account
(e.g. other waiting loops in this driver, implications when multiple
instances would reach this point in the code simultaneously) would be
accepted immediately.
> As for the failure that was noticed, it is when flashrom was being used in 2 modes
> 1. flashrom standalone
> 2. via futility which uses libflashrom implementation
>
> The standalone flashrom already has a fix for the issue, i.e, lockfile mechanism.
> libflashrom did not utilize/inherit the lockfile feature and did not acquire the lock. Which allowed other instances to run. This is already being fixed https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/…
AFAIK, there is no such locking (yet) in this project. However, I fully
agree that file-based locking is the correct way. And it's interesting
to see that the problem is already solved for the fork. Makes me wonder
why Subrata asks how to solve such things here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 14:23:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Patrick Georgi, Martin Roth, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> My recommendation was given in my very first comment on this change and was
> repeated later. You choose to ignore it from the beginning. I can't help you
> there any more.
I've tried to respond to each questions being asked and shared the possible information that we are getting out from debug. Sometime early debug data was misleading (like, I suspected CSE FW is accessing the SPI bus when we are seeing 61849 is failing at Host side) and then have clarified saying (multiple processes might trying to access the same flashrom at host side is the culprit). Please help me know which one you felt is being ignored.
> Nobody is walking away here.
It sounds like walking but I'm happy if that's not the case.
> It will just take time to answer your questions
Sure, take time, In either case I was not going to merge this CL with just one +2 from Intel (Rizwan) and waited for community for review time.
> .(I tried to answer everything that was on topic so far). You can't expect
> reviewers to quit their day jobs to attend your changes full-time.
No I don't either, but they do ack when they are actively reviewing a change and help to fix a problem which is paining.
I might be wrong, bt felt that you just drop your -2 without leaving comment on previous discussions (I agree those might be over weekend and that shared the urgency about the failure), especially this one,
Please see this, prior to Intel HW seq, we always used SCIP for syncing between the operations.
https://github.com/flashrom/flashrom/blob/master/ichspi.c#L968
Hence, I told, this is missing step in HW seq (and not only in flashrom but in coreboot CB:61849 and https://chromium-review.googlesource.com/c/chromiumos/platform/depthcharge/…)
I do respect your contribution and help. Kindly keep it in that way going forward to.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 13:35:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Stefan Reinauer, Sridhar Siricilla, Angel Pons, Alex Levin, YH Lin, Nico Huber, Martin Roth, Subrata Banik, Caveh Jalali, Tim Wawrzynczak, Nick Vaccaro, Boris Mittelberg.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> > @Nico, i didn't understand the comment about "cover up the actual problem", the code change in que […]
Aah, I understand now, Thanks @Nico.
So, if this code change is considered a missed step in HW sequencing recommendation, then this could still be merged..correct?
As for the failure that was noticed, it is when flashrom was being used in 2 modes
1. flashrom standalone
2. via futility which uses libflashrom implementation
The standalone flashrom already has a fix for the issue, i.e, lockfile mechanism.
libflashrom did not utilize/inherit the lockfile feature and did not acquire the lock. Which allowed other instances to run. This is already being fixed https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/…
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 13:21:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Martin Roth, Subrata Banik, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> @Nico, i didn't understand the comment about "cover up the actual problem", the code change in question is not a work around, but a recommendation in the chipset EDS.
The problem description was that multiple processes might use the registers at
once. And as was explained before, the SCIP bit is not meant to be used to
synchronize between multiple processes, and doesn't suffice to do so. Hence,
trying to synchronize via SCIP covers up that there are more severe problems
when using the register interface from mutliple processes at once.
It can, in theory, also cover up problems within flashrom itself, e.g. when
the current checks _after_ the flash cycle are not sufficient.
I fully agree to the EDS recommendation. We should check the bit and if it's
set unexpectedly, bail out. If flashrom itself is responsible for the unexpectedly
set bit, fix that as well.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 11:51:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Patrick Georgi, Martin Roth, Subrata Banik, Caveh Jalali, Stefan Reinauer, Tim Wawrzynczak, Sridhar Siricilla, Angel Pons, Alex Levin, Nick Vaccaro, YH Lin, Boris Mittelberg.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61854 )
Change subject: ichspi.c: Check SPI Cycle In-Progress prior start HW Seq
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> @Nico, here is guideline above giving `-2` in coreboot code review, looks like you are ignoring the prior comments and discussion about this thread to understand what is the chipset recommendation from Intel and what is being done in flashrom. As suggested below, please bring your recommendation along with `-2`.
My recommendation was given in my very first comment on this change and was
repeated later. You choose to ignore it from the beginning. I can't help you
there any more.
Nobody is walking away here. It will just take time to answer your questions
(I tried to answer everything that was on topic so far). You can't expect
reviewers to quit their day jobs to attend your changes full-time.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61854
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib9265cc20513fd00f32f8fa22e28c312903ca484
Gerrit-Change-Number: 61854
Gerrit-PatchSet: 6
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: YH Lin <yueherngl(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Patrick Georgi <patrick(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Sridhar Siricilla <sridhar.siricilla(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alex Levin <levinale(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: YH Lin <yueherngl(a)chromium.org>
Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 11:42:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Gerrit-MessageType: comment