Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/71834
to look at the new patch set (#2).
Change subject: internal: Move parallel logic into internal_par implementation
......................................................................
internal: Move parallel logic into internal_par implementation
The parallel internal programmer is its own implementation. Move
it and call into it from the top-level internal.c programmer
implementation.
Change-Id: Idabeceb59a36680f5fbb45d3ee4bd5dbf837373b
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M Makefile
M include/programmer.h
M internal.c
A internal_par.c
M meson.build
5 files changed, 101 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/71834/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/71834
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idabeceb59a36680f5fbb45d3ee4bd5dbf837373b
Gerrit-Change-Number: 71834
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71834 )
Change subject: internal: Move parallel logic into internal_par implementation
......................................................................
Patch Set 1:
(1 comment)
File internal_par.c:
https://review.coreboot.org/c/flashrom/+/71834/comment/44ca68c1_3ccbc74e
PS1, Line 16:
> I would add a paragraph here, because this is not an "independent" programmer. […]
Actually it is a independent programmer and probably _should_ be registered just by itself.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71834
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idabeceb59a36680f5fbb45d3ee4bd5dbf837373b
Gerrit-Change-Number: 71834
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 04 Feb 2023 04:13:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71577 )
(
3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: internal.c: Move sio register to own object
......................................................................
internal.c: Move sio register to own object
While super i/o is related to the internal programmer it
isn't actually _the_ internal programmer. Move register
logic to its own object consistent with other programmer
types.
Change-Id: I9a4c3e12bce5d22492c8d1b8f4a3f49d736dcf31
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/71577
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M Makefile
M internal.c
M meson.build
A superio.c
4 files changed, 63 insertions(+), 29 deletions(-)
Approvals:
build bot (Jenkins): Verified
Angel Pons: Looks good to me, but someone else must approve
Anastasia Klimchuk: Looks good to me, approved
diff --git a/Makefile b/Makefile
index a7ab00b..2ebd370 100644
--- a/Makefile
+++ b/Makefile
@@ -584,7 +584,7 @@
ifeq ($(CONFIG_INTERNAL) $(CONFIG_INTERNAL_X86), yes yes)
FEATURE_FLAGS += -D'CONFIG_INTERNAL=1'
PROGRAMMER_OBJS += processor_enable.o chipset_enable.o board_enable.o cbtable.o \
- internal.o it87spi.o sb600spi.o amd_imc.o wbsio_spi.o mcp6x_spi.o \
+ internal.o it87spi.o sb600spi.o superio.o amd_imc.o wbsio_spi.o mcp6x_spi.o \
ichspi.o dmi.o known_boards.o
ACTIVE_PROGRAMMERS += internal
endif
diff --git a/internal.c b/internal.c
index 808039d..c5fdb74 100644
--- a/internal.c
+++ b/internal.c
@@ -34,34 +34,6 @@
enum chipbustype internal_buses_supported = BUS_NONE;
-#if defined(__i386__) || defined(__x86_64__)
-void probe_superio(void)
-{
- probe_superio_winbond();
- /* ITE probe causes SMSC LPC47N217 to power off the serial UART.
- * Always probe for SMSC first, and if a SMSC Super I/O is detected
- * at a given I/O port, do _not_ probe that port with the ITE probe.
- * This means SMSC probing must be done before ITE probing.
- */
- //probe_superio_smsc();
- probe_superio_ite();
-}
-
-int superio_count = 0;
-#define SUPERIO_MAX_COUNT 3
-
-struct superio superios[SUPERIO_MAX_COUNT];
-
-int register_superio(struct superio s)
-{
- if (superio_count == SUPERIO_MAX_COUNT)
- return 1;
- superios[superio_count++] = s;
- return 0;
-}
-
-#endif
-
static void internal_chip_writeb(const struct flashctx *flash, uint8_t val,
chipaddr addr)
{
diff --git a/meson.build b/meson.build
index 36cb4f8..57ac438 100644
--- a/meson.build
+++ b/meson.build
@@ -247,6 +247,7 @@
'internal.c',
'it87spi.c',
'sb600spi.c',
+ 'superio.c',
'amd_imc.c',
'wbsio_spi.c',
'mcp6x_spi.c',
diff --git a/superio.c b/superio.c
new file mode 100644
index 0000000..3121578
--- /dev/null
+++ b/superio.c
@@ -0,0 +1,42 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2009 Carl-Daniel Hailfinger
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include "programmer.h"
+
+int superio_count = 0;
+#define SUPERIO_MAX_COUNT 3
+
+struct superio superios[SUPERIO_MAX_COUNT];
+
+int register_superio(struct superio s)
+{
+ if (superio_count == SUPERIO_MAX_COUNT)
+ return 1;
+ superios[superio_count++] = s;
+ return 0;
+}
+
+void probe_superio(void)
+{
+ probe_superio_winbond();
+ /* ITE probe causes SMSC LPC47N217 to power off the serial UART.
+ * Always probe for SMSC first, and if a SMSC Super I/O is detected
+ * at a given I/O port, do _not_ probe that port with the ITE probe.
+ * This means SMSC probing must be done before ITE probing.
+ */
+ //probe_superio_smsc();
+ probe_superio_ite();
+}
--
To view, visit https://review.coreboot.org/c/flashrom/+/71577
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a4c3e12bce5d22492c8d1b8f4a3f49d736dcf31
Gerrit-Change-Number: 71577
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72408 )
Change subject: dummyflasher: use new API to register shutdown function
......................................................................
Patch Set 2:
(1 comment)
File dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/72408/comment/9e789263_e0994291
PS2, Line 1416: data->refs_cnt += ((dummy_buses_supported & BUS_PROG) != BUS_NONE);
: data->refs_cnt += ((dummy_buses_supported & BUS_NONSPI) != BUS_NONE);
: data->refs_cnt += ((dummy_buses_supported & BUS_SPI) != BUS_NONE);
:
: #if 0
: data->refs_cnt += (dummy_buses_supported & BUS_PROG) ? 1 : 0;
: data->refs_cnt += (dummy_buses_supported & BUS_NONSPI) ? 1 : 0;
: data->refs_cnt += (dummy_buses_supported & BUS_SPI) ? 1 : 0;
: #endif
> I have another idea! :) What if you add `refs_cnt++` into every if below. […]
I thought about it at first sight. But this approach may potentially cause flashrom to crash.
If the dummy programmer registers more than one bus, then the next accident can happen => If the first condition the programmer enters fails because `register_*_master()` could not register shutdown (`register_shutdown()`), then it will call shutdown immediately (`mst->shutdown(data)`, opaque.c:55). And since we only have one owner in refs_cnt at that moment, data will be released.
When the dummy tries to enter the second condition and increase `data->refs_cnt`, flashrom will crash because this memory isn't valid anymore.
This should be mentioned in the code, but I cannot come up with brief comment for that.
--
To view, visit https://review.coreboot.org/c/flashrom/+/72408
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0c67c25b0f53cd8c564c4ea0f09f2728e856f6ea
Gerrit-Change-Number: 72408
Gerrit-PatchSet: 2
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 03 Feb 2023 08:47:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
Attention is currently required from: Miklós Márton, Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 )
Change subject: ni845x_spi: pass global state through func params
......................................................................
Patch Set 3:
(4 comments)
File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/72154/comment/8e228805_709e6be8
PS2, Line 131: static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle,
: enum USB845x_type device_pid)
> I am thinking, this could fit in one line: the hard limit for line length is 112 chars. […]
OK, It takes 113 chars - it's no big deal.
https://review.coreboot.org/c/flashrom/+/72154/comment/0fdc1a02_ba9ad6a4
PS2, Line 152: static int ni845x_spi_open(const char *serial, uInt32 *return_handle,
: enum USB845x_type *device_pid)
> this can be one line too
Done
https://review.coreboot.org/c/flashrom/+/72154/comment/bf52b72b_21093f01
PS2, Line 233: set_io_voltage_mV
> Not sure you meant it: there is `set_io_voltage_mV` which is an existing second argument in the func […]
This second argument (`set_io_voltage_mV`) contains a pointer to `io_voltage_in_mV` global var. If we add one more argument to this function, it will be pointer to `io_voltage_in_mV` too! So, let's just use the second argument.
Code snippets that prove my words are below.
ni845x_spi.c:447:
```
if (usb8452_spi_set_io_voltage(flash->chip->voltage.max, &io_voltage_in_mV, USE_LOWER)) {
...
}
```
ni845x_spi.c:470:
```
if (usb8452_spi_set_io_voltage(flash->chip->voltage.min, &io_voltage_in_mV, USE_HIGHER)) {
...
}
```
ni845x_spi.c:619:
```
if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER) < 0) {
...
}
```
https://review.coreboot.org/c/flashrom/+/72154/comment/134fdc2c_2be02433
PS2, Line 290: if (set_io_voltage_mV)
Miklós, how about dropping this condition? Maybe not in this patch, but in the one of the next. This argument should contain a valid pointer when you call the function to get the IO voltage that was set. Or am I missing something?
--
To view, visit https://review.coreboot.org/c/flashrom/+/72154
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61
Gerrit-Change-Number: 72154
Gerrit-PatchSet: 3
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 03 Feb 2023 08:07:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Miklós Márton, Alexander Goncharov.
Hello build bot (Jenkins), Miklós Márton, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/72154
to look at the new patch set (#3).
Change subject: ni845x_spi: pass global state through func params
......................................................................
ni845x_spi: pass global state through func params
Instead of relying on global variables, pass and use their value or
pointer to functions where possible. This patch prepares the programmer
to move global singleton states into a struct.
TOPIC=register_master_api
Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61
Signed-off-by: Alexander Goncharov <chat(a)joursoir.net>
Ticket: https://ticket.coreboot.org/issues/391
---
M ni845x_spi.c
1 file changed, 27 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/72154/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/72154
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61
Gerrit-Change-Number: 72154
Gerrit-PatchSet: 3
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 )
Change subject: tests: add bus coverage test for dummy
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72665
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319
Gerrit-Change-Number: 72665
Gerrit-PatchSet: 2
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Fri, 03 Feb 2023 07:48:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72665 )
Change subject: tests: add bus coverage test for dummy
......................................................................
Patch Set 2:
(1 comment)
File tests/dummyflasher.c:
https://review.coreboot.org/c/flashrom/+/72665/comment/695bb03c_a71edf47
PS1, Line 126: dummy_cover_buses_test_success
> I would rename to `dummy_all_buses_test_success`
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/72665
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iafe715feb5f5c0b5efd6827cdb2c3a314f542319
Gerrit-Change-Number: 72665
Gerrit-PatchSet: 2
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 03 Feb 2023 07:39:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment