Attention is currently required from: Anil Kumar K, Jérémy Compostella, Krishna P Bhat D, Paul Menzel, Rizwan Qureshi, sridhar siricilla.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577?usp=email )
Change subject: soc/intel/cse: Back up PSR data during CSE FW downgrade ......................................................................
Patch Set 52:
(4 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/684379d8_b4ccac4f : PS50, Line 1081: cse_boot_to_rw
as explained above we do not want to prevent the system to boot whenever there is a failure to PSR backup attempt.
I'm missing the point. why PSR data backup failure is considered non-critical information. if it's non-critical then why do we want to backup that up even for downgrading ? can't we just ignore the PSR data on every downgrade like what you have done here and continue booting ?
https://review.coreboot.org/c/coreboot/+/74577/comment/c7b99d20_e49e40ed : PS50, Line 1092: goto log_and_exit;
As Rizwan mentioned , we backup data whenever possible.
what does that mean? "backup the data whenever possible?" are you saying that the PSR backup is not something mandatory ?
When there is a failure to backup (which is a rare event) we dont want to go to recovery. We log that as an event and continue to boot.
I'm looking for some documentation that says there is no harm in ignoring the backup failure and don't need to go into recovery.
(which is a rare event)
what is the reason behind such *rare* events? and how to handle the rear handle ? just ignore it ?
https://review.coreboot.org/c/coreboot/+/74577/comment/15bb2a4a_0998ac26 : PS50, Line 1125: An attempt to send PSR back-up command has been made.
what will happen when you just `return' after marking PSR_BACKUP_DONE (even in case the PSR backup is failing)? This information is missing in the above flow diagram? system will boot into OS in recovery mode and the device eventually lost its old telemetry data ? if there is no impact of losing those PSR data (in downgrade scenario) then why are we even care so much about implementing PSR ?
I thought you originally wished to ensure that before CSE clearing the data, we need to preserve the PSR and then CSE reapplying that backup data later. Now if PSR backup is failing and you don't consider that as any catastrophic failure, I'm missing the purpose of doing all the flow change. We can simply assume, PSR is not backup in all CSE downgrade scenario ?
That is a false logic, you are basing the above conclusion of not backing up the data for something that happens as an failure/anomaly and a rare event. In all normal scenarios if there is an opportunity to back-up the data, we should.
What do you mean by false logic? `cse_boot_to_rw` is not a false logic. if the device is unable to boot to RW and you aren't able to send PSR_HECI_FW_DOWNGRADE_BACKUP cmd, hence, you finally marked things as PSR backup done ? i don't understand the motivation here.
if you started ignoring all the failure cases and corner cases then I'm afraid we shall miss the original intent. What I'm asking is don't call the PSR backup done when it's not actually attempted or succeeded. Rather use something as PSR_BACKUP_ABORTED. At least it will help us to know whether the attempt is successful genuinely or not.
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/ea8d2ead_c2249abe : PS52, Line 1152: CONFIG(SOC_INTEL_CSE_LITE_PSR) use this as the first case
``` if (CONFIG(SOC_INTEL_CSE_LITE_PSR) && status == CSE_UPDATE_DOWNGRADE) ```
-
Subrata Banik (Code Review)