LinuxLists.cc - [PATCH 0/2] Improve support for sof_ipc{3|4}_bytes_ext

2023-05-03 08:20:26

Subject: [PATCH 0/2] Improve support for sof_ipc{3|4}_bytes_ext_put

From: Daniel Baluta <[email protected]>

This patch series provides better handling of cases where sending
a data blob to FW results in a validation error.

In this case we restore to the last good known value instead of keeping
the data that firwmare rejected.

Paul Olaru (2):
ASoC: sof: Improve sof_ipc3_bytes_ext_put function
ASoC: sof: Improve sof_ipc4_bytes_ext_put function

sound/soc/sof/ipc3-control.c | 54 ++++++++++++++++++++++++++++++++----
sound/soc/sof/ipc4-control.c | 39 +++++++++++++++++++++++---
sound/soc/sof/sof-audio.h | 1 +
3 files changed, 84 insertions(+), 10 deletions(-)

--
2.25.1

2023-05-03 08:20:42

by Daniel Baluta (OSS)

[permalink] [raw]

Subject: [PATCH 1/2] ASoC: sof: Improve sof_ipc3_bytes_ext_put function

From: Paul Olaru <[email protected]>

The function is improved in the way that if the firmware returns a
validation error on the newly sent bytes, then the kernel will
automatically restore to the old bytes value for a given kcontrol.

This way, if the firmware rejects a data blob then the kernel will also
reject it, instead of saving it for the next suspend/resume cycle. The
old behaviour is that the kernel would save it anyway and on next
firmware boot it would apply the previously-rejected configuration,
leading to errors during playback.

Additionally, the function also saves previously validated
configurations, so that if the firmware does end up rejecting a new
bytes value the kernel can send an old, previously-valid configuration.

Reviewed-by: Daniel Baluta <[email protected]>
Reviewed-by: Péter Ujfalusi <[email protected]>
Signed-off-by: Paul Olaru <[email protected]>
Signed-off-by: Daniel Baluta <[email protected]>
---
sound/soc/sof/ipc3-control.c | 54 ++++++++++++++++++++++++++++++++----
sound/soc/sof/sof-audio.h | 1 +
2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sof/ipc3-control.c b/sound/soc/sof/ipc3-control.c
index ad040e7bb850..a8deec7dc021 100644
--- a/sound/soc/sof/ipc3-control.c
+++ b/sound/soc/sof/ipc3-control.c
@@ -96,6 +96,26 @@ static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol,
cdata->elems_remaining = 0;

ret = iops->set_get_data(sdev, cdata, cdata->rhdr.hdr.size, set);
+ if (!set)
+ goto unlock;
+
+ /* It is a set-data operation, and we have a backup that we can restore */
+ if (ret < 0) {
+ if (!scontrol->old_ipc_control_data)
+ goto unlock;
+ /*
+ * Current ipc_control_data is not valid, we use the last known good
+ * configuration
+ */
+ memcpy(scontrol->ipc_control_data, scontrol->old_ipc_control_data,
+ scontrol->max_size);
+ kfree(scontrol->old_ipc_control_data);
+ scontrol->old_ipc_control_data = NULL;
+ /* Send the last known good configuration to firmware */
+ ret = iops->set_get_data(sdev, cdata, cdata->rhdr.hdr.size, set);
+ if (ret < 0)
+ goto unlock;
+ }

unlock:
if (lock)
@@ -351,6 +371,7 @@ static int sof_ipc3_bytes_ext_put(struct snd_sof_control *scontrol,
struct sof_ipc_ctrl_data *cdata = scontrol->ipc_control_data;
struct snd_soc_component *scomp = scontrol->scomp;
struct snd_ctl_tlv header;
+ int ret = -EINVAL;

/*
* The beginning of bytes data contains a header from where
@@ -381,31 +402,52 @@ static int sof_ipc3_bytes_ext_put(struct snd_sof_control *scontrol,
return -EINVAL;
}

- if (copy_from_user(cdata->data, tlvd->tlv, header.length))
- return -EFAULT;
+ if (!scontrol->old_ipc_control_data) {
+ /* Create a backup of the current, valid bytes control */
+ scontrol->old_ipc_control_data = kmemdup(scontrol->ipc_control_data,
+ scontrol->max_size, GFP_KERNEL);
+ if (!scontrol->old_ipc_control_data)
+ return -ENOMEM;
+ }
+
+ if (copy_from_user(cdata->data, tlvd->tlv, header.length)) {
+ ret = -EFAULT;
+ goto err_restore;
+ }

if (cdata->data->magic != SOF_ABI_MAGIC) {
dev_err_ratelimited(scomp->dev, "Wrong ABI magic 0x%08x\n", cdata->data->magic);
- return -EINVAL;
+ goto err_restore;
}

if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_VERSION, cdata->data->abi)) {
dev_err_ratelimited(scomp->dev, "Incompatible ABI version 0x%08x\n",
cdata->data->abi);
- return -EINVAL;
+ goto err_restore;
}

/* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */
if (cdata->data->size > scontrol->max_size - sizeof(struct sof_abi_hdr)) {
dev_err_ratelimited(scomp->dev, "Mismatch in ABI data size (truncated?)\n");
- return -EINVAL;
+ goto err_restore;
}

/* notify DSP of byte control updates */
- if (pm_runtime_active(scomp->dev))
+ if (pm_runtime_active(scomp->dev)) {
+ /* Actually send the data to the DSP; this is an opportunity to validate the data */
return sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
+ }

return 0;
+
+err_restore:
+ /* If we have an issue, we restore the old, valid bytes control data */
+ if (scontrol->old_ipc_control_data) {
+ memcpy(cdata->data, scontrol->old_ipc_control_data, scontrol->max_size);
+ kfree(scontrol->old_ipc_control_data);
+ scontrol->old_ipc_control_data = NULL;
+ }
+ return ret;
}

static int _sof_ipc3_bytes_ext_get(struct snd_sof_control *scontrol,
diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
index a090a9eb4828..5d5eeb1a1a6f 100644
--- a/sound/soc/sof/sof-audio.h
+++ b/sound/soc/sof/sof-audio.h
@@ -362,6 +362,7 @@ struct snd_sof_control {
size_t priv_size; /* size of private data */
size_t max_size;
void *ipc_control_data;
+ void *old_ipc_control_data;
int max; /* applicable to volume controls */
u32 size; /* cdata size */
u32 *volume_table; /* volume table computed from tlv data*/
--
2.25.1

2023-05-03 08:21:04

by Daniel Baluta (OSS)

[permalink] [raw]

Subject: [PATCH 2/2] ASoC: sof: Improve sof_ipc4_bytes_ext_put function

2023-05-23 21:51:13

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 0/2] Improve support for sof_ipc{3|4}_bytes_ext_put

On Wed, 03 May 2023 11:10:47 +0300, Daniel Baluta wrote:
> This patch series provides better handling of cases where sending
> a data blob to FW results in a validation error.
>
> In this case we restore to the last good known value instead of keeping
> the data that firwmare rejected.
>
> Paul Olaru (2):
> ASoC: sof: Improve sof_ipc3_bytes_ext_put function
> ASoC: sof: Improve sof_ipc4_bytes_ext_put function
>
> [...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: sof: Improve sof_ipc3_bytes_ext_put function
commit: 299f6c752f8f7dabb62fe4df62ebd233b58402bd
[2/2] ASoC: sof: Improve sof_ipc4_bytes_ext_put function
commit: db38d86d0c54e0dbea063e915ce3e1fe394af444

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark