2023-05-16 13:44:33

by Daniel Baluta (OSS)

[permalink] [raw]
Subject: [RESEND 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-16 13:55:34

by Daniel Baluta (OSS)

[permalink] [raw]
Subject: [RESEND 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-16 14:00:46

by Daniel Baluta (OSS)

[permalink] [raw]
Subject: [RESEND PATCH 2/2] ASoC: sof: Improve sof_ipc4_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/ipc4-control.c | 39 ++++++++++++++++++++++++++++++++----
1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sof/ipc4-control.c b/sound/soc/sof/ipc4-control.c
index 6f0698be9451..c6d404d44097 100644
--- a/sound/soc/sof/ipc4-control.c
+++ b/sound/soc/sof/ipc4-control.c
@@ -54,6 +54,26 @@ static int sof_ipc4_set_get_kcontrol_data(struct snd_sof_control *scontrol,
msg->primary |= SOF_IPC4_MOD_INSTANCE(swidget->instance_id);

ret = iops->set_get_data(sdev, msg, msg->data_size, set);
+ if (!set)
+ goto unlock;
+
+ /* It is a set-data operation, and we have a valid 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, msg, msg->data_size, set);
+ if (ret < 0)
+ goto unlock;
+ }

unlock:
if (lock)
@@ -327,13 +347,24 @@ static int sof_ipc4_bytes_ext_put(struct snd_sof_control *scontrol,
return -EINVAL;
}

+ 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;
+ }
+
/* Copy the whole binary data which includes the ABI header and the payload */
- if (copy_from_user(data, tlvd->tlv, header.length))
+ if (copy_from_user(data, tlvd->tlv, header.length)) {
+ 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;
return -EFAULT;
+ }

- sof_ipc4_set_get_bytes_data(sdev, scontrol, true, true);
-
- return 0;
+ return sof_ipc4_set_get_bytes_data(sdev, scontrol, true, true);
}

static int _sof_ipc4_bytes_ext_get(struct snd_sof_control *scontrol,
--
2.25.1


2023-05-16 15:05:12

by Mark Brown

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

On Tue, May 16, 2023 at 04:23:59PM +0300, Daniel Baluta wrote:
> 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.

Please allow a reasonable time for review, we're only at -rc1.


Attachments:
(No filename) (302.00 B)
signature.asc (499.00 B)
Download all attachments