Skip to content

ASoC: SOF: rework FW ABI version handling on kernel side #3296

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

Closed
kv2019i wants to merge 1 commit into from
Closed

ASoC: SOF: rework FW ABI version handling on kernel side #3296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 0 additions & 22 deletions include/uapi/sound/sof/abi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,9 @@
* Copyright(c) 2018 Intel Corporation. All rights reserved.
*/

/**
* SOF ABI versioning is based on Semantic Versioning where we have a given
* MAJOR.MINOR.PATCH version number. See https://semver.org/
*
* Rules for incrementing or changing version :-
*
* 1) Increment MAJOR version if you make incompatible API changes. MINOR and
* PATCH should be reset to 0.
*
* 2) Increment MINOR version if you add backwards compatible features or
* changes. PATCH should be reset to 0.
*
* 3) Increment PATCH version if you add backwards compatible bug fixes.
*/

#ifndef __INCLUDE_UAPI_SOUND_SOF_ABI_H__
#define __INCLUDE_UAPI_SOUND_SOF_ABI_H__

/* SOF ABI version major, minor and patch numbers */
#define SOF_ABI_MAJOR 3
#define SOF_ABI_MINOR 18
#define SOF_ABI_PATCH 0

/* SOF ABI version number. Format within 32bit word is MMmmmppp */
#define SOF_ABI_MAJOR_SHIFT 24
#define SOF_ABI_MAJOR_MASK 0xff
Expand All @@ -54,8 +34,6 @@
SOF_ABI_VERSION_MAJOR((client_ver)) \
)

#define SOF_ABI_VERSION SOF_ABI_VER(SOF_ABI_MAJOR, SOF_ABI_MINOR, SOF_ABI_PATCH)

/* SOF ABI magic number "SOF\0". */
#define SOF_ABI_MAGIC 0x00464F53

Expand Down
15 changes: 0 additions & 15 deletions sound/soc/sof/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,21 +111,6 @@ config SND_SOC_SOF_NOCODEC_SUPPORT
Say Y if you need this nocodec fallback option.
If unsure select "N".

config SND_SOC_SOF_STRICT_ABI_CHECKS
bool "SOF strict ABI checks"
help
This option enables strict ABI checks for firmware and topology
files.
When these files are more recent than the kernel, the kernel
will handle the functionality it supports and may report errors
during topology creation or run-time usage if new functionality
is invoked.
This option will stop topology creation and firmware load upfront.
It is intended for SOF CI/releases and not for users or distros.
Say Y if you want strict ABI checks for an SOF release.
If you are not involved in SOF releases and CI development,
select "N".

config SND_SOC_SOF_DEBUG
bool "SOF debugging features"
help
Expand Down
10 changes: 7 additions & 3 deletions sound/soc/sof/control.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
#include "sof-priv.h"
#include "sof-audio.h"

/* Interface version implemented for controls IPC */
#define SOF_CONTROL_ABI_VERSION SOF_ABI_VER(3, 18, 0)
Copy link
Collaborator

@marc-hb marc-hb Nov 24, 2021

Choose a reason for hiding this comment

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

Where is the authoritative list of new features added to each MINOR increment? Is it https://github.com/orgs/thesofproject/projects/2? Either way, add a link to it somewhere in the source code?


static void update_mute_led(struct snd_sof_control *scontrol,
struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
Expand Down Expand Up @@ -79,7 +82,7 @@ static void snd_sof_refresh_control(struct snd_sof_control *scontrol)

/* set the ABI header values */
cdata->data->magic = SOF_ABI_MAGIC;
Copy link

@cujomalainey cujomalainey Dec 3, 2021

Choose a reason for hiding this comment

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

for ease in the future for changing ABI, we should probably use an "init_ipc" function so we only have to change in one place

cdata->data->abi = SOF_ABI_VERSION;
cdata->data->abi = SOF_CONTROL_ABI_VERSION;

/* refresh the component data from DSP */
scontrol->comp_data_dirty = false;
Expand Down Expand Up @@ -433,7 +436,8 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _

/* set the ABI header values */
cdata->data->magic = SOF_ABI_MAGIC;
cdata->data->abi = SOF_ABI_VERSION;
cdata->data->abi = SOF_CONTROL_ABI_VERSION;

/* get all the component data from DSP */
ret = snd_sof_ipc_set_get_comp_data(scontrol, false);
if (ret < 0)
Expand Down Expand Up @@ -500,7 +504,7 @@ int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol,

/* set the ABI header values */
cdata->data->magic = SOF_ABI_MAGIC;
cdata->data->abi = SOF_ABI_VERSION;
cdata->data->abi = SOF_CONTROL_ABI_VERSION;

/* check data size doesn't exceed max coming from topology */
if (cdata->data->size > be->max - sizeof(struct sof_abi_hdr)) {
Expand Down
14 changes: 2 additions & 12 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,26 +947,16 @@ int snd_sof_ipc_valid(struct snd_sof_dev *sdev)
"Firmware info: version %d:%d:%d-%s\n", v->major, v->minor,
v->micro, v->tag);
dev_info(sdev->dev,
"Firmware: ABI %d:%d:%d Kernel ABI %d:%d:%d\n",
"Firmware: ABI %d:%d:%d\n",
Copy link
Collaborator

@marc-hb marc-hb Nov 24, 2021

Choose a reason for hiding this comment

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

Suggested change
"Firmware: ABI %d:%d:%d\n",
"Firmware ABI: %d.%d.%d\n",

SOF_ABI_VERSION_MAJOR(v->abi_version),
SOF_ABI_VERSION_MINOR(v->abi_version),
SOF_ABI_VERSION_PATCH(v->abi_version),
SOF_ABI_MAJOR, SOF_ABI_MINOR, SOF_ABI_PATCH);
SOF_ABI_VERSION_PATCH(v->abi_version));

if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_VERSION, v->abi_version)) {
dev_err(sdev->dev, "error: incompatible FW ABI version\n");
return -EINVAL;
}

if (SOF_ABI_VERSION_MINOR(v->abi_version) > SOF_ABI_MINOR) {
if (!IS_ENABLED(CONFIG_SND_SOC_SOF_STRICT_ABI_CHECKS)) {
dev_warn(sdev->dev, "warn: FW ABI is more recent than kernel\n");
} else {
dev_err(sdev->dev, "error: FW ABI is more recent than kernel\n");
return -EINVAL;
}
}

if (ready->flags & SOF_IPC_INFO_BUILD) {
dev_info(sdev->dev,
"Firmware debug build %d on %s-%s - options:\n"
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ bool sof_debug_check_flag(int mask);
/* max number of DSP cores */
#define SOF_MAX_DSP_NUM_CORES 8

/* IPC interface version implemented in driver */
#define SOF_ABI_VERSION SOF_ABI_VER(3, 0, 0)
Copy link

@cujomalainey cujomalainey Dec 3, 2021

Choose a reason for hiding this comment

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

are we certain the kernel handles all versions back this far?

Copy link
Collaborator

@ranj063 ranj063 Dec 9, 2021

Choose a reason for hiding this comment

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

I dont think this is correct @kv2019i. We do need to the minor versions I think.

Copy link
Collaborator

@ranj063 ranj063 Dec 9, 2021

Choose a reason for hiding this comment

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

sorry I take that back. I got confused with the firmware ABI version which is stored in sdev. This is fine

Copy link
Collaborator Author

@kv2019i kv2019i Dec 10, 2021

Choose a reason for hiding this comment

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

@cujomalainey The ABI process has evolved quite a bit since ABI3.0.0, but yes, we have no known issues with backward compatibility. In early times, the MINOR was bumped many times in the a single release window and first SOF release with major set to 3 was SOF1.3 and this was ABI3.7.0. So we have no public releases that are older than this. SOF1.4 was ABI3.11 and that is probably still in use in some systems (some distros still ship old firmware binaries but users update the kernel, so this will actually happen).


struct sof_dsp_power_state {
u32 state;
u32 substate; /* platform-specific */
Expand Down
16 changes: 2 additions & 14 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3580,11 +3580,8 @@ static int sof_manifest(struct snd_soc_component *scomp, int index,
return -EINVAL;
}

dev_info(scomp->dev,
"Topology: ABI %d:%d:%d Kernel ABI %d:%d:%d\n",
man->priv.data[0], man->priv.data[1],
man->priv.data[2], SOF_ABI_MAJOR, SOF_ABI_MINOR,
SOF_ABI_PATCH);
dev_info(scomp->dev, "Topology: ABI %d:%d:%d\n",
Copy link
Collaborator

@marc-hb marc-hb Nov 24, 2021

Choose a reason for hiding this comment

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

Suggested change
dev_info(scomp->dev, "Topology: ABI %d:%d:%d\n",
dev_info(scomp->dev, "Topology ABI: %d.%d.%d\n",

man->priv.data[0], man->priv.data[1], man->priv.data[2]);

abi_version = SOF_ABI_VER(man->priv.data[0],
man->priv.data[1],
Expand All @@ -3595,15 +3592,6 @@ static int sof_manifest(struct snd_soc_component *scomp, int index,
return -EINVAL;
}

if (SOF_ABI_VERSION_MINOR(abi_version) > SOF_ABI_MINOR) {
if (!IS_ENABLED(CONFIG_SND_SOC_SOF_STRICT_ABI_CHECKS)) {
dev_warn(scomp->dev, "warn: topology ABI is more recent than kernel\n");
} else {
dev_err(scomp->dev, "error: topology ABI is more recent than kernel\n");
return -EINVAL;
}
}

Copy link
Member

@plbossart plbossart Nov 29, 2021

Choose a reason for hiding this comment

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

I don't really mind if we remove the checks, but now how do we update the shared files when there is a change to the IPC structures? This was precisely when we bumped the ABI on the kernel side, so now there's no information to tell you what the kernel WILL support.

Not convinced or missing the point.

Copy link
Collaborator Author

@kv2019i kv2019i Nov 30, 2021

Choose a reason for hiding this comment

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

@plbossart Problem is the kernel update to a new ABI is not atomic. FW is released once per quarter with multiple distinct changes covered by a single ABI change. Sometimes kernel will only implement a subset and cirtically, sometimes a kernel commit depending on ABI extension will get selectively backported (like will happen for the stop-dma fix which requires 3.20).

In this context, a single ABI version for kernel side implementation is rather impossible to maintain. We get problems like this: #3298 (comment) (i.e. can we declare ABI3.20 now)

We could change the approach and make the kernel version just refer to the shared files. That could be done more systematically, but would that be useful. The fact that headers are updated does not imply the implementation is updated.

Copy link
Member

@plbossart plbossart Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would be happy with a kernel level version that refers only to the header files, to help with an elimination process when dealing with bug reports.

If these files do not have the relevant version, then for sure the new features exposed by the firmware will not be used. That's already useful information.

If the files do have the relevant version, we would have to check if all the relevant patches provide the implementation for the ABI change, as you rightly noted it's not an atomic update.

Copy link
Collaborator Author

@kv2019i kv2019i Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@plbossart W.r.t. the option of tying the version to header alone. This will have some implications to our kernel upstreaming process. Let's say we have ABI extensions A, B and C in SOF2.1, bumping ABI to 3.21. Let's say we implement feature A in January and have kernel patches adding the definitions to kernel, and implementation in sof-dev. At this point, the version in kernel header is not correct as it only has the change of A, so it's a mix of ABU3.20 and ABI3.21. They don't match the header of same version in firmware (as SOF2.1 is not yet complete).

Plus we need to have a maintainer check the ABI and make a patch for every SOF release (e.g. my #3298 ).

If we are ok with this, then I can update #3298 and close this.

UPDATE: more issues. I noticed we are missing header bits in kernel. E.g. ABI3.19 added IMR support in firmware, but we have no support for it in kernel yet. (e.g. SOF_IPC_INFO_D3_PERSISTENT bit definition). So in practise sof-dev kernel has bits of ABI3.20 (stop-dma) but it does not yet have patches for ABI3.19. So I'd now have to go and look at the delta manually to make a kernel patch to bump the version of the interface files? Yuk.

Copy link
Member

@plbossart plbossart Dec 3, 2021

Choose a reason for hiding this comment

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

yes we'll never get it completely right, but we can treat this value as informative for debug.

ok to remove the strict checks and the warning, we can make it an info level.

Copy link
Collaborator Author

@kv2019i kv2019i Dec 10, 2021

Choose a reason for hiding this comment

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

@plbossart We do still need to update the ABI process documentation to ensure the files get updated. Otherwise the data will be incorrect and not useful even for debug. I don't have time to do this now, so I'll close this PR. There also seems to be IPC4 changes coming that will anyways change the headers, so let's focus on those.

return 0;
}

Expand Down