- flashrom-gerrit - mail.coreboot.org

[L] Change in flashrom[main]: cli_client: Add rpmc command support
by Anastasia Klimchuk (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Matti Finder. Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) Change subject: cli_client: Add rpmc command support ...................................................................... Patch Set 8: (1 comment) File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84840/comment/5c115fde_a533ae5c?us… : PS8, Line 35: Adding support for RPMC commands as specified by JESD260 to the cli_client. Main Let's give this paragraph its own header, it's a big deal: > RPMC support added (works via SFDP detection) Or something like that, you can compose another header title -- To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 8 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 11:27:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in flashrom[main]: cli_client: Add rpmc command support
by Anastasia Klimchuk (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Matti Finder. Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) The change is no longer submittable: All-Comments-Resolved is unsatisfied now. Change subject: cli_client: Add rpmc command support ...................................................................... Patch Set 8: (4 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/ec662565_04199d24?us… : PS8, Line 1272: any_rpmc_op Would it ever be needed to set multiple commands at the same time? And what would that mean? If not maybe you can check here (or few lines below where I added another comment on the same topic) that only one command is set. https://review.coreboot.org/c/flashrom/+/84840/comment/1636a7d8_ea803a77?us… : PS8, Line 1289: (options.enable_wp && options.disable_wp) Would it make sense to add few checks similar to this, what do you think? Although I see that code goes through `else if`, so only one command would actually be processed. But it might prevent confusion if someone adds more than one command line option (maybe by accident) and then we silently run only one. File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/84840/comment/54768394_c7686e70?us… : PS8, Line 334: If your chip is detected correctly but the feature is not enabled, try using : ``-c "SFDP-capable chip"`` for automatic feature detection. I want to think how to maybe rephrase it to be more clear. We do have a situation, when flashchip has a definition, and it has a RPMC supported (but it won't work from definition), the error message, as I understand form previous patch, will be `Flash hardening is not supported on this chip, aborting.` It might be not entirely clear for the user that this is the same as manpage (currently) says `If your chip is detected correctly but the feature is not enabled`, because "not enabled" and "not supported" is not exact same thing. Also if RPMC can be detected and works reliably via SFDP - maybe that can be the official way to use it? What happens if you need to run several commands over the same chip: for example 1) read 2) get-rpmc-status 3) write Can you do 1 with classic probing, then 2 with -c "SFDP-capable chip" and then 3 again with classic probing, over the same chip? Would it work like this? I wrote a long comment, but what I am trying to figure out is a perfect text for manpage, so that every user reads and immediately everything is clear. File include/flash.h: https://review.coreboot.org/c/flashrom/+/84840/comment/0861c592_375be5d9?us… : PS4, Line 581: rpmc_ctx Oh this is such a good topic. For this patch, I will maybe just think a bit more about the manpage description, so that it is crystal clear. > I think for vendors that still have enough ids to give special ones to rpmc capable devices, we can just or the FEATURE_FLASH_HARDENING bit and set the struct rpmc_ctx values accordingly. From the recent experience, the IDs repeat. Several models added recently and the pair of models with/without rpmc feature share the same ID. > But I'm not sure how the future of flashrom should look like. If maintaining a complicated flashchips.c is worth it, when most of the information can also be parsed from sfdp pages (for newer flashchips). I think parsing info from sfdp pages, for newer flashchips, is how future should look like. That's a lot of work for sure, but you know what's so amazing: we have been just recently discussing the topic of sfdp (it came up from repeated IDs) and now boom you sent your contribution, and this is going right in the direction of future! Thank you so much :) -- To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 8 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 11:05:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Matti Finder <matti.finder(a)gmail.com>

1 0

[XS] Change in flashrom[main]: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID
by Michał Żygowski (Code Review) 04 Nov '24

04 Nov '24

Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84987?usp=email ) Change subject: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID ...................................................................... chipset_enable.c: Add TGL chipset detection based on SPI PCI ID Add detection of Tiger Point chipsets based on SPI controller PCI ID. Current detection is based on ESPI PCI ID only which limits the flashrom operability to 2 out of many chipset variants. TEST=Read flash on a platform with Intel Corporation Tiger Lake-LP SPI Controller [8086:a0a4] and ISA bridge [0601]: Intel Corporation Device [8086:a088] ESPI device. Change-Id: Ie6859d81157760ca03fe34ba5ac311eba5a7c46b Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> --- M chipset_enable.c 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/84987/1 diff --git a/chipset_enable.c b/chipset_enable.c index 2adc425..ed53702 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2106,6 +2106,8 @@ {0x8086, 0x9d84, B_S, DEP, "Intel", "Cannon Lake U Premium", enable_flash_pch300}, {0x8086, 0x0284, B_S, DEP, "Intel", "Comet Lake U Premium", enable_flash_pch400}, {0x8086, 0x0285, B_S, DEP, "Intel", "Comet Lake U Base", enable_flash_pch400}, + {0x8086, 0xa0a4, B_S, DEP, "Intel", "Tiger Lake LP", enable_flash_pch500}, + {0x8086, 0x43a4, B_S, DEP, "Intel", "Tiger Lake H", enable_flash_pch500}, {0x8086, 0xa082, B_S, DEP, "Intel", "Tiger Lake U Premium", enable_flash_pch500}, {0x8086, 0xa088, B_S, DEP, "Intel", "Tiger Lake UP3", enable_flash_pch500}, {0x8086, 0xa141, B_S, NT, "Intel", "Sunrise Point Desktop Sample", enable_flash_pch100}, -- To view, visit https://review.coreboot.org/c/flashrom/+/84987?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ie6859d81157760ca03fe34ba5ac311eba5a7c46b Gerrit-Change-Number: 84987 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>

1 0

[L] Change in flashrom[main]: rpmc: add rpmc commands feature
by Anastasia Klimchuk (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Matti Finder. Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email ) Change subject: rpmc: add rpmc commands feature ...................................................................... Patch Set 4: (9 comments) Patchset: PS4: Matti, thank you so much for your patience! I made another round of comments, but probably the last one. There is one more thing, at the end when code is ready and all comments fixed, if you could re-test it again, on the latest version of the patch? I am thinking about two scenarios to test: 1) chip model that supports rpmc, and handled as "SFDP-capable chip" 2) chip model that does not support rpmc, but is recognised and handled as a "SFDP-capable chip": to make sure baseline case is not broken. File include/rpmc.h: https://review.coreboot.org/c/flashrom/+/84934/comment/29424b16_61412a25?us… : PS4, Line 90: struct flashrom_flashctx * flash Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashctx *flash). As an example is sfdp.c. I see that you used space between * and <name> consistently in all your new function declarations and definitions... but I need to ask you to change to align with our existing code style. Thank you so much! (hope you are not too upset) File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/76c3bb63_6891df63?us… : PS3, Line 224: return -1; > My original of how to handle the return values wasn't that good, and just lead to a lot of questions […] Enum of return codes is good idea! Now it's all nice and readable. File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/153afc03_2820beee?us… : PS4, Line 117: usleep(flash->chip->rpmc_ctx.polling_long_delay_write_counter_us) Peter, I wanted to check, is this the right way to sleep? Are we supposed to be using `programmer_delay` everywhere? https://review.coreboot.org/c/flashrom/+/84934/comment/6f8648e0_a80bd1fc?us… : PS4, Line 131: if (res != RPMC_SUCCESS) { : return res; : } Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body is one-line. And also above, when you call usleep https://review.coreboot.org/c/flashrom/+/84934/comment/2fd0acbb_c9d89e30?us… : PS4, Line 157: if (keyfile == NULL || read_buf_from_file(key, RPMC_HMAC_KEY_LENGTH, keyfile) != 0) { : return RPMC_ERROR_KEY_READ; : } And here, { } not needed, 1-line body https://review.coreboot.org/c/flashrom/+/84934/comment/02e8f632_a82d2ae4?us… : PS4, Line 497: no error string defined for this value Maybe > Unknown Internal Error: Error code not recognised (because, the fact that we don't have a string is not a problem by itself, the problem we don't know what happened) File sfdp.c: https://review.coreboot.org/c/flashrom/+/84934/comment/062d90aa_49653796?us… : PS3, Line 449: msg_cdbg("The chip contains an unknown " : "version of the JEDEC flash " : "parameters table, skipping it.\n"); > Done Thank you! File sfdp.c: https://review.coreboot.org/c/flashrom/+/84934/comment/acfe3672_a84de7bb?us… : PS4, Line 457: if (sfdp_fill_flash(flash->chip, tbuf, len) == 0) { : ret = 1; : } I have a question: if for some reasons `sfdp_fill_flash` fails (i.e. returns non-zero), would RPMC feature work? (or any other feature). It seems like having `sfdp_fill_flash` successful is a pre-condition for doing anything with the chip? If we are handling the chip as generic SFDP-chip. I am asking because right now failure of `sfdp_fill_flash` does not stop parsing the headers, and possibly below in line 468 `ret = 1` will mark probing successful, but it shouldn't be? Maybe this should return error straight away if `sfdp_fill_flash` fails? Previously it wasn't important because only page 0 of headers was parsed. -- To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Gerrit-Change-Number: 84934 Gerrit-PatchSet: 4 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 09:07:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Matti Finder <matti.finder(a)gmail.com>

1 0

[L] Change in flashrom[main]: cli_client: Add rpmc command support
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) Change subject: cli_client: Add rpmc command support ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 8 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:16:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[L] Change in flashrom[main]: rpmc: add rpmc commands feature
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email ) Change subject: rpmc: add rpmc commands feature ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File include/rpmc.h: https://review.coreboot.org/c/flashrom/+/84934/comment/42f3d851_50f9be22?us… : PS4, Line 71: dependant ```suggestion * Some bits might exclude others or their meaning might be dependent ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Gerrit-Change-Number: 84934 Gerrit-PatchSet: 4 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:13:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: doc: Add few sections to recent development doc
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84954?usp=email ) Change subject: doc: Add few sections to recent development doc ...................................................................... Patch Set 1: (9 comments) File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84954/comment/af96bb2b_72e4ff57?us… : PS1, Line 70: Programmers ```suggestion Programmer updates ``` https://review.coreboot.org/c/flashrom/+/84954/comment/0d7aebfb_a09f5c3a?us… : PS1, Line 73: * ch347_spi: Add spi clock frequency selection ```suggestion * ch347_spi: Add spi clock frequency selection ("spispeed" option) ``` https://review.coreboot.org/c/flashrom/+/84954/comment/cd0a62c7_0cdea139?us… : PS1, Line 78: Bugs fixes ```suggestion Bugs fixed ``` I think it'll be more useful for these to describe what can go wrong, and link to the change. Users want to know how they might have been affected by the bugs that have been fixed (which isn't entirely clear with how these are written now), and distributors might want to know which patches they should backport. Suggestions inline. https://review.coreboot.org/c/flashrom/+/84954/comment/f08e31f3_5a52b9db?us… : PS1, Line 81: * Ensure verify operation completed in full if chip memory modified : : Post-cleanup after processing unaligned region for the case when end : region needs to be extended to align with erase block. Writing was : done correctly, but post-processing of newcontents could cause : one-off offset at the end of the region, which would make : verification appear false-negative ```suggestion * Modified bytes would sometimes not be verified after writing In some situations an off-by-one error would cause the last byte of memory that was modified by an operation to not be verified, which could prevent some erase or write errors from being detected. Fixed by https://review.coreboot.org/c/flashrom/+/84078. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/bef7a071_86818501?us… : PS1, Line 89: * erasure_layout: Fix init_eraseblock segmentation fault : : Fix a segmentation fault that is caused by accessing an invalid : "subedata" pointer on the last iteration of the init_eraseblock loop. : Instead, short circuit the loop condition to check the sub block index : first, and do not access the invalid pointer if it is the last sub : block. : : Issue was encountered in OpenBSD. ```suggestion * Possible crashes while preparing erase operations An out-of-bounds memory read was found in the algorithm that selects methods to erase memory, which could cause flashrom to crash. This issue was first introduced in release 1.4, and crashes were observed when running on OpenBSD. See https://ticket.coreboot.org/issues/555, and fixed by https://review.coreboot.org/c/flashrom/+/84234. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/ad7de33d_dc526450?us… : PS1, Line 99: * Fix FEATURE_NO_ERASE chips and add test for them : : https://ticket.coreboot.org/issues/553 ```suggestion * Crash when attempting to erase FEATURE_NO_ERASE chips When attempting to erase a chip that doesn't need to be erased before being written, flashrom could attempt to read through a null pointer and crash. The only supported chip that is affected is the M95M02 EEPROM. See https://ticket.coreboot.org/issues/553, and fixed by https://review.coreboot.org/c/flashrom/+/84203. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/12fcf664_f4f55eb0?us… : PS1, Line 103: * build: never install cmocka : : https://ticket.coreboot.org/issues/561 ```suggestion * install failures related to libcmocka In some configurations, the install target provided by the buildsystem (like meson install) would fail to execute successfully due to a missing libcmocka file. This is fixed by not installing libcmocka, because it is a third-party library used by flashrom only for testing. See https://ticket.coreboot.org/issues/561, and fixed by https://review.coreboot.org/c/flashrom/+/84557. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/2d2a7ed4_3cd7c9a8?us… : PS1, Line 107: * ichspi: Probe opcode in POSSIBLE_OPCODES[] as well : : https://ticket.coreboot.org/issues/556 ```suggestion * Excess erase of automatically-probed chips on Intel chipsets When erasing some chips using the ichspi programmer (for Intel ICH chipsets), the entire chip would be erased and rewritten even when the hardware supported erasing smaller blocks, causing operations to take longer to complete and negatively impacting chip longevity. This issue was first introduced in version 1.4. See https://ticket.coreboot.org/issues/556, and fixed by https://review.coreboot.org/c/flashrom/+/84253. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/bfbc5ac4_6d937ca3?us… : PS1, Line 111: * erasure_layout: Erase larger block only when all sub-block need erase : : * erase/write: Deselect all smaller blocks when large block is selected These are two changes relating to the same issue, so I think they make sense to combine. ```suggestion * Unnecessary erases When erasing parts of a memory, some blocks could be erased and rewritten unnecessarily or erased multiple times which could hurt the longevity of the memory chip. This behavior was introduced in version 1.4. Fixed by https://review.coreboot.org/c/flashrom/+/84614 and https://review.coreboot.org/c/flashrom/+/84686. ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84954?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d Gerrit-Change-Number: 84954 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:09:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: doc: Change link from gitiles to github
by Peter Marheine (Code Review) 03 Nov '24

03 Nov '24

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84960?usp=email ) Change subject: doc: Change link from gitiles to github ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84960?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9103e5199bdf1b65fa3136aa01259a85e788a251 Gerrit-Change-Number: 84960 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Nov 2024 22:32:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 03 Nov '24

03 Nov '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email ) Change subject: Display progress for what is actually erased/written ...................................................................... Display progress for what is actually erased/written The patch updates calculation of total length for the operation which is displayed with progress. The reason is: even if, for example the whole chip erase or write was requested, the actual length of bytes modified can be less than whole chip size (areas which already have expected content, are skipped). Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84439 Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M doc/classic_cli_manpage.rst M flashrom.c M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 113 insertions(+), 9 deletions(-) Approvals: build bot (Jenkins): Verified Sergii Dmytruk: Looks good to me, approved diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index f0fd681..f0879e4 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -317,7 +317,7 @@ **--progress** - [Experimental feature] Show progress percentage of operations on the standard output. + Show progress percentage of operations on the standard output. **--sacrifice-ratio <ratio>** Fraction (as a percentage, 0-50) of an erase block that may be erased even if unmodified. diff --git a/flashrom.c b/flashrom.c index 88fa5d9..4ae9390 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1243,6 +1243,56 @@ init_progress(flashctx, stage, total); } + +static void setup_progress_from_layout_and_diff(struct flashctx *flashctx, + const void *have, + const void *want, + enum flashrom_progress_stage stage) +{ + if (!flashctx->progress_callback) + return; + + const struct flashrom_layout *flash_layout = get_layout(flashctx); + const size_t page_size = flashctx->chip->page_size; + + size_t total = 0; + + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + const struct flash_region *region = &entry->region; + + if (stage == FLASHROM_PROGRESS_ERASE) { + size_t offset; + for (offset = region->start; offset <= region->end; offset += page_size) { + const size_t len = min(page_size, region->end + 1 - offset); + + if (need_erase(have, want, len, flashctx->chip->gran, ERASED_VALUE(flashctx))) + total += len; + } + } + + if (stage == FLASHROM_PROGRESS_WRITE) { + unsigned int start = region->start; + unsigned int len; + while ((len = get_next_write(have + start, want + start, + region->end + 1 - start, &start, flashctx->chip->gran))) { + start += len; + total += len; + } + + if (flashctx->chip->feature_bits & FEATURE_NO_ERASE) + /* For chips with FEATURE_NO_ERASE erase op is running as write under the hood. + * So typical write, which usually consists of erasing and then writing, + * would be writing and then writing again. The planned total length for the + * progress indicator for write is double. */ + total *= 2; + } + } + + init_progress(flashctx, stage, total); +} + + /** * @brief Reads the included layout regions into a buffer. * @@ -1374,7 +1424,7 @@ memset(newcontents, ERASED_VALUE(flashctx), flash_size); setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_ERASE); const struct flashrom_layout *const flash_layout = get_layout(flashctx); const struct romentry *entry = NULL; @@ -1413,8 +1463,8 @@ } setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_WRITE); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_WRITE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_ERASE); const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { diff --git a/tests/chip.c b/tests/chip.c index ac60663..25bb8db 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -47,7 +47,8 @@ }; struct progress_user_data { - size_t last_seen; /* % of progress last reported, to be asserted in the progress callback. */ + /* % of progress last reported for each operation, to be asserted in the progress callback. */ + size_t last_seen[FLASHROM_PROGRESS_NR]; }; static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) @@ -86,13 +87,14 @@ printf("Progress started for stage %d, initial callback call\n", flash->progress_state->stage); } else { /* Progress cannot go backwards. */ - assert_true(flash->progress_state->current >= progress_user_data->last_seen); + assert_true(flash->progress_state->current >= progress_user_data->last_seen[flash->progress_state->stage]); } - if (flash->progress_state->current >= flash->progress_state->total) - printf("Progress complete for stage %d, final callback call\n", flash->progress_state->stage); + if (flash->progress_state->current >= flash->progress_state->total - 1) + printf("Progress almost complete for stage %d, current %ld, total %ld\n", + flash->progress_state->stage, flash->progress_state->current, flash->progress_state->total); - progress_user_data->last_seen = flash->progress_state->current; + progress_user_data->last_seen[flash->progress_state->stage] = flash->progress_state->current; } static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, @@ -144,6 +146,7 @@ .tested = TEST_OK_PREW, .read = TEST_READ_INJECTOR, .write = TEST_WRITE_INJECTOR, + .page_size = 256, .block_erasers = {{ /* All blocks within total size of the chip. */ @@ -586,6 +589,55 @@ free(newcontents); } +void write_chip_feature_no_erase_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + + /* + * Tricking the dummyflasher by asking to emulate W25Q128FV but giving to it + * mock chip with FEATURE_NO_ERASE. + * As long as chip size is the same, this is fine. + */ + struct flashchip mock_chip = chip_no_erase; + const char *param_dup = "bus=spi,emulate=W25Q128FV"; + + setup_chip(&flashctx, &layout, &mock_chip, param_dup, &chip_io); + + /* See comment in write_chip_test_success */ + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + printf("Write chip operation started.\n"); + assert_int_equal(0, read_buf_from_file(newcontents, size, filename)); + assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, size, NULL)); + assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, size)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + + void write_nonaligned_region_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/tests.c b/tests/tests.c index 00b4665..72b4868 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,7 @@ cmocka_unit_test(write_chip_with_progress), cmocka_unit_test(write_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_feature_no_erase), + cmocka_unit_test(write_chip_feature_no_erase_with_progress), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), cmocka_unit_test(verify_chip_test_success), cmocka_unit_test(verify_chip_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index dba33fb..85c4f52 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -91,6 +91,7 @@ void write_chip_with_progress(void **state); void write_chip_with_dummyflasher_test_success(void **state); void write_chip_feature_no_erase(void **state); +void write_chip_feature_no_erase_with_progress(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state); void verify_chip_test_success(void **state); void verify_chip_with_dummyflasher_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 18 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: Split cli_output declarations to a separate header.
by Antonio Vázquez Blanco (Code Review) 03 Nov '24

03 Nov '24

Antonio Vázquez Blanco has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84985?usp=email ) Change subject: Split cli_output declarations to a separate header. ...................................................................... Split cli_output declarations to a separate header. This is a simple refactor that aims to simplify maintenance and to clarify file dependency inside the project. Currently, many declarations reside in flash.h making it difficult to really understand file dependency. Change-Id: I45d62bd219bdbed919788ae17a64aeb119a8aac4 Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> --- M cli_classic.c M cli_output.c A include/cli_output.h M include/flash.h 4 files changed, 25 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/84985/1 diff --git a/cli_classic.c b/cli_classic.c index 3343438..1669076 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -25,6 +25,7 @@ #include <stdbool.h> #include <stdlib.h> #include <cli_classic.h> +#include "cli_output.h" #include "flash.h" #include "flashchips.h" #include "fmap.h" diff --git a/cli_output.c b/cli_output.c index e5b829a..cf96bc0 100644 --- a/cli_output.c +++ b/cli_output.c @@ -15,11 +15,12 @@ * GNU General Public License for more details. */ +#include "cli_output.h" + #include <stdio.h> #include <stdarg.h> #include <string.h> #include <errno.h> -#include "flash.h" enum flashrom_log_level verbose_screen = FLASHROM_MSG_INFO; enum flashrom_log_level verbose_logfile = FLASHROM_MSG_DEBUG2; diff --git a/include/cli_output.h b/include/cli_output.h new file mode 100644 index 0000000..3f6107a --- /dev/null +++ b/include/cli_output.h @@ -0,0 +1,21 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-FileCopyrightText: 2024 Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#ifndef __CLI_OUTPUT_H__ +#define __CLI_OUTPUT_H__ + +#include <stdarg.h> +#include "flash.h" + +extern enum flashrom_log_level verbose_screen; +extern enum flashrom_log_level verbose_logfile; +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap); +void flashrom_progress_cb(struct flashrom_flashctx *flashctx); + +#endif /* __CLI_OUTPUT_H__ */ diff --git a/include/flash.h b/include/flash.h index d0e55af..f0b7f3b 100644 --- a/include/flash.h +++ b/include/flash.h @@ -680,14 +680,7 @@ /* cli_common.c */ void print_chip_support_status(const struct flashchip *chip); -/* cli_output.c */ -extern enum flashrom_log_level verbose_screen; -extern enum flashrom_log_level verbose_logfile; -int open_logfile(const char * const filename); -int close_logfile(void); -void start_logging(void); -int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap); -void flashrom_progress_cb(struct flashrom_flashctx *flashctx); +/* libflashrom.c */ /* Let gcc and clang check for correct printf-style format strings. */ int print(enum flashrom_log_level level, const char *fmt, ...) #ifdef __MINGW32__ -- To view, visit https://review.coreboot.org/c/flashrom/+/84985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I45d62bd219bdbed919788ae17a64aeb119a8aac4 Gerrit-Change-Number: 84985 Gerrit-PatchSet: 1 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit