Skip to content

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

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.

Sign up for GitHub

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

Merged
lgirdwood merged 4 commits into from
Mar 31, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4e61df
ASoC: SOF: change type char to uint8_t in info.h
xiulipan Mar 24, 2020
41fb480
ASoC: SOF: change type char to uint8_t in trace.h
xiulipan Mar 26, 2020
8fc1902
ASoC: SOF: change type char to uint8_t in topology.h
xiulipan Mar 26, 2020
1515627
ASoC: SOF: make sof_ipc_cc_version to fixed length
xiulipan Mar 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/sound/sof/info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ struct sof_ipc_cc_version {
/* reserved for future use */
uint32_t reserved[4];

char name[16]; /* null terminated compiler name */
char optim[4]; /* null terminated compiler -O flag value */
char desc[]; /* null terminated compiler description */
uint8_t name[16]; /* null terminated compiler name */
uint8_t optim[4]; /* null terminated compiler -O flag value */
uint8_t desc[32]; /* null terminated compiler description */
} __packed;

/* extended data: Probe setup */
Expand Down
2 changes: 1 addition & 1 deletion include/sound/sof/topology.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ struct sof_ipc_comp_process {
/* reserved for future use */
uint32_t reserved[7];

unsigned char data[0];
uint8_t data[0];
} __packed;

/* frees components, buffers and pipelines
Expand Down
2 changes: 1 addition & 1 deletion include/sound/sof/trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ struct sof_ipc_dma_trace_posn {
struct sof_ipc_panic_info {
struct sof_ipc_hdr hdr;
uint32_t code; /* SOF_IPC_PANIC_ */
char filename[SOF_TRACE_FILENAME_SIZE];
uint8_t filename[SOF_TRACE_FILENAME_SIZE];
uint32_t linenum;
} __packed;

Expand Down
2 changes: 1 addition & 1 deletion include/uapi/sound/sof/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

/* SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 14
#define SOF_ABI_MINOR 15
Copy link
Member

@plbossart plbossart Mar 23, 2020

Choose a reason for hiding this comment

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

why do we go from 13 to 15 here?

Copy link
Author

@xiulipan xiulipan Mar 24, 2020

Choose a reason for hiding this comment

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

@plbossart For FW this change is for ABI 15, but kernel is still 13.
@lgirdwood Not sure who missed an ABI 14 patch for kernel

Copy link
Collaborator

@kv2019i kv2019i Mar 24, 2020

Choose a reason for hiding this comment

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

@xiulipan @plbossart You can get open Linux PRs that modify ABI by searching with ABI tag. 14 update is the DMIC one and it's here #1924

Copy link
Author

@xiulipan xiulipan Mar 24, 2020

Choose a reason for hiding this comment

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

@kv2019i I think we should align the ABI between FW and Kernel. The FW is already 14 from thesofproject/sof#2398

I send #1929 to clean up the mismatched ABI version and info.h

Copy link
Collaborator

@kv2019i kv2019i Mar 24, 2020

Choose a reason for hiding this comment

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

Ack @xiulipan

Copy link
Author

@xiulipan xiulipan Mar 26, 2020

Choose a reason for hiding this comment

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

@kv2019i The issue here is
Add probe support from thesofproject/sof#2338 ABI version 3.14.0 (this do not have an ABI at first time in FW, but I think it should be 14)
Add debug ABI version from thesofproject/sof#2398 ABI version 3.15.0 (this do have an ABI 14 for FW at first time, but I think we should make it to 15)

So here I think this fix should use 16.
@lgirdwood any idea? Do we need to ignore the ABI for probe support or fix the ABI like this? We are not that far to make things back to right. I also make my FW patch align with this change to bump the master ABI to 15 first thesofproject/sof#2522

Copy link
Collaborator

@kv2019i kv2019i Mar 26, 2020

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@lyakh lyakh Mar 26, 2020

Choose a reason for hiding this comment

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

am I the only one who thinks that we're doing something wrong with ABI numbers in general? I see several PRs currently in review changing the ABI version from X to Y, where X is often 13 and Y is one of 14, 15, or 16. And I don't think we practice manual patch merging by maintainers. Can we not try to separate feature merging and version incrementing? How about

int sof_ipc_tx_message(struct snd_sof_ipc *ipc, struct sof_ipc_message *msg)
{
	if (message->abi_version < ipc->abi_version)
		return -EPROTO;
	...
}
struct sof_ipc_message {
	int abi_version;
	u32 header;
	void *msg_data;
	size_t msg_bytes;
	void *reply_data;
	size_t reply_bytes;
}

Then we'll be able to accumulate patches that actually need a new functionality before incrementing the version, but then callers would need to check for that error and "fail gracefully."
Or we could require every such functionality to have a central switch "disable unless version >= X."
Or use #if ABI > X
And in fact we're talking about the minor number here, changing which shouldn't break functionality.

Copy link
Collaborator

@kv2019i kv2019i Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@lyakh This is probably not the best place to discuss other ways to manage ABIs, as this discussion is already pretty complex and we should complete this PR as per our currently agreed process. And currently, the ABI is bumped in FW when a change altering the ABI is merged. This is good if patches are cherry-picked to release branches, or snapshot firmware are used for CI -> the ABI version FW reports will be always accurate.

The kernel side is trickier. We have a set of headers describing the FW IPC interface. We should update this in same way as FW in that we update the kernel side headers and bump the ABI version in the same commit. In this git commit we should also document what the ABI changes are about. Kernel patches that take advantage of new FW ABIs can be sent later, that is ok, but we should keep the headers in sync.

And for this patch @xiulipan, FW support for debug version reporting, was merged with ABI 14, so any binaries built from FW master, will support this interface and report out ABI 14. So in this kernel patch, where we are the debug version fields to the ipc headers, we should keep the ABI level at 14 as well (that's where it was merged to FW).

UPDATE: our ABI process actually already supports grouping (e.g. multiple changes tagged for same minor bump). So you don't have to bump version in every patch. But we need to align the patches to the ABI dashboard decisions.

Copy link
Collaborator

@lyakh lyakh Mar 27, 2020

Choose a reason for hiding this comment

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

@kv2019i I understand that this is complex and that I'm late on the train, and that a lot of people spent many hours thinking this process through, so, they probably had all those ideas and rejected them for some reasons, that I haven't thought through. I just see a couple of difficulties with the current process:

  1. It seems there are currently several PRs changing the ABI version in conflicting ways, aren't there? Maybe looking at these patches daily they multiplied in my brain, but it seemed to me that I saw several patches doing 13->14, 13->15, 14->16 or whatever. So, applying them will need manual fixing.
  2. when you group features you have periods of inconsistency. Which is bad for bisecting. If you first collect N features and then bump the ABI version, before that last patch you have a wrong state. And vice versa.

But yes, I understand that it's complex and I'm not sure I have any good ideas how to improve this.

#define SOF_ABI_PATCH 0

/* SOF ABI version number. Format within 32bit word is MMmmmppp */
Expand Down