Attention is currently required from: Matti Finder.
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
4 comments:
File cli_classic.c:
Patch Set #8, 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.
Patch Set #8, 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:
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:
Patch Set #4, 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 change 84840. To unsubscribe, or for help writing mail filters, visit settings.