Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logger: Enable LDC and FW compatibility check by default #3326

Merged
merged 2 commits into from
Sep 2, 2020

Conversation

ktrzcinx
Copy link
Member

By default user should use compatible files but forcing no check also should be possible.

@plbossart
Copy link
Member

What would be the results if the two firmware binary and ldc are not aligned?
the trace 'mostly' works but cannot be trusted?
Is there really an actual need for disabling this check?

@ktrzcinx
Copy link
Member Author

What would be the results if the two firmware binary and ldc are not aligned?
the trace 'mostly' works but cannot be trusted?

It depends how much both of them are misaligned, it may be just point wrong line in a trace source location (when a few lines in source code will be added / deleted) or when some trace will be added/deleted, then it may be complete mess-up.

Is there really an actual need for disabling this check?

In general, this option is usable when there is no access to version file and for developers for fast FW validation after some quick-fix,

From #3195 (review):

My dictionary proliferation and usability concern is not about full time SOF developers like you. It's about infrequent users. For instance a kernel developer who doesn't know much about SOF, reports a bug and receives a slightly different firmware file from someone else for a quick test. ~@marc-hb

@plbossart
Copy link
Member

What would be the results if the two firmware binary and ldc are not aligned?
the trace 'mostly' works but cannot be trusted?

It depends how much both of them are misaligned, it may be just point wrong line in a trace source location (when a few lines in source code will be added / deleted) or when some trace will be added/deleted, then it may be complete mess-up.

Is there really an actual need for disabling this check?

In general, this option is usable when there is no access to version file and for developers for fast FW validation after some quick-fix,

From #3195 (review):

My dictionary proliferation and usability concern is not about full time SOF developers like you. It's about infrequent users. For instance a kernel developer who doesn't know much about SOF, reports a bug and receives a slightly different firmware file from someone else for a quick test. ~@marc-hb

ok, so not for someone like me...

@lgirdwood
Copy link
Member

@ktrzcinx btw what is the lookup mechanism used in LDC files ? are we using the binary address as a lookup into the LDC string table ? It may be more consistent if we were to hash the dir, file, line and use this lookup as this would survive binary changes.

@lgirdwood
Copy link
Member

Even better would be to hash dir + file and trace string.

@ktrzcinx
Copy link
Member Author

ktrzcinx commented Aug 21, 2020

@ktrzcinx btw what is the lookup mechanism used in LDC files ?

each tr_xxx() is forwarded to _log_message() macro. There static part of trace (lvl, format, line_idx, file name, component_class deprecated, number of arguments) is moved to .static_log. lvl section (.static_log_entries segment) and address of this static entry (plus parameters, component and pipe id) is used in runtime, passed through DMA and parsed by logger.

.static_log_entries is saved in LDC file as it is + some header. So changing any trace line_idx will change LDC file content, but traces layout should looks the same, addidng/delete trace will shuffle LDC content.

hash used to verification LDC and FW compatibility is calculated from source code, so any FW change will be reported.

@lgirdwood
Copy link
Member

@ktrzcinx thanks for the info ! I guess this is something we could look at over the long term so that tooling can be more robust for non technical users.

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

@ktrzcinx think we are good to merge once rebased.

By default user should use compatible files.

Signed-off-by: Karol Trzcinski <[email protected]>
It may be needed for situation when user have firmware after
small fixes without updated ldc file or fw_ready file is not
accessible. Option dedicated for advanced users.

Signed-off-by: Karol Trzcinski <[email protected]>
@ktrzcinx
Copy link
Member Author

ktrzcinx commented Sep 2, 2020

@lgirdwood already rebased and tested by CI

@lgirdwood lgirdwood merged commit 7650398 into thesofproject:master Sep 2, 2020
@marc-hb marc-hb added the logger Dictionary and logger label Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logger Dictionary and logger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants