- flashrom-gerrit - mail.coreboot.org

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

05 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 5: (1 comment) File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/67fd41d4_feda1e76?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_de […] `programmer_delay` seems like the right choice here, since we have a flashctx. -- 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: 5 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: Tue, 05 Nov 2024 03:37:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID
by Anastasia Klimchuk (Code Review) 05 Nov '24

05 Nov '24

Attention is currently required from: Michał Żygowski. Anastasia Klimchuk has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/flashrom/+/84987?usp=email ) Change subject: chipset_enable.c: Add TGL chipset detection based on SPI PCI ID ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: I didn't find a datasheet, but at least I found IDs 0xa0a4 and 0x43a4 on the internet as of being indeed Tiger Lake LP and Tiger Lake H :) And you tested (and you probably have a datasheet) -- 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: comment 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> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Comment-Date: Tue, 05 Nov 2024 01:39:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

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

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Matti Finder 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 5: (7 comments) File include/rpmc.h: https://review.coreboot.org/c/flashrom/+/84934/comment/08dcfa13_6490f47f?us… : PS4, Line 71: dependant > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84934/comment/78b96543_dbc24fb3?us… : PS4, Line 71: dependant > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84934/comment/dfb12815_7d20d2a9?us… : PS4, Line 90: struct flashrom_flashctx * flash > Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashc […] Done File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/5a8154a3_3ad5aaa0?us… : PS4, Line 131: if (res != RPMC_SUCCESS) { : return res; : } > Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body i […] Done https://review.coreboot.org/c/flashrom/+/84934/comment/1fce12c7_fee71ca7?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 Done https://review.coreboot.org/c/flashrom/+/84934/comment/e629f8c4_be003353?us… : PS4, Line 497: no error string defined for this value > Maybe […] Done File sfdp.c: https://review.coreboot.org/c/flashrom/+/84934/comment/f33b9fd7_264fc3c3?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). I don't know exactly but from looking at the code it only seems to set information about erase and write calls. > 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. Yes for safety and also consistency I agree. Failure to parse the rpmc page is no problem since the feature bit is only set when the function can't fail anymore. I have removed it's influence on the return value of the function. -- 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: 5 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Nov 2024 14:51:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

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

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine. Matti Finder 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 9: (4 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/64bc5425_ed573d5f?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? […] Before your question the `cli_classic_validate_singleop(&operation_specified);` before any of the options prevented multiple rpmc operations. But thanks to this question I realized that using multiple commands in one call is actually very usefull. Updating the hmac key is something that needs to be done at least after every chip reset. So calling --update-hmac-key and then immediatly after something like --get-counter is probably something a lot of users want to do. The commands don't get into the way of eachother so running multiple commands at after each other is possible. I have adjusted the order slightly to have --get-rpmc-status run last. https://review.coreboot.org/c/flashrom/+/84840/comment/cf31a42e_332e4a78?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? […] See also #1272. The else if is now gone so every commands gets run. I have also added a rpmc_root_key_file to the cli_opts struct so the commands don't get into the way of -w, -r etc.. Doing something like writing something to flash and then incrementing the counter is probably how the counters are meant to be used. So beeing able to run other commands together with them is mostly usefull. I can't imagine any other scenerio where running a rpmc command next to some other command is destructive, so in my opinion there are no other checks necessary. File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/84840/comment/47cc9cc7_9a3dc029?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. […] I have added a quote to the error message, this should make it better to understand. Running multiple commands together would currently either fail at the rpmc command or force the read and write to use SFDP-capable chip (which works perfectly in my testing). If the commands are run in seperate calls to flashrom (which read and write have to be anyways, since they mutually exclusive) everything works as normal, you just have to specify the chip manually for the rpmc command. I also had another idea for detection we could always poll the SFDP-capable chip and then silently use it, instead of the user specified one, for the rpmc commands. But that is something I would try in a different commit. File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84840/comment/ee5453a0_0d1068e6?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: […] Done -- 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: 9 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Nov 2024 14:37:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

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

04 Nov '24

Attention is currently required from: Matti Finder, Peter Marheine. Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84934?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins) Change subject: rpmc: add rpmc commands feature ...................................................................... rpmc: add rpmc commands feature Added optional support for all the commands specified in JESD260. Added a new optional dependency to openssls libcrypto. Added parsing for the rpmc parameter sfdp table. Added necessary rpmc parameter information to flashchips struct and the flash hardening feature to enable rpmc commands. Enables future use of these commands in the cli_client and also libflashrom. Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Signed-off-by: Matti Finder <matti.finder(a)gmail.com> --- M MAINTAINERS M include/flash.h A include/rpmc.h M meson.build M meson_options.txt A rpmc.c M sfdp.c 7 files changed, 822 insertions(+), 17 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/34/84934/5 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Gerrit-Change-Number: 84934 Gerrit-PatchSet: 5 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>

1 0

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

04 Nov '24

Attention is currently required from: Matti Finder, Peter Marheine. Hello Anastasia Klimchuk, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84840?usp=email to look at the new patch set (#9). The following approvals got outdated and were removed: Code-Review+2 by Peter Marheine, Verified+1 by build bot (Jenkins) Change subject: cli_client: Add rpmc command support ...................................................................... cli_client: Add rpmc command support This commit adds uses the new rpmc command implementation to add them as a new feature to the cli_client. Also adds the necessary documentation for this new feature. Tested on the Winbond W25R128JV as a 'SFDP-capable chip'. This patch was done to add rpmc command support to flashrom. This enables users to write root keys to their flash chips while they flash data on the chip. This might become useful in the future as rpmc support is extended in coreboot. Also adds debug tools to flashrom, which might be useful in implementing coreboots rpmc support. Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Signed-off-by: Matti Finder <matti.finder(a)gmail.com> --- M cli_classic.c M doc/classic_cli_manpage.rst M doc/release_notes/devel.rst 3 files changed, 266 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/84840/9 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 9 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com>

1 0

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

04 Nov '24

Attention is currently required from: Peter Marheine. Anastasia Klimchuk 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 2: (10 comments) Patchset: PS2: Thank you so much! Your suggestions are super useful, I appreciate a lot. File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84954/comment/ec9dca1d_564164b2?us… : PS1, Line 70: Programmers > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/d2f73af4_24091a83?us… : PS1, Line 73: * ch347_spi: Add spi clock frequency selection > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/4d6d83f4_7a475dc2?us… : PS1, Line 78: Bugs fixes > ```suggestion […] All suggestions are great, and all done. https://review.coreboot.org/c/flashrom/+/84954/comment/c7b511b6_4488bc60?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 […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/5f89c532_fc0bb0a2?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 […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/1eccc21b_c4d1f8f6?us… : PS1, Line 99: * Fix FEATURE_NO_ERASE chips and add test for them : : https://ticket.coreboot.org/issues/553 > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/c15db004_03eac81e?us… : PS1, Line 103: * build: never install cmocka : : https://ticket.coreboot.org/issues/561 > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/0b08e978_87eda435?us… : PS1, Line 107: * ichspi: Probe opcode in POSSIBLE_OPCODES[] as well : : https://ticket.coreboot.org/issues/556 > ```suggestion […] Done https://review.coreboot.org/c/flashrom/+/84954/comment/ce47fd27_80be04af?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. […] Done -- 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: 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: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Nov 2024 12:00:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

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

04 Nov '24

Attention is currently required from: Anastasia Klimchuk. Hello Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84954?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: doc: Add few sections to recent development doc ...................................................................... doc: Add few sections to recent development doc Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/release_notes/devel.rst 1 file changed, 78 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/54/84954/2 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d Gerrit-Change-Number: 84954 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>

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 ) 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit