ASoC: SOF: loader: do not warn about unknown firmware headers

[kv2019i]

The firmware extended data IPC and manifest structures are designed to
be extendable without breaking the driver-firmware ABI. Driver should
not raise a warning in case a new header type is detected at
firmware boot. There are already checks for IPC ABI compatibility in
snd_sof_ipc_valid() and if the versions are deemed compatible, extra
fields in IPC messages should not trigger warnings.

Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com

[kv2019i]

@fredoh9 and @ktrzcinx I know this conflicts with #2459 and #2297 , but I'd like to get this in first and have a separate commit for it.

Emitting a warning on a codepath that is basicly run every time user plays any sound on the machine, is pretty big no-no unless something is really wrong. So let's downgrade the warning and make the ABI changes in this area a bit easier and less likely to leak dmesg spam into upstream kernels.

[fredoh9]

Looks good!

[ranj063]

typo unnessary in commit message

[kv2019i]

Oops, thanks, fixed. The checkpatch codespell doesn't catch much...

[ranj063]

well it missed one more fraffic haha

[lyakh]

maybe we need dev_warn_once()

[kv2019i]

@lyakh @ktrzcinx but is this a warning? if we have an extendable interface, parser should not be alerted when extensions are used. even the one warning will gather user attention

@ranj063 Oops, heh, I'll address that.

[kv2019i]

@ktrzcinx That's indeed useful, but at least for this example, it seems better for sof-logger to detect the debugfs interface is missing and print out an error to user that your kernel is missing memory tracing feature, please upgrade. This is especially true as parsing a manifest header does not mean the kernel has all patches needed for this feature.

@xiulipan That's another option to limit parsing only for first boot. There's a small risk in there, in that we have no guarantees the firmware is not updated on a running system.. it's actually very likely sequence of events when systems are upgraded (there is no forced reboot), so I'd be cautious to build a lot of if-firstboot-then type of logic. Printing warnings like this would maybe qualify (not really an issue if warnings don't get printed until system is fully rebooted).

Even still, let's make sure our warnings and errors do really count. Whenever we emit one, a bug gets reported and it creates work for multiple organizations (integrators, distro maintainers, our customer support and so forth), so even if it's once per boot, it still matters. If the warning can ensure, we can analyze a bug, then it's ok,but let's consider each case on its own merits.

[xiulipan]

@kv2019i Interesting topic, will runtime PM be influenced by firmware change on rootfs?

To me I think the first_boot will be reset with kmod reload and that's when the FW change take effect(we will call require firmware). So we should make ext manifest parser every time we require firmware from rootfs, then we are good with mismatch risk.

I will try to check our FW load sequence to confirm this.

[kv2019i]

@xiulipan I now checked the codepaths and I think we are safe with this.

FW:
sof_resume -> snd_sof_load_firmware() -> snd_sof_load_firmware_raw()

There's a check for "plat_data->fw" and if FW was already loaded, it's not loaded again from rootfs.

Topology is only loaded at sof-audio probe. So if you do a system upgrade on a live system, you need to reload the SOF driver modules to refresh FW and topology, and then all first_boot checks will be done.

[ranj063]

@kv2019i the commit message mentions IPC data but we're not really talking IPCs here right? Its purely extended manifest headers, isnt it?

[xiulipan]

@ranj063 Yes, I think we have add some ext_manifest both in FW (eg. SOF_IPC_EXT_CC_INFO by IPC for old drviers) and ext_manifest (parse after FW load from rootfs)

@kv2019i I am OK with this as this part will be removed in few versions right? I think it won't break anything as no new feature in driver will relay on the ext data.

[lyakh]

maybe we need dev_warn_once()

[xiulipan]

@kv2019i I double check the kernel, it seems runtime PM will not output this warning message.

The function snd_sof_fw_ext_man_parse is called alongside with request_firmware, so this warning will only take effect when boot up or kmod reload.
linux/sound/soc/sof/loader.c

Line 687 in d0a335d

ext_man_size = snd_sof_fw_ext_man_parse(sdev, plat_data->fw);
As runtime pm is enabled on most platforms and
snd_sof_fw_parse_ext_data() is called for every DSP boot, this creates
unnessary kernel dmesg fraffic.

What you said in commit will never happen

[xiulipan]

@kv2019i ha, I found I was mislead by the commit title

ASoC: SOF: loader: do not warn about unknown firmware manifest headers

But what you change is actually extend data from DSP IPC

linux/sound/soc/sof/loader.c

Lines 473 to 475 in d0a335d

	/* now check for extended data */
	snd_sof_fw_parse_ext_data(sdev, bar, offset +
	sizeof(struct sof_ipc_fw_ready));

But still, I think a better solution would be only call snd_sof_fw_parse_ext_data with first boot.

see my previous comments about request_firmware and real FW binary change may happen.

[kv2019i]

I looks like it already is done as you wish, see

linux/sound/soc/sof/loader.c

Line 462 in d0a335d

if (!sdev->first_boot)

Ack, you are correct. I tested by observing "DSP is ready" trace after each runtime-resume, but alas, that's emitted on L458, just before the boottime check. @xiulipan Maybe using "manifest" word was misleading, I did specifically intend to change snd_sof_fw_parse_ext_data() -- it's really the same data, we just used the fw_ready IPC before we had the ext-manifest, but to me it's really the same data. But yeah, agreed, better to use extended data.

[kv2019i]

@ktrzcinx @xiulipan I'll meet you halfway. :) I address both "extended data" and "extended manifest" headers in the revised patch, but as this is not hit on the the pm-resume path, I upped the trace level to INFO, so it gets logged. How about this?

[xiulipan]

If we do not need to add anything new to IPC_EXT data, it is OK to lower the message level.

[xiulipan]

@ranj063 Yes, I think we have add some ext_manifest both in FW (eg. SOF_IPC_EXT_CC_INFO by IPC for old drviers) and ext_manifest (parse after FW load from rootfs)

@kv2019i I am OK with this as this part will be removed in few versions right? I think it won't break anything as no new feature in driver will relay on the ext data.

[kv2019i]

Sorry all, the patch revision in review was not correct. :( Commit message was correct, but the changes to use dev_info were not in. I now pushed the new version, please check again. Sorry for the noise.

[ktrzcinx]

Now I'm not puzzled what's going on 👍🏼

[kv2019i]

Thanks all for the reviews, merging this.