ASoC: SOF: make sof_ipc_cc_version to fixed length and fix ABI mismatch #1890

xiulipan · 2020-03-13T03:15:47Z

Add probe support from thesofproject/sof#2338 ABI version 3.14.0
Add debug ABI version from thesofproject/sof#2398 ABI version 3.15.0

Align struct sof_ipc_cc_version to firmware definition in SOF ABI 3.16.0.
The struct definition was changed due to errors in FW build.
The Cadence XCC compiler produces incorrect linkage section sizes, when a
variable length array is used in the compiler version struct. The firmware
definition was changed to a fixed 32 byte compiler description string.
This length covers all released firmware binaries and thus only a minor
ABI change is needed.

As the same structure is used in IPC messages between driver and firmware,
the kernel needs to be aligned to firmware change.

Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com

FW change is thesofproject/sof#2522

aiChaoSONG · 2020-03-13T03:53:49Z

@xiulipan Catch a typo in commit message: troble -> trouble. I am a little confused by "static" here, I think it is actually "fixed" length

xiulipan · 2020-03-13T04:32:23Z

@aiChaoSONG thanks, updated

lyakh

...provided 24 characters are enough - this should be good. But two notes:

I still don't understand why our ABI headers aren't under include/uapi
strictly speaking char isn't a proper type for fixed-size objects, uint8_t or __u8 should be used instead. But that can be a separate patch.

include/sound/sof/info.h

plbossart

this needs more explanations, sorry

include/sound/sof/info.h

xiulipan · 2020-03-16T08:19:54Z

@plbossart @kv2019i @lyakh comment updated, please check if this would explain the issue.
Mode detail with debug logs please see thesofproject/sof#2507

kv2019i

Commit message still needs work.

include/sound/sof/info.h

xiulipan · 2020-03-17T07:59:39Z

@kv2019i @plbossart Thanks for point out the problem. I think the comment may be more fit for FW side. I will try to refine the comment here to explain the change is align with FW.

lgirdwood · 2020-03-17T11:45:43Z

@plbossart @kv2019i there are bug in the Cadence compiler that give us different linkage section sizes and some offsets in packed data compared to GCC. Specifying a size here aligns XCC with GCC output.

plbossart · 2020-03-17T12:49:40Z

@plbossart @kv2019i there are bug in the Cadence compiler that give us different linkage section sizes and some offsets in packed data compared to GCC. Specifying a size here aligns XCC with GCC output.

Right, so let's capture that in the commit message.

xiulipan · 2020-03-18T04:50:23Z

@plbossart @kv2019i @lgirdwood update the comments with ABI version 3.15.0

kv2019i

Now commit message has all the info. As this is a fairly complex rootcause, let's iterate on the language a bit more, but close to merging.

include/sound/sof/info.h

xiulipan · 2020-03-26T03:30:44Z

@plbossart @kv2019i @lyakh @lgirdwood update commits and merge with ABI fix patches to make ABI change in order.

kv2019i

Many good updates. I still have a few minor nits on the commit messages and we need to resolve the debug API ABI version alignment, please see inline.

include/sound/sof/info.h

include/sound/sof/trace.h

include/sound/sof/info.h

kv2019i · 2020-03-26T08:12:11Z

include/uapi/sound/sof/abi.h

@xiulipan @lgirdwood We have a conflict still here:

this kernel patch is set debug ABI to 15

FW patch was merged with ABI 14 thesofproject/sof@e6940da

ABI classifier states this is ABI 15

I would propose to change classifier and kernel patch to ABI14 to match the already merged FW. Please ack.

xiulipan · 2020-03-26T09:31:34Z

@kv2019i PR updated, thanks for comments advice.

kv2019i

@xiulipan Thanks, commit messages now look good! Please check my proposal for ABI versioning. If @lgirdwood approves, let's do this and get this PR to ready and bump ABI to 16 in kernel as well.

kv2019i · 2020-03-26T09:47:49Z

include/uapi/sound/sof/abi.h

@xiulipan @lgirdwood This PR anyways includes all changes from ABI13-14-15-16, so this is mostly to get the history records and git commit messages accurate. I'd change this commit in the series to assign ABI14 as that's how the FW was merged. And then align rest of the series as per ABI classifier (and unmerged patches). We'll end up with bumping ABI to 16 in this series in any case.

xiulipan · 2020-03-26T10:04:14Z

@kv2019i Thanks for the proposal.
This PR and thesofproject/sof#2522 will achieve the target to make FW and Kernel ABI align to 16.

kv2019i

@xiulipan Can you fix the "ASoC: SOF: add debug ABI version" to set version to 3.14.0 to match the merged FW patch thesofproject/sof#2398

I think that's the only change we still need to merge this. @lyakh @lgirdwood @plbossart please comment if you disagree on this

xiulipan · 2020-03-27T09:39:53Z

@kv2019i The point here is probe support do not have an ABI version, do we need add one for it or ignore the ABI version for it?

kv2019i · 2020-03-27T10:01:36Z

@xiulipan wrote:

@kv2019i The point here is probe support do not have an ABI version, do we need add one for it or ignore the ABI version for it?

? the FW patch clearly modifies ABI version to 3.14.0 -> thesofproject/sof@e6940da

And in your kernel commit message for debug interface, you state "This change main ABI to 3.15.0 and you modify the header to ABI 15.

So I don't want to have merged FW and kernel patches to have conflicting information. The FW patch was now merged as 14, so kernel should not claim 15 either. Or just don't touch the ABI at all in this patch and only bump in the follow-up patches.

lgirdwood

@xiulipan this needs split into 3 PRs, one for each ABI bump then mapped into ABI tool. We should then be good to merge all PRs.

xiulipan · 2020-03-28T04:36:07Z

@lgirdwood Got that, but It may got some conflict on the ABI number change. As both PR will base on ABI 13. The merge conflict can not be avoided. Other wise, we need to rebase each PR after the merge.
I will send 3 new PR and keep this PR as well. You can decided which way is better.

xiulipan · 2020-03-28T05:28:40Z

@lgirdwood ABI 3.14 #1946
ABI 3.15 #1947
ABI 3.16 #1890

Use uint8_t to replace char in packed ABI structs to have fixed length for struct. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

Align struct sof_ipc_cc_version to firmware definition in SOF ABI 3.15.0. The struct definition was changed due to errors in FW build. The Cadence XCC compiler produces incorrect linkage section sizes, when a variable length array is used in the compiler version struct. The firmware definition was changed to a fixed 32 byte compiler description string. This length covers all released firmware binaries and thus only a minor ABI change is needed. As the same structure is used in IPC messages between driver and firmware, the kernel needs to be aligned to firmware change. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

xiulipan · 2020-03-31T11:22:06Z

@lgirdwood @kv2019i Update and rebase as requested. Ping for review and merge.

lgirdwood · 2020-03-31T12:33:09Z

@kv2019i @plbossart FW and kernel now aligned at 14, this and thesofproject/sof#2522 will then align at 15 and everything thereafter will be 16 onwards

kv2019i

Yes, good now! Thanks @xiulipan FYI @lgirdwood

kv2019i · 2020-03-31T14:10:48Z

NOTE: now that sof-logger starts to work again, we have some failures reported, but as sof-logger has been out-of-business for some days now, it's impossible to tell when it broke. Better to merge this (to get the logs) and sort out any errors as a next step.

kv2019i · 2020-03-31T14:12:58Z

@plbossart @lyakh You have pending review comments open. We are in a bit of a hurry with this one...

lgirdwood · 2020-03-31T14:19:59Z

Now just waiting on Internal CI completing on FW side. Will merge both when it's done.

xiulipan requested review from juimonen, keyonjie, lyakh and ranj063 as code owners March 13, 2020 03:15

xiulipan mentioned this pull request Mar 13, 2020

cc_version: use fixed length for CC_DESC thesofproject/sof#2522

Merged

xiulipan changed the title ~~ASoC: SOF: make sof_ipc_ext_data to static length~~ ASoC: SOF: make sof_ipc_ext_data to fixed length Mar 13, 2020

xiulipan force-pushed the pr/fixflexcc branch from 8309530 to 0625519 Compare March 13, 2020 04:31

lyakh previously approved these changes Mar 13, 2020

View reviewed changes

kv2019i requested changes Mar 13, 2020

View reviewed changes

include/sound/sof/info.h Outdated Show resolved Hide resolved

plbossart requested changes Mar 13, 2020

View reviewed changes

include/sound/sof/info.h Outdated Show resolved Hide resolved

xiulipan dismissed lyakh’s stale review via 4d51aa0 March 16, 2020 08:13

xiulipan force-pushed the pr/fixflexcc branch from 0625519 to 4d51aa0 Compare March 16, 2020 08:13

xiulipan requested review from kv2019i and plbossart March 16, 2020 08:16

kv2019i requested changes Mar 16, 2020

View reviewed changes

include/sound/sof/info.h Outdated Show resolved Hide resolved

include/sound/sof/info.h Outdated Show resolved Hide resolved

plbossart requested changes Mar 16, 2020

View reviewed changes

include/sound/sof/info.h Outdated Show resolved Hide resolved

xiulipan changed the title ~~ASoC: SOF: make sof_ipc_ext_data to fixed length~~ ASoC: SOF: make sof_ipc_cc_version to fixed length Mar 17, 2020

xiulipan force-pushed the pr/fixflexcc branch 2 times, most recently from d8ccf94 to 846fb7f Compare March 17, 2020 08:29

xiulipan added the ABI involves ABI changes, need extra attention label Mar 18, 2020

xiulipan force-pushed the pr/fixflexcc branch from 846fb7f to c404034 Compare March 18, 2020 04:49

xiulipan requested a review from dbaluta as a code owner March 18, 2020 04:49

kv2019i requested changes Mar 18, 2020

View reviewed changes

include/sound/sof/info.h Outdated Show resolved Hide resolved

xiulipan force-pushed the pr/fixflexcc branch from cc2a709 to b7ffac2 Compare March 26, 2020 03:21

xiulipan changed the title ~~ASoC: SOF: make sof_ipc_cc_version to fixed length~~ ASoC: SOF: make sof_ipc_cc_version to fixed length and fix ABI mismatch Mar 26, 2020

kv2019i requested changes Mar 26, 2020

View reviewed changes

xiulipan force-pushed the pr/fixflexcc branch from b7ffac2 to 3552e9c Compare March 26, 2020 09:25

kv2019i requested changes Mar 26, 2020

View reviewed changes

kv2019i requested changes Mar 27, 2020

View reviewed changes

lgirdwood requested changes Mar 27, 2020

View reviewed changes

xiulipan added 4 commits March 31, 2020 19:20

ASoC: SOF: change type char to uint8_t in info.h

f4e61df

Use uint8_t to replace char in packed ABI structs to have fixed length for struct. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

ASoC: SOF: change type char to uint8_t in trace.h

41fb480

Use uint8_t to replace char in packed ABI structs to have fixed length for struct. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

ASoC: SOF: change type char to uint8_t in topology.h

8fc1902

Use uint8_t to replace char in packed ABI structs to have fixed length for struct. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

xiulipan force-pushed the pr/fixflexcc branch from 3552e9c to 1515627 Compare March 31, 2020 11:21

lgirdwood approved these changes Mar 31, 2020

View reviewed changes

kv2019i approved these changes Mar 31, 2020

View reviewed changes

kv2019i requested a review from lyakh March 31, 2020 14:11

plbossart approved these changes Mar 31, 2020

View reviewed changes

lgirdwood merged commit 3bdd89d into thesofproject:topic/sof-dev Mar 31, 2020

xiulipan mentioned this pull request Oct 23, 2020

[RFC]ext_man: fix size issue for ext_man_config_data elem thesofproject/sof#3527

Closed

ASoC: SOF: make sof_ipc_cc_version to fixed length and fix ABI mismatch #1890

ASoC: SOF: make sof_ipc_cc_version to fixed length and fix ABI mismatch #1890

Uh oh!

Conversation

xiulipan commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiChaoSONG commented Mar 13, 2020

Uh oh!

xiulipan commented Mar 13, 2020

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiulipan commented Mar 16, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiulipan commented Mar 17, 2020

Uh oh!

lgirdwood commented Mar 17, 2020

Uh oh!

plbossart commented Mar 17, 2020

Uh oh!

xiulipan commented Mar 18, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiulipan commented Mar 26, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Mar 26, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Mar 26, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Mar 27, 2020

Uh oh!

kv2019i commented Mar 27, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Mar 28, 2020

Uh oh!

xiulipan commented Mar 28, 2020

Uh oh!

xiulipan commented Mar 31, 2020

Uh oh!

lgirdwood commented Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiulipan commented Mar 13, 2020 •

edited

Loading

lgirdwood commented Mar 31, 2020 •

edited

Loading