ASoC: SOF: ext_manifest: Parse debug ABI version #2155
Conversation
sound/soc/sof/loader.c
Outdated
There was a problem hiding this comment.
isn't this a bit verbose, we are going to have one log for each syspend/resume cycle, no?
dev_dbg?
There was a problem hiding this comment.
@plbossart ok, I am sorry for raising this now instead of yesterday immediately after your comment, but what strikes me in this new version, is that now in the "normal" stock configuration, where dev_dbg() is disabled, this manifest entry will have 0 effect. Is this really what we want? As for every suspend / resume, it's now guarded by a first_boot test, so, maybe we do want dev_info() for this? Not something I feel strongly about, maybe that's indeed what we want, but just seems a bit weird to me.
There was a problem hiding this comment.
Thanks! One trivial linefeed problem that should be addressed but otherwise good to go.
On a side note: this is an example PR that is mandatory as per current ABI change process, but which would not be needed according to the yet-to-be-ratified new process (thesofproject/sof-docs#244). There is no functional need to have this code in the kernel. Unlike e.g. EXT_MAN_ELEM_CC_VERSION, capturing this info to dmesg kernel logs does not seem important for bug triage. FYI @lgirdwood @plbossart @lbetlej
The debug ABI can be extracted from the extended manifest content. This information known at build time does not need to be provided in a mailbox. Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
juimonen
left a comment
juimonen
left a comment
There was a problem hiding this comment.
LGTM, so address the other comments and should be good to go.
|
@plbossart Can you take a new look? |
|
@kv2019i I can't see any blockers |
|
@plbossart @lyakh ping, can you give this a review? |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
I've lost track of when the ext-manifest would be used but this code seems fine.
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
I'm still a bit unclear about the merits of
if (sdev->first_boot)
dev_dbg(sdev->dev,...);
but if that's indeed the level of verbosity, that we want, I have no objections otherwise
|
@lyakh I think construction is ok, we get only one log message (after first boot) and only in with enabled verbose logs option, exactly as @plbossart suggest. |
|
One warning about 80+ line from old checkpatch, but this is within the new 100 column limit, so ok. Merging. Thanks @ktrzcinx and all the reviewers! |
|
@plbossart @ktrzcinx @xiulipan We need to follow-up with ext-manifest enabling in the FW build process. Now with ext-support enabled in mainline kernel, we should enable ext-manifest for all new platforms in the release binaries (a new platform is only supported by new kernels, so we have no issue with backwards compatibility). |
6 participants
The debug ABI can be extracted from the extended manifest content.
This information known at build time does not need to be provided
in a mailbox.
Signed-off-by: Karol Trzcinski karolx.trzcinski@linux.intel.com
Related with thesofproject/sof#3003