Attention is currently required from: Anastasia Klimchuk, Edward O'Callaghan. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/75646?usp=email ) Change subject: Replace all flashchips accesses with get functions ...................................................................... Patch Set 5: (15 comments) Patchset: PS5:

I realise this patch was a intermediate step so the comments are just how it should probably look fo […] Done

File flashchips.c: https://review.coreboot.org/c/flashrom/+/75646/comment/31223b9d_eaf0ba69 : PS5, Line 28: * Temporarily, this file is sorted alphabetically by vendor and name to : * assist with merging the Chromium fork of flashrom.

Can we remove this comment by now? (maybe in a separate patch). […] Done

https://review.coreboot.org/c/flashrom/+/75646/comment/afb19e6f_1f31d463 : PS5, Line 34: internal_

I agree that "internal" almost always means internal programmer. It is flashromspeak :) […] Done

https://review.coreboot.org/c/flashrom/+/75646/comment/79a33fb2_da183bd0 : PS5, Line 20844: get_flashchips(void)

Where would the caller get the pointer to flashchip db? and why the caller needs to have the pointer […] Any code that needs to access the supported flashchips would use this function instead of accessing the array directly. This allows to later load the database of supported flashchips dynamically rather than hard coding it in the binary.

https://review.coreboot.org/c/flashrom/+/75646/comment/b58c418a_34e7a568 : PS5, Line 20849: unsigned int

maybe so. it was unsigned int before, so i didn't change it. […] Done

File flashrom.c: https://review.coreboot.org/c/flashrom/+/75646/comment/217b37be_b2fa90d2 : PS5, Line 1099: return -1;

Maybe you can add `msg_gerr` before returning. […] Done

File ichspi.c: https://review.coreboot.org/c/flashrom/+/75646/comment/cc185715_06991b98 : PS5, Line 1469: const struct flashchip *chip; : : struct flashchip *flashchips=get_flashchips(); : : if (!flashchips) : return NULL;

very clever. and doesn't check the return value. […] Done

File libflashrom.c: https://review.coreboot.org/c/flashrom/+/75646/comment/ad9a70ce_cf7e3c48 : PS5, Line 95: unsigned int

shouldn't then i downstairs also be size_t? not against this, but it seems that is a larger consiste […] Done

File print.c: https://review.coreboot.org/c/flashrom/+/75646/comment/e4138b87_af71dddb : PS5, Line 56: const struct flashchip *chip;

delete This won't work because we walk the whole array twice and chip is modified the first time.

https://review.coreboot.org/c/flashrom/+/75646/comment/1cf56450_5101611f : PS5, Line 62: flashchips

chip ditto

https://review.coreboot.org/c/flashrom/+/75646/comment/9a2e313e_0260c79b : PS5, Line 65: chip = flashchips

delete ditto.

File print_wiki.c: https://review.coreboot.org/c/flashrom/+/75646/comment/a2bdff40_14e6d3e5 : PS5, Line 294: flashchips

get_flashchips() Given that get_flashchips() might do more in the future, it doesn't make sense to optimize two lines of code saved by making two function calls in my opinion.

https://review.coreboot.org/c/flashrom/+/75646/comment/c12e89f3_e7f98e63 : PS5, Line 310: flashchips

get_flashchips() ditto

File tests/selfcheck.c: https://review.coreboot.org/c/flashrom/+/75646/comment/94c28057_65adc2ea : PS5, Line 80: /* unused */

But it is used two lines below? Done

https://review.coreboot.org/c/flashrom/+/75646/comment/83be1367_b595402e : PS5, Line 83: flashchips = get_flashchips(); : flashchips_size = get_flashchips_size();

Can this be immediately initialised? (and the same above). This is what non-test code is doing. Done

-- To view, visit https://review.coreboot.org/c/flashrom/+/75646?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ibc89e32c83975e01c958b8cf0d11dad73c461a53 Gerrit-Change-Number: 75646 Gerrit-PatchSet: 5 Gerrit-Owner: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Comment-Date: Fri, 26 Jan 2024 21:57:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Stefan Reinauer <stefan.reinauer@coreboot.org> Comment-In-Reply-To: Edward O'Callaghan <quasisec@chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm@chromium.org> Gerrit-MessageType: comment