Skip to content

Conversation

@ujfalusi
Copy link
Contributor

The is_zephyr check looks at the number of 'zephyr' strings in the LDC file while the is_firmware_file_zephyr is doing the same on the actual firmware file.
It has been observed that the is_zephyr function can get the detection wrong and reports false for a zephyr firmware.

Note also that virtually only the check-sof-logger is using the is_zephyr call, all other test cases use the is_firmware_file_zephyr call.

The is_zephyr check looks at the number of 'zephyr' strings in the LDC file
while the is_firmware_file_zephyr is doing the same on the actual
firmware file.
It has been observed that the is_zephyr function can get the detection
wrong and reports false for a zephyr firmware.

Note also that virtually only the check-sof-logger is using the is_zephyr
call, all other test cases use the is_firmware_file_zephyr call.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

My only minor concern is: now this test requires the firmware to have been loaded at least once on this linux boot so the firmware file name can be extracted from the kernel logs. There's a good chance this is already required for some other reason, so be it.

Approved assuming the tests are passing. Note PR testing does not test MTL yet.

While in the check-sof-logger.sh context please review one line long but important fix #1086 in the same test.

@ujfalusi
Copy link
Contributor Author

@marc-hb, it is kind of hard to test if the firmware logging is working without a loaded firmware, no?

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 18, 2023

@marc-hb, it is kind of hard to test if the firmware logging is working without a loaded firmware, no?

This test unloads and reloads the firmware, using the firmware boot banner as a sanity check. So in theory this test can work while being the first to load the firmware. But now this won't work anymore because is_firmware_file_zephyr is called first.

Minor new requirement, no big deal.

@ujfalusi
Copy link
Contributor Author

@marc-hb, it is kind of hard to test if the firmware logging is working without a loaded firmware, no?

This test unloads and reloads the firmware, using the firmware boot banner as a sanity check. So in theory this test can work while being the first to load the firmware. But now this won't work anymore because is_firmware_file_zephyr is called first.

Hrm, I'm not sure about that as the test will first reloads the drivers, thus boots the firmware then it will starts the logger to fetch the logs:
https://github.com/thesofproject/sof-test/blob/main/test-case/check-sof-logger.sh#L257-L259

Or am I looking at it in a wrong way?

@ujfalusi
Copy link
Contributor Author

All check-sof-logger cases are passing

@marc-hb
Copy link
Collaborator

marc-hb commented Jul 19, 2023

as the test will first reloads the drivers, thus boots the firmware then it will starts the logger to fetch the logs:

Then we're good.

I guess I had logger_disabled()->is_firmware_file_zephyr() in mind. This one is called very early but it's fortunately not used by check-sof-logger.sh right now (I think someone asked for that a long time ago)

These dependencies are very brittle, seems more complicated than booting an OS :-)

@marc-hb marc-hb merged commit 52bc60a into thesofproject:main Jul 19, 2023
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.

3 participants