Skip to content

Conversation

@marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Dec 18, 2021

3 commits

Sample output:

logger ABI Version is   5:3:0
ldc_file ABI Version is 5:3:0
ldc_file src checksum           0x07d4f1ad       <=== NEW!
Loaded FW expects chksum        0x07d4f1ad       <=== NEW!

Components uuid dictionary size:        2400 bytes
Components uuid base address:           0x1fffa000
...

@marc-hb marc-hb marked this pull request as ready for review December 18, 2021 07:40
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 19, 2021

Everything is green except this checkpatch warning which was already there before this PR:

WARNING: quoted string split across lines
#74: FILE: tools/logger/logger.c:47:
+	fprintf(stdout, "%s:\t -v ver_file\t\tUse ver_file instead of "
+		"/sys/kernel/debug/sof/fw_version\n", APP_NAME);

I suspect the purpose of this warning is to make sure error messages can be git greped. Not really an issue here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What motivation stands behind this change ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ktrzcinx for reviewing this PR, really appreciated.

This is what allows the -d ldc_file option to ALSO show the checksum expected by the firmware - when and only when a firmware is running. See sample output in the description. I suggest looking at each commit independently if you have looked only at the entire diff for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've got it!

@ujfalusi
Copy link
Contributor

Does the duplicated checksum gives anything for the user?

ldc_file src checksum           0x07d4f1ad       <=== NEW!
Loaded FW expects chksum        0x07d4f1ad       <=== NEW!

Would not be better to print a single line of the two checksum matches:

ldc checksum: 0x07d4f1ad

If there is a mismatch between the two checksum then:

lcd_file src checksum is 0x07d4f1ad while the loaded firmware expects 0x1b67cba9

@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 20, 2021

Would not be better to print a single line of the two checksum matches:

I don't think so because a single line is also printed when the firmware is not loaded.

If there is a mismatch between the two checksum then:
lcd_file src checksum is 0x07d4f1ad while the loaded firmware expects 0x1b67cba9

I personally prefer having the checksums close to each other:

ldc_file src checksum          0x07d4f1ad
Loaded FW expects chksum       0x1b67cba9

... and this makes the code simpler because it does not have to compare them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C style

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I thought #5046 meant this is allowed now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine in the SPDX, but my pattern matching needs C style in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Fixes commit 901f991 ("logger: Validate by src_hash instead of abi
version from fw_ready") that changed the feature without updating the
name.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Since commit 901f991 ("logger: Validate by src_hash instead of abi
version from fw_ready") the dictionary checksum has become the most
important information.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Sample output:

logger ABI Version is   5:3:0
ldc_file ABI Version is 5:3:0
ldc_file src checksum           0x07d4f1ad
Loaded FW expects chksum        0x07d4f1ad       <=== NEW!

Components uuid dictionary size:        2400 bytes
Components uuid base address:           0x1fffa000
...

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Dec 21, 2021

Unusual, isolated and totally unrelated timeout in https://sof-ci.01.org/sofpr/PR5110/build11443/devicetest/?model=ADLP_RVP_NOCODEC&testcase=check-pause-resume-capture-10, everything else is green.

@lgirdwood lgirdwood merged commit 352ed2c into thesofproject:main Dec 22, 2021
@marc-hb marc-hb deleted the logger-ldc-sum branch January 7, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants