Repository navigation
Warning emitted if FW revision is newer than what kernel knows about #2647
Description
Latest sof-dev (03b55f5) with FW version SOF1.6 emits following warnings:
[24943.815895] sof-audio-pci 0000:00:1f.3: Firmware: ABI 3:18:1 Kernel ABI 3:18:0
[24943.815896] sof-audio-pci 0000:00:1f.3: warn: FW ABI is more recent than kernel
If CONFIG_SND_SOC_SOF_STRICT_ABI_CHECKS=y FW load will fail..
As only PATCH version is changed, a warning should not be emitted. Following ABI semantics, PATCH level updates should be fully backward compatible.
If we don't relax the check, we'd have to make continuous updates to kernel IPC definitions whenever FW is changed. This is no longer the case with the relaxed ABI change process introduced this year, allowing compatible ABI changes to be merged at different base to kernen and firmware master branches.
FYI @lgirdwood @plbossart
Activity
lgirdwood commentedon Jan 4, 2021
@kv2019i fwiw, this PATCH ABI version ignore should be a "fix" for earlier kernels.
plbossart commentedon Jan 11, 2021
@kv2019i I don't mind if we remove the warning if only the 'PATCH' level is different.
But having "CONFIG_SND_SOC_SOF_STRICT_ABI_CHECKS" means you really want to test the same versions. It's useful for tests to detect the ABI versions are not aligned, and e.g. the ABI changes at the firmware level were not followed by a kernel change.
kv2019i commentedon Jan 12, 2021
@plbossart The one issue with that is that it forces us to do somewhat odd patches to upstream kernel. E.g. we would now need to do a patch that just updates the kernel PATCH level to 3.18.1, with no functional change elsewhere in the codebase.
You can say this makes it explicit kernel developers have reviewed FW 3.18.1 ABI, and it has been deemed compatible. But given PATCH-LEVEL changes should be ABI compatible anyways, there should be nothing to check. So this really comes down to being a "testing statement" that I have tested kernel with 3.18.1 FW and can verify it works ok, thus bumping kernel side version to match.
I know this is a bit academic as we've only ever had one change of PATCH-LEVEL, but how we handle this one case, will set the precedent for any future use.
To counter myself, I can imagine we could have a case where we have a bug in implementation and we'd increment the PATCH-LEVEL to indicate that a version of FW has the required fix. Driver side could then have an additional check to emit an error if PATCH-LEVEL is not correct. E.g. let's say we'd have a bug in PM_GATE IPC. It would be ok for driver (that uses PM_GATE) to use the PATCH-LEVEL as well to ensure the FW is not just compatible but has the fix.
But even here, this would be an additional check, and specific to one ABI major-minor version. In generic terms, PATCH-LEVEL should not be included in ABI compatibility checks.
plbossart commentedon Jan 12, 2021
Sorry @kv2019i I don't get your point. When the PATCH level is changed, surely some structure must be changed as well, no? why do you say there is 'no functional change'? is there an example of what the PATCH level change does?
kv2019i commentedon Jan 13, 2021
@plbossart That's why I stated this is somewhat of an academic discussion. To my knowledge, in the four years of history we have for SOF, there's only case where a non-zero PATCH level has been used. And as it's a once-in-four-years part of the process, the best practises have not been established.
thesofproject/sof#3543
FYI to @cujomalainey and @lgirdwood
So this is a change to abi header, but based on review, there is no functional impact so only PATCH level was changed. In retrospect, maybe we should not have bumped the patch level even. It seemed harmless at the time. Curtis did send a kernel PR, but I didn't want to take it as it seemed wrong to just update a number in kernel header, without any functional change or nobody checking for the version.
A more useful usage of PATCH level would perhaps be a change that has no change to interface, but a change in IPC implementation that might be important to some clients. By bumping the PATCH in FW, this allows clients to detect (at runtime) combinations of drirver+fw that are known not to work. In short, you express that "FW must be updated, but no changes are needed on client side".
But yeah. interface versioning is really hard. I personally prefer the libtool approach, where each version of the (library) interface is identified with a running number (current), and you have two additional numbers to describe implementation's relation to other implementations of the same library:
To assess compatibility, linker will only consider the two numbers current and age. Revision is only considered if there are multiple libraries in the system meeting the requirements. In that case, linker will choose the library with highest revision:
https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
In our semver.org numbering scheme (which is ok as well but just in my experience needs more discussion about the practical rules when to update the numbers), I think SOF driver should only consider MAJOR.MINOR when checking ABI. We cannot have multiple firmware versions, so any generic check on PATCH level is not possible. It can only be used to identify known bad combinations, but these should be the rare exception.
plbossart commentedon Jan 13, 2021
@kv2019i thanks for the pointer and yeah, I don't know why the PATCH level was bumped for that PR, that causes confusion. I am fine ignoring the PATCH level if this is meant for this kind of non-functional changes.
cujomalainey commentedon Jan 13, 2021
I'm pretty sure i bumped the abi as patchcheck screams if you touch those files and was relying on ABI reviewers to guide me otherwise. I think firmware landed before kernel so it was too late by time kernel change was requested.
lgirdwood commentedon Jan 18, 2021
PATCH not really used that much as we have been in the rapid state of feature development as opposed to glibc where it's the opposite. @plbossart you can ignore PATCH for compatibility reasons, but we should still print it for reference.
marc-hb commentedon Jan 31, 2021
https://semver.org/ is designed to version releases, not to version interfaces. Not APIs, not ABIs. I think a lot of the confusion and/or misunderstandings on this page come from this.
At the top of https://semver.org/:
In other words: increment the PATCH level when you make a release with absolutely zero API/ABI changes.
In yet other words: when "hijacking" semver.org to version an interface instead of a release, the PATCH level makes no sense. Probably why it was used only once in 4 years.
ASoC: SOF: relax ABI checks and avoid unnecessary warnings
ASoC: SOF: relax ABI checks and avoid unnecessary warnings
ASoC: SOF: relax ABI checks and avoid unnecessary warnings
ASoC: SOF: relax ABI checks and avoid unnecessary warnings
kv2019i commentedon Feb 16, 2021
Fixed and merged upstream.
marc-hb commentedon Nov 12, 2021
A PATCH level makes sense only on the implementation side. So in general it does not make semver sense to define
SOF_ABI_PATCHon the user side, that#ifdefcould be removed entirely from the kernel. I mean it does not make sense in general for any API/ABI user side to requires a specific level of bug fixes in the implementation, that does not follow the semver.org definitions.Fair enough. As you wrote, semver is not perfect.
marc-hb commentedon Nov 16, 2021
Funny enough most recent SOF release notes require a kernel version with a MINOR too low that also triggers this warning: thesofproject/sof-bin#9 (comment)
UPSTREAM: ASoC: SOF: relax ABI checks and avoid unnecessary warnings