Attention is currently required from: Subrata Banik.
Edward O'Callaghan 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 17:
(1 comment)
Patchset:
PS17:
> I know I can react much too harsh sometimes (it's just frustrating to see […]
Thanks Nico,
I just wanted to follow up here quickly to ensure that messaging does not get mixed up and this is going to get followed up on! Tomorrow is some company-wide day off.
The short answer just for the moment is that Subrata and I were discussing today if a possible unit-test using HECI command set that could perhaps reliably exploit the issue and how it is remediated by this patch can be contrived to make this more concrete.
I also followed up with Anastasia to help assist in making these changes smoother and she has provided some very useful insight to.
More to come but I just wanted to follow up with something here ASAP. Leaving as unresolved.
--
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: 17
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Alex Levin <levinale(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Damien Zammit
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: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 09:37:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Light.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62724 )
Change subject: pony_spi.c: Fixed memory leak in function pony_init_spi
......................................................................
Patch Set 1:
(6 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62724/comment/9628eccf_f3b982d7
PS1, Line 9: Memory leaked was caused as data variable wasn't deallocated in some error cases where
: the function returned without deallocatiing it.
Please wrap commit message lines at 72 characters, c.f. https://flashrom.org/Development_Guidelines#Commit_message
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/62724/comment/8a9dfab8_dbaf898a
PS1, Line 164: free(data);
We use tabs for indentation, and a tab is 8 characters wide. Please refer to https://flashrom.org/Development_Guidelines#Coding_style
Besides the indentation, this is correct. The shutdown function hasn't been registered (see comment on line 167), so `data` leaks.
https://review.coreboot.org/c/flashrom/+/62724/comment/7260c2c7_1e1d0a77
PS1, Line 167:
Once execution reaches this point, we know that `register_shutdown(pony_spi_shutdown, data)` has been called successfully. This means flashrom will automatically execute the `pony_spi_shutdown()` function before exiting, which in turn calls `free(data)`. So, there's no need to `free(data)` from this point onwards.
https://review.coreboot.org/c/flashrom/+/62724/comment/b28a00e2_9407e9d0
PS1, Line 182: free(data);
This isn't needed because the shutdown function has been registered, which will run `free(data)` already. Adding `free(data)` here would result in a double-free bug.
https://review.coreboot.org/c/flashrom/+/62724/comment/63515572_6b943e63
PS1, Line 248: free(data);
This isn't needed either, see my comment on line 182.
https://review.coreboot.org/c/flashrom/+/62724/comment/c1f1bcca_174a24a2
PS1, Line 255: free(data);
This won't work. The `register_spi_bitbang_master()` function stores a copy of the `data` pointer and uses it as argument for the `spi_data` parameter of the functions referenced by the `bitbang_spi_master_pony` struct. The memory is released by the `pony_spi_shutdown()` function.
--
To view, visit https://review.coreboot.org/c/flashrom/+/62724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7910db94f63693e7f131836d4963e88cfdbec301
Gerrit-Change-Number: 62724
Gerrit-PatchSet: 1
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
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: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 09:34:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Light.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62724 )
Change subject: pony_spi.c: Fixed memory leak in function pony_init_spi
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62724/comment/a3f432a9_bf62dac3
PS1, Line 7: Fixed
Imperative mood: Fix
--
To view, visit https://review.coreboot.org/c/flashrom/+/62724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7910db94f63693e7f131836d4963e88cfdbec301
Gerrit-Change-Number: 62724
Gerrit-PatchSet: 1
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 09:25:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Light.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62725 )
Change subject: libflashrom.c: fixed unintialized value passed to function
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62725/comment/482e8d14_eed9b097
PS1, Line 7: fixed
Fix
--
To view, visit https://review.coreboot.org/c/flashrom/+/62725
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iacbd7bf9cdf897cc2a732c1dc6568845a4ab804d
Gerrit-Change-Number: 62725
Gerrit-PatchSet: 1
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 09:24:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Light.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62726 )
Change subject: serprog.c: Fixed passed null value when expectingn nonnull
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62726/comment/0c10b7ac_e09a5a2c
PS1, Line 7: Fixed
Fix
--
To view, visit https://review.coreboot.org/c/flashrom/+/62726
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I850123237e328f9548ba7f77a01888be2cbc9e7b
Gerrit-Change-Number: 62726
Gerrit-PatchSet: 1
Gerrit-Owner: Light <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Light <aarya.chaumal(a)gmail.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 09:24:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment