Attention is currently required from: Anastasia Klimchuk, Alexander Goncharov.
Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 )
Change subject: ni845x_spi: refactor singleton states into reentrant pattern
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> Oh! do you think it was broken by the patch, or at some point earlier? […]
No it got broken earlier. Or maybe something on the mingw make had been changed, it looks that it has issues with the spaces in the linker/include path (which is C:\Program Files\-...)
I think I am going to remove the auto detection at all (still keeping it optional), as I doubt that NI will ever change the installation path/method and keep the current path "hardcoded".
--
To view, visit https://review.coreboot.org/c/flashrom/+/72160
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484
Gerrit-Change-Number: 72160
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-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Tue, 25 Apr 2023 13:24:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Miklós Márton, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 )
Change subject: ni845x_spi: refactor singleton states into reentrant pattern
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> Well I need to make a few patches before testing it: the NI-845x detection looks to be broken and so […]
Oh! do you think it was broken by the patch, or at some point earlier?
Also, you said you made few patches to fix the situation: probably send them to Gerrit? can I help with anything? Thank you so much!
--
To view, visit https://review.coreboot.org/c/flashrom/+/72160
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484
Gerrit-Change-Number: 72160
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-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Tue, 25 Apr 2023 13:08:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Angel Pons, Steve Markgraf.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71801 )
Change subject: programmer: Add bitbanging programmer driver for Linux libgpiod
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
Hello Steve! We have some comments on the patch, but it looks like maybe you don't have much time at the moment. I was wondering how we can complete this work, I really don't want your work to be lost.
Do you have any time to resolve the comments?
If not, we can wait for you to come back and resolve later.
Another option can be: someone else (maybe me :D ) resolves the comments and uploads new patchset. Would you be able to do the final testing on the hardware in this case?
Thank you!
--
To view, visit https://review.coreboot.org/c/flashrom/+/71801
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Icad3eb7764f28feaea51bda3a7893da724c86d06
Gerrit-Change-Number: 71801
Gerrit-PatchSet: 6
Gerrit-Owner: Steve Markgraf <steve(a)steve-m.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Steve Markgraf <steve(a)steve-m.de>
Gerrit-Comment-Date: Mon, 24 Apr 2023 13:48:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73039 )
Change subject: flashrom: rewrite flashbuses_to_text()
......................................................................
flashrom: rewrite flashbuses_to_text()
The previous implementation had no error handling, as a result the
flashrom could crash if the computer ran out of memory. The new
version returns NULL in such cases.
Also, rewrite lots of `if` conditions to one cycle, store a name of
buses and `enum chipbustype` in an array by using a custom struct.
The caller always expected a non-null value, so change its behavior to
handle a possible null value or use the `?` symbol. As far as `free()`
can handle null pointers, do nothing with such callers.
TEST=ninja test
Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c
Signed-off-by: Alexander Goncharov <chat(a)joursoir.net>
Ticket: https://ticket.coreboot.org/issues/408
Reviewed-on: https://review.coreboot.org/c/flashrom/+/73039
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M cli_classic.c
M flashrom.c
M print.c
M print_wiki.c
4 files changed, 81 insertions(+), 23 deletions(-)
Approvals:
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c
index 787de67..e4db913 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -1011,7 +1011,7 @@
goto out_shutdown;
}
tempstr = flashbuses_to_text(get_buses_supported());
- msg_pdbg("The following protocols are supported: %s.\n", tempstr);
+ msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?");
free(tempstr);
for (j = 0; j < registered_master_count; j++) {
@@ -1082,7 +1082,8 @@
/* repeat for convenience when looking at foreign logs */
tempstr = flashbuses_to_text(flashes[0].chip->bustype);
msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n",
- flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size, tempstr);
+ flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size,
+ tempstr ? tempstr : "?");
free(tempstr);
}
diff --git a/flashrom.c b/flashrom.c
index 3e6e0fb..e14516f 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -65,6 +65,17 @@
*/
static bool may_register_shutdown = false;
+static struct bus_type_info {
+ enum chipbustype type;
+ const char *name;
+} bustypes[] = {
+ { BUS_PARALLEL, "Parallel, " },
+ { BUS_LPC, "LPC, " },
+ { BUS_FWH, "FWH, " },
+ { BUS_SPI, "SPI, " },
+ { BUS_PROG, "Programmer-specific, " },
+};
+
/* Register a function to be executed on programmer shutdown.
* The advantage over atexit() is that you can supply a void pointer which will
* be used as parameter to the registered function upon programmer shutdown.
@@ -912,35 +923,44 @@
/*
* Return a string corresponding to the bustype parameter.
- * Memory is obtained with malloc() and must be freed with free() by the caller.
+ * Memory to store the string is allocated. The caller is responsible to free memory.
+ * If there is not enough memory remaining, then NULL is returned.
*/
char *flashbuses_to_text(enum chipbustype bustype)
{
- char *ret = calloc(1, 1);
+ char *ret, *ptr;
+
/*
* FIXME: Once all chipsets and flash chips have been updated, NONSPI
* will cease to exist and should be eliminated here as well.
*/
- if (bustype == BUS_NONSPI) {
- ret = strcat_realloc(ret, "Non-SPI, ");
- } else {
- if (bustype & BUS_PARALLEL)
- ret = strcat_realloc(ret, "Parallel, ");
- if (bustype & BUS_LPC)
- ret = strcat_realloc(ret, "LPC, ");
- if (bustype & BUS_FWH)
- ret = strcat_realloc(ret, "FWH, ");
- if (bustype & BUS_SPI)
- ret = strcat_realloc(ret, "SPI, ");
- if (bustype & BUS_PROG)
- ret = strcat_realloc(ret, "Programmer-specific, ");
- if (bustype == BUS_NONE)
- ret = strcat_realloc(ret, "None, ");
+ if (bustype == BUS_NONSPI)
+ return strdup("Non-SPI");
+ if (bustype == BUS_NONE)
+ return strdup("None");
+
+ ret = calloc(1, 1);
+ if (!ret)
+ return NULL;
+
+ for (unsigned int i = 0; i < ARRAY_SIZE(bustypes); i++)
+ {
+ if (bustype & bustypes[i].type) {
+ ptr = strcat_realloc(ret, bustypes[i].name);
+ if (!ptr) {
+ free(ret);
+ return NULL;
+ }
+ ret = ptr;
+ }
}
+
/* Kill last comma. */
ret[strlen(ret) - 2] = '\0';
- ret = realloc(ret, strlen(ret) + 1);
- return ret;
+ ptr = realloc(ret, strlen(ret) + 1);
+ if (!ptr)
+ free(ret);
+ return ptr;
}
static int init_default_layout(struct flashctx *flash)
@@ -1165,7 +1185,7 @@
tmp = flashbuses_to_text(flash->chip->bustype);
msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) ", force ? "Assuming" : "Found",
- flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp);
+ flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp ? tmp : "?");
free(tmp);
if (master_uses_physmap(mst))
msg_cinfo("mapped at physical address 0x%0*" PRIxPTR ".\n",
diff --git a/print.c b/print.c
index 98d3109..0caa0da 100644
--- a/print.c
+++ b/print.c
@@ -103,6 +103,10 @@
} while (1);
s = flashbuses_to_text(chip->bustype);
+ if (s == NULL) {
+ msg_gerr("Out of memory!\n");
+ return 1;
+ }
maxtypelen = max(maxtypelen, strlen(s));
free(s);
}
@@ -265,6 +269,12 @@
msg_ginfo(" ");
s = flashbuses_to_text(chip->bustype);
+ if (s == NULL) {
+ msg_gerr("Out of memory!\n");
+ free(ven);
+ free(dev);
+ return 1;
+ }
msg_ginfo("%s", s);
for (i = strlen(s); i < maxtypelen; i++)
msg_ginfo(" ");
diff --git a/print_wiki.c b/print_wiki.c
index 2c8c109..a0cade9 100644
--- a/print_wiki.c
+++ b/print_wiki.c
@@ -325,7 +325,7 @@
"|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}"
"|| %s || %s\n",
(c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
- f->total_size, s,
+ f->total_size, s ? s : "?",
test_state_to_template(f->tested.probe),
test_state_to_template(f->tested.read),
test_state_to_template(f->tested.erase),
--
To view, visit https://review.coreboot.org/c/flashrom/+/73039
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c
Gerrit-Change-Number: 73039
Gerrit-PatchSet: 8
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Anastasia Klimchuk, Alexander Goncharov.
Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 )
Change subject: ni845x_spi: refactor singleton states into reentrant pattern
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> Yes, I remember that thread on the mailing list, I responded into the thread but let's talk here as […]
Well I need to make a few patches before testing it: the NI-845x detection looks to be broken and some warnings trigered build failure.
--
To view, visit https://review.coreboot.org/c/flashrom/+/72160
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484
Gerrit-Change-Number: 72160
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-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Mon, 24 Apr 2023 11:23:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com>
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/+/74537 )
Change subject: usb_device.c: remove LIBUSB() wrapper around call that may fail
......................................................................
usb_device.c: remove LIBUSB() wrapper around call that may fail
The libusb_detach_kernel_driver() call may return
LIBUSB_ERROR_NOT_FOUND, which should not be treated as an error.
Wrapping the call in LIBUSB() caused the error code to be transformed by
LIBUSB_ERROR(), so LIBUSB_ERROR_NOT_FOUND was not recognized at the call
site and was treated as a real error.
BUG=b:278635575
TEST=flashrom -p raiden_debug_spi:target=AP
BRANCH=none
Change-Id: I38e4642bcbddaf3f37821093f6b919806134ed7b
Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/74537
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Thomas Heijligen <src(a)posteo.de>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M usb_device.c
1 file changed, 27 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Thomas Heijligen: Looks good to me, approved
Edward O'Callaghan: Looks good to me, approved
diff --git a/usb_device.c b/usb_device.c
index c9c8d92..0c8e3e2 100644
--- a/usb_device.c
+++ b/usb_device.c
@@ -342,8 +342,8 @@
}
}
- ret = LIBUSB(libusb_detach_kernel_driver(device->handle,
- device->interface_descriptor->bInterfaceNumber));
+ ret = libusb_detach_kernel_driver(device->handle,
+ device->interface_descriptor->bInterfaceNumber);
if (ret != 0 && ret != LIBUSB_ERROR_NOT_FOUND && ret != LIBUSB_ERROR_NOT_SUPPORTED) {
msg_perr("Cannot detach the existing usb driver. %s\n",
libusb_error_name(ret));
--
To view, visit https://review.coreboot.org/c/flashrom/+/74537
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I38e4642bcbddaf3f37821093f6b919806134ed7b
Gerrit-Change-Number: 74537
Gerrit-PatchSet: 2
Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.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