cc_version: use fixed length for CC_DESC #2522

xiulipan · 2020-03-11T05:30:06Z

Fix ABI version to 3.15.0 on master first.

There is a bug in the Cadence XCC compiler that give us different
linkage section sizes for flex length struct sof_ipc_ext_data in
ELF file compared to GCC version. And there are appending struct
in the same ELF section, this will bring wrong offset for those
struct binary in ELF file.

Example:
When the CC_DESC is " RG-2017.8-linux", we should have struct
length for 0x50 in header size, but the binary length in ELF file
is 0x4c. When the CC_DESC is " RG-2017.8-win", size are both 0x4c

All existing compiler description has length less than
24 bytes. So use a fixed length 24 in this structure.

ABI version 3.16.0

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

fix #2507

ktrzcinx

Please add in comment comment that sof_ipc_cc_version.desc is read as flex array from kernel side. This solution also resolve compilation problem from #2427.

xiulipan · 2020-03-11T15:09:00Z

@kakulesza it seems from the log, there is not change needed for kernel https://sof-ci.01.org/sofpr/PR2522/build5231/devicetest/APL_UP2_PCM512X/verify-sof-firmware-load.sh/verify-sof-firmware-load.sh.txt
Change flex into static will not have any side effect if the string size is big enough.
kernel: [ 4.157801] sof-audio-pci 0000:00:0e.0: Firmware info: used compiler XCC 12:0:8 RG-2017.8-linux used optimization flags O2

@lgirdwood Please help to review this fix for the logger regression.

src/include/ipc/info.h

jajanusz · 2020-03-12T12:11:43Z

@lgirdwood This is hard one, cos it's in abi header, but it's not really abi change, cos these fields are null-terminated strings anyway, so it doesn't matter for parsing if it's flex array or const array. It's fixed-size array just to overcome some embedded compiler limitations that are not up-to-date with gcc.

lgirdwood

Pends on kernel update.

src/include/ipc/info.h

xiulipan · 2020-03-13T03:16:48Z

@lgirdwood @ktrzcinx @jajanusz Thanks for remind about the Kernel part. There is indeed a need to make align on both side.
PR thesofproject/linux#1890 send.

src/ipc/cc_version.c

lgirdwood · 2020-03-13T16:19:36Z

@xiulipan thanks, ping me once kernel one is approved. Can you assign ABI number too in classifier.

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

@lgirdwood It seems I did not have access for the project

lgirdwood · 2020-03-16T10:39:49Z

@xiulipan updated, I've given all member W access bow to align their ABI updates.

xiulipan · 2020-03-17T05:26:19Z

@lgirdwood Should this be go with an ABI patch or an ABI minor?

lgirdwood · 2020-03-17T11:41:42Z

@xiulipan Lets make it minor since it's impact is user & developer confusion if version is wrong

xiulipan · 2020-03-18T03:32:38Z

@lgirdwood update the PR with ABI to 3.15.0

lgirdwood · 2020-03-18T10:24:07Z

@xiulipan ok, please ping when kernel is approved and we can merge.

kv2019i · 2020-03-20T09:03:21Z

@xiulipan @lgirdwood Kernel change is ready to go, but a concern was raised by @plbossart why we pick 24 chars as the length here. Is that future-proof enough? @paulstelian97 also had concerns on this. Let's agree on this, and then we can merge.

jajanusz · 2020-03-20T09:25:11Z

@xiulipan you need to rebase

xiulipan · 2020-03-24T09:03:59Z

@lgirdwood @plbossart I also find another mismatch in the info.h header. #2338
Do we need a ABI for the probe also? Or we just take it as ABI 14 also?

lgirdwood · 2020-03-24T15:02:37Z

@xiulipan I've assigned to ABI 14
For #2338 do you mean kernel ABI +1 FW ABI ? I cannot see the the kernel PR in the list (other kernel PRs missing too ?)
@ranj063 @plbossart @kv2019i it looks like we are missing the matching kernel PRs in the ABI tool. These have to be manually added atm in the "Projects" button on the PR (then you can align on a suitable ABI version)

xiulipan · 2020-03-25T02:28:59Z

@lgirdwood So here is my thinking, we will need fix for FW to make
#2338 to add ABI 14
#2398 to change ABI 14 => 15
@ranj063 @plbossart @kv2019i
For above FW side ABI change I got a simple fix in thesofproject/linux#1929 but not sure if I got the right ABI version or header change.
I will also send an email to discuss about this issue.

lgirdwood · 2020-03-25T11:46:38Z

@xiulipan anything in your plan preventing this PR merging into 14 ?

xiulipan · 2020-03-26T03:32:59Z

@lgirdwood I have updated thesofproject/linux#1890 to make each ABI change has its own ABI.
Probe support should have ABI 3.14.0
Debug support should fixed to be ABI 3.15.0
This cc_version length should be ABI 3.16.0

lgirdwood · 2020-03-26T10:56:47Z

@lgirdwood I have updated thesofproject/linux#1890 to make each ABI change has its own ABI.
Probe support should have ABI 3.14.0
Debug support should fixed to be ABI 3.15.0
This cc_version length should be ABI 3.16.0

ok, I can see the classifier has been updated. Does this now fully reflect your plan.
https://github.com/orgs/thesofproject/projects/2
@plbossart are you good with @xiulipan ABI update plan ?

jajanusz · 2020-03-27T08:24:29Z

@xiulipan @plbossart @lgirdwood Please prioritize merging this PR. Logs are already broken for like 2 weeks.

kv2019i · 2020-03-27T09:42:55Z

ok, I can see the classifier has been updated. Does this now fully reflect your plan.
https://github.com/orgs/thesofproject/projects/2
@plbossart are you good with @xiulipan ABI update plan ?

@lgirdwood I think we we are good to go now. We hit issues by merging FW patches for ABI bumps that were not approved on kernel side, but we gain nothing by delaying this. I.e. let's put this in, and then @xiulipan linux#1890 and then we are aligned again.

lgirdwood

@xiulipan it looks like #2398 should be in MINOR 14, @kv2019i does this being 14 align with kernel too ? If so, I can update classifier and we can put this PR in 15 ?

lgirdwood · 2020-03-27T11:45:29Z

src/include/kernel/abi.h

@xiulipan This does not align with the classifier or the commit message. Can you double check this.

@lgirdwood @xiulipan D'oh, you are right. The kernel patch (not yet merged) puts cc_version as ABI16:
thesofproject/linux@3552e9c
So definitely commit message and patch are not in sync here.

This probably happened because FW PRs for ABI15 have been merged without bumping the version. E.g.
e6940da
... was the last PR for ABI14, but it was merged with abi.h of ABI14.

I think we can probably still go ahead with this. The abi.h version needs to be changed to 16 when the last PR for ABI16 is merged. As @lyakh commented on the kernel side, if this is the norm to have multiple changes done in a single bump, we need to separate the commits that add functionality from commits that bump the version. I.e. we open ABIxx for development, have interface changes approved for this ABI level, and then we have a closing commit that freezes ABIxx. No binary FW releases should be done from master during this time.

@kv2019i I see why I'm not finding the kernel PR's - @xiulipan can you split your kernel PR into 3, one PR per ABI bump. That way we can align the with the FW on the classifier and follow. Once this is done and ABIs bumps aligned we can merge all at one go.

@xiulipan ping ?

@lgirdwood The three kernel pr is already there
thesofproject/linux#1947
thesofproject/linux#1946
thesofproject/linux#1890

Replace char with uint8_t to have fixed lenght for string. char has minimum 8 bits length. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

There is a bug in the Cadence XCC compiler that give us different linkage section sizes for flex length struct sof_ipc_ext_data in ELF file compared to GCC version. And there are appending struct in the same ELF section, this will bring wrong offset for those struct binary in ELF file. Example: When the CC_DESC is " RG-2017.8-linux", we should have struct length for 0x50 in header size, but the binary length in ELF file is 0x4c. When the CC_DESC is " RG-2017.8-win", size are both 0x4c All existing compiler description has length less than 32 bytes. So use a fixed length 32 in this structure. ABI version changes to 3.15.0 Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

kv2019i · 2020-03-31T14:11:24Z

@lgirdwood Kernel side now ready to go.

lgirdwood · 2020-04-01T10:47:32Z

CI know issues.

lgirdwood · 2020-04-01T10:49:32Z

@kv2019i Merged, I think we are good now

kv2019i · 2020-04-01T11:17:04Z

@lgirdwood wrote:

@kv2019i Merged, I think we are good now

Ack, all changes for ABI versions 14&15 are now good in both kernel and fw.

xiulipan requested review from a team and bardliao as code owners March 11, 2020 05:30

xiulipan mentioned this pull request Mar 11, 2020

[BUG]Sof-logger ABI version is mismatch with .ldc file built by xcc #2507

Closed

ktrzcinx mentioned this pull request Mar 11, 2020

Create extended manifest #2427

Merged

ktrzcinx approved these changes Mar 11, 2020

View reviewed changes

xiulipan requested a review from lgirdwood March 11, 2020 15:08

xiulipan mentioned this pull request Mar 11, 2020

comp: update api documentation for component state #2514

Merged

xiulipan requested a review from mmaka1 March 12, 2020 05:33

ktrzcinx reviewed Mar 12, 2020

View reviewed changes

src/include/ipc/info.h Outdated Show resolved Hide resolved

lgirdwood approved these changes Mar 12, 2020

View reviewed changes

src/include/ipc/info.h Outdated Show resolved Hide resolved

lgirdwood added the ABI ABI change involved label Mar 12, 2020

xiulipan mentioned this pull request Mar 13, 2020

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

Merged

lyakh reviewed Mar 13, 2020

View reviewed changes

src/ipc/cc_version.c Outdated Show resolved Hide resolved

xiulipan force-pushed the pr/fixccinfo branch from 102f9ac to fa000f5 Compare March 17, 2020 08:18

lgirdwood approved these changes Mar 17, 2020

View reviewed changes

xiulipan force-pushed the pr/fixccinfo branch from fa000f5 to db22257 Compare March 18, 2020 03:29

xiulipan changed the title ~~cc_version: use static array for CC_DESC~~ cc_version: use fixed length for CC_DESC Mar 18, 2020

xiulipan force-pushed the pr/fixccinfo branch from db22257 to 5e58cda Compare March 20, 2020 09:27

xiulipan force-pushed the pr/fixccinfo branch from 8786084 to bc4f3f3 Compare March 24, 2020 08:38

xiulipan force-pushed the pr/fixccinfo branch from bc4f3f3 to d11f18b Compare March 26, 2020 03:27

jajanusz added the P1 Blocker bugs or important features label Mar 27, 2020

kv2019i approved these changes Mar 27, 2020

View reviewed changes

jajanusz mentioned this pull request Mar 27, 2020

logger: Make CC_DESC string length indivisible by four #2647

Merged

lgirdwood requested changes Mar 27, 2020

View reviewed changes

This was referenced Mar 31, 2020

ASoC: SOF: add probe support extend data thesofproject/linux#1946

Merged

ASoC: SOF: add debug ABI version thesofproject/linux#1947

Merged

ipc: cc_version: change type char to uint8_t

a546444

Replace char with uint8_t to have fixed lenght for string. char has minimum 8 bits length. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>

xiulipan force-pushed the pr/fixccinfo branch from d11f18b to 988fcf6 Compare March 31, 2020 11:15

kv2019i requested review from lgirdwood, lyakh and paulstelian97 March 31, 2020 11:36

paulstelian97 approved these changes Mar 31, 2020

View reviewed changes

lgirdwood approved these changes Mar 31, 2020

View reviewed changes

lgirdwood merged commit beea8e5 into thesofproject:master Apr 1, 2020

ktrzcinx mentioned this pull request Oct 9, 2020

Support extended manifest config data for clock config thesofproject/linux#2297

Closed

cc_version: use fixed length for CC_DESC #2522

cc_version: use fixed length for CC_DESC #2522

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ktrzcinx left a comment

Choose a reason for hiding this comment

Uh oh!

xiulipan commented Mar 11, 2020

Uh oh!

Uh oh!

jajanusz commented Mar 12, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiulipan commented Mar 13, 2020

Uh oh!

Uh oh!

lgirdwood commented Mar 13, 2020

Uh oh!

xiulipan commented Mar 16, 2020

Uh oh!

lgirdwood commented Mar 16, 2020

Uh oh!

xiulipan commented Mar 17, 2020

Uh oh!

lgirdwood commented Mar 17, 2020

Uh oh!

xiulipan commented Mar 18, 2020

Uh oh!

lgirdwood commented Mar 18, 2020

Uh oh!

kv2019i commented Mar 20, 2020

Uh oh!

jajanusz commented Mar 20, 2020

Uh oh!

xiulipan commented Mar 24, 2020

Uh oh!

lgirdwood commented Mar 24, 2020

Uh oh!

xiulipan commented Mar 25, 2020

Uh oh!

lgirdwood commented Mar 25, 2020

Uh oh!

xiulipan commented Mar 26, 2020

Uh oh!

lgirdwood commented Mar 26, 2020

Uh oh!

jajanusz commented Mar 27, 2020

Uh oh!

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

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

xiulipan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Mar 31, 2020

Uh oh!

lgirdwood commented Apr 1, 2020

xiulipan commented Mar 11, 2020 •

edited

Loading

kv2019i commented Mar 27, 2020 •

edited

Loading

kv2019i Mar 27, 2020 •

edited

Loading

lgirdwood Mar 27, 2020 •

edited

Loading