eshankelkar(a)galorithm.com has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71791 )
Change subject: sfdp.c : Initializing hbuf to avoid warnings generated by scan-build
......................................................................
sfdp.c : Initializing hbuf to avoid warnings generated by scan-build
scan-build uses the clang analyzer which is giving warnings
about garbage/undefined values in hbuf assigned to member of hdrs[0]
if hbuf is not initialized.
Though the path of the control flow shown by it in its html output
cannot occur in reality(since it assumes that (nph+1)*8 is <= 0 i.e
nph<=-1 but later assumes 0 <= nph ) hence its a false positive.
Still initializing all bytes of hbuf to 0 leads to the two warnings
for sfdp.c (one for hbuf, one for tbuf) to go away.
Signed-off by : Eshan Kelkar <eshankelkar(a)galorithm.com>
Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b
---
M sfdp.c
1 file changed, 27 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/71791/1
diff --git a/sfdp.c b/sfdp.c
index 5eddb21..b5010d6 100644
--- a/sfdp.c
+++ b/sfdp.c
@@ -296,6 +296,12 @@
/* Fetch all parameter headers, even if we don't use them all (yet). */
hbuf = malloc((nph + 1) * 8);
+ uint8_t k;
+ for (i=0; i<=nph; i++){
+ for (k=0 ; k<=7 ; k++)
+ hbuf[i*8+k]=(uint8_t)0;
+ }
+
hdrs = malloc((nph + 1) * sizeof(*hdrs));
if (hbuf == NULL || hdrs == NULL ) {
msg_gerr("Out of memory!\n");
--
To view, visit https://review.coreboot.org/c/flashrom/+/71791
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6815e246b4fd225d1837cae6e7d2aa0236b48b1b
Gerrit-Change-Number: 71791
Gerrit-PatchSet: 1
Gerrit-Owner: eshankelkar(a)galorithm.com
Gerrit-MessageType: newchange
Attention is currently required from: Thomas Heijligen, Angel Pons.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71577 )
Change subject: internal.c: Move sio register to own object
......................................................................
Patch Set 2:
(1 comment)
File superio.c:
https://review.coreboot.org/c/flashrom/+/71577/comment/a75f571a_637f144b
PS1, Line 40: //probe_superio_smsc();
> Wow, this is dead code
Well let's get this in 'as-is' as it just moves around code. Cleanups can be separate commits.
--
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: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(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-Comment-Date: Tue, 10 Jan 2023 10:03:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Anastasia Klimchuk, Evan Benn.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 )
Change subject: tests/: Add non-aligned write within a region unit-test
......................................................................
Patch Set 2:
(16 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71659/comment/0901f4f6_097bdf43
PS2, Line 7: tests/: Add subregion alignment unit-test
> is a subregion a thing? […]
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/6ca559fc_d50b4381
PS2, Line 9: A written region that is sized below that of the erasure granularity
> I don't understand the bug from this description
should make more sense now given all the other fixes give context.
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/71659/comment/2dbdad01_75b44fb5
PS2, Line 110: va_list logfile_args;
> you dont need to va_copy because you only pass ap to vfprintf
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/f25d06f3_9a15fed8
PS2, Line 114: output_type = stderr;
> I don't know the motivation for printing the messages, so I also dont know the motivation for splitt […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/96e19d18_041e83d5
PS2, Line 116: if (level <= verbose_screen) {
> verbose_screen is part of `cli_output. […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/9f190440_a96060c8
PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly
> why do we need to fflush at all?
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/04957694_341b54a0
PS2, Line 444: void write_chip_subregion_with_dummyflasher_test_success(void **state) // XXX
> this is the first mention of a 'subregion' in flashrom, maybe there is an existing word you can choo […]
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/20aeae7f_05b0d69b
PS2, Line 458: struct flashchip mock_chip = chip_W25Q128_V;
> do you want to assert something about the size of the erasers here? as much for documentation as en […]
Well that should all be part of the W25Q128V chip spec this struct is defining. That sounds like another test to add to ensure the data structure is maintained or a comment above it. Either way, maybe out of scope here.
https://review.coreboot.org/c/flashrom/+/71659/comment/4c742e8a_f82500af
PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC
> this can be a const int instead of a define
well not quite in this case because reasons, however I added a comment.
https://review.coreboot.org/c/flashrom/+/71659/comment/5b635c7d_837de663
PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}.
> im not sure what this is telling me
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/2ba94eae_f9400a56
PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size);
> memsetting so calloc could be malloc
I prefer consistent use of calloc() that ensures heap is always zero. malloc nano optimisation for a uninit heap gains me nothing over the advantage of consistency.
https://review.coreboot.org/c/flashrom/+/71659/comment/05d64979_94ba7308
PS2, Line 477: flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false);
> suspect these flags are important what is being tested, but its not documented here or […]
Documenting those flags better is outside the scope of this change. However I agree they need better doxygen comments in libflashrom.h
https://review.coreboot.org/c/flashrom/+/71659/comment/e80af7bd_d27a532c
PS2, Line 478: flashrom_set_log_callback((flashrom_log_callback *)&unittest_print_cb);
> why are we setting up a print callback? We also dont seem to undo this at the end, which will be tri […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/9b3356db_91ef6bed
PS2, Line 502: printf("Subregion chip W op..\n");
> write the word
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/24fbe3ff_33366af9
PS2, Line 508: //flashrom_layout_set(&flashctx, NULL); // use default layout.
> the layout you use is the same as the default or NULL layout, does that not work?
No, it segfauls for some unknown reason outside the scope of the work here.
https://review.coreboot.org/c/flashrom/+/71659/comment/692915fe_1814f6a8
PS2, Line 514: assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, mock_chip_size));
> maybe documenting right here what the predicted bug is would be useful. […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/71659
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f
Gerrit-Change-Number: 71659
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Tue, 10 Jan 2023 00:43:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk.
Hello Sam McNally, build bot (Jenkins), Evan Benn,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/71659
to look at the new patch set (#3).
Change subject: tests/: Add non-aligned write within a region unit-test
......................................................................
tests/: Add non-aligned write within a region unit-test
A written region that is sized below that of the erasure granularity
can result in a incorrectly read region that does not include prior
content within the region before the write op. This was dealt with
in ChromeOS downstream by expanding out the read to match the erase
granularity however does not seem to impact upstream. Add a unit-test
to avoid regression as this is important behaviour to cover.
Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M tests/chip.c
M tests/tests.c
M tests/tests.h
3 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/71659/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/71659
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f
Gerrit-Change-Number: 71659
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Edward O'Callaghan, Angel Pons.
Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71742 )
Change subject: flashrom.c: Rewrite prepare_chip_access() 4BA enablement
......................................................................
Patch Set 1:
(2 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/71742/comment/e4a27f2c_8a507ddc
PS1, Line 1916: if (!spi_master_no_4ba_modes(flash)) {
I don't really understand this check - the comment on SPI_MASTER_NO_4BA_MODES says "Compatibility modes (..., 4BA mode switch) don't work" - would this flag mean the master has 4BA but the mode switch doesn't work?
I would assume it is not just missing 4BA mode since that would be the same as !spi_master_4ba(), there would probably be a reason for two distinct flags right? It looks like 'dediprog' could set both flags, I don't have any device to test that though.
Do you know what this flag means? It used to possibly enter/exit 4ba in this state depending on spi_chip_4ba()/spi_master_4ba().
https://review.coreboot.org/c/flashrom/+/71742/comment/a3bbedc4_6afea3fb
PS1, Line 1938: int ret = spi_master_4ba(flash) ? spi_enter_4ba(flash): spi_exit_4ba(flash);
It's not possible to reach spi_exit_4ba() here due to returning if spi_master_4ba() was false above.
I think the intent was that if the chip supports 4ba but doesn't require it (<= 16M), and master doesn't support 4ba, it would exit 4ba here. I'm not sure if that makes sense though, why would such a chip be in 4ba in the first place? Maybe if some buggy program changed it?
So I'm not sure if the answer is to delete the unreachable code or restore the disable path, what do you think?
--
To view, visit https://review.coreboot.org/c/flashrom/+/71742
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I10a426331cb2a3485e3dd786fa771e20870cbd84
Gerrit-Change-Number: 71742
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 09 Jan 2023 14:31:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71745 )
Change subject: parallel: Drop explicit fallback_chip_X boilerplate
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/71745
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If25c0048a07057aa72be6ffa8d8ad7f0a568dcf7
Gerrit-Change-Number: 71745
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Mon, 09 Jan 2023 01:35:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 )
Change subject: programmer.h: Guard against sending spi commands on non-spi mst
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/71269
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b
Gerrit-Change-Number: 71269
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 09 Jan 2023 01:26:43 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Stefan Reinauer.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71745 )
Change subject: parallel: Drop explicit fallback_chip_X boilerplate
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Stefan, can you test with your satasii.c case.
--
To view, visit https://review.coreboot.org/c/flashrom/+/71745
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If25c0048a07057aa72be6ffa8d8ad7f0a568dcf7
Gerrit-Change-Number: 71745
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Comment-Date: Mon, 09 Jan 2023 00:18:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment