The size of the data behind of scontrol->ipc_control_data for bytes controls is: [1] sizeof(struct sof_ipc4_control_data) + // kernel only struct [2] sizeof(struct sof_abi_hdr)) + payload

The max_size specifies the size of [2] and it is coming from topology.

Change the function to take this into account and allocate adequate amount of memory behind scontrol->ipc_control_data.

With the change we will allocate [1] amount more memory to be able to hold the full size of data.

Fixes: a062c8899fed ("ASoC: SOF: ipc4-topology: add byte kcontrol support") Cc: stable@vger.kernel.org Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/ipc4-topology.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c index 221e9d4052b8..4272d84679ac 100644 --- a/sound/soc/sof/ipc4-topology.c +++ b/sound/soc/sof/ipc4-topology.c @@ -2855,22 +2855,41 @@ static int sof_ipc4_control_load_bytes(struct snd_sof_dev *sdev, struct snd_sof_ struct sof_ipc4_msg *msg; int ret;

- if (scontrol->max_size < (sizeof(*control_data) + sizeof(struct sof_abi_hdr))) { - dev_err(sdev->dev, "insufficient size for a bytes control %s: %zu.\n", + /* + * The max_size is coming from topology and indicates the maximum size + * of sof_abi_hdr plus the payload, which excludes the local only + * 'struct sof_ipc4_control_data' + */ + if (scontrol->max_size < sizeof(struct sof_abi_hdr)) { + dev_err(sdev->dev, + "insufficient maximum size for a bytes control %s: %zu.\n", scontrol->name, scontrol->max_size); return -EINVAL; }

- if (scontrol->priv_size > scontrol->max_size - sizeof(*control_data)) { - dev_err(sdev->dev, "scontrol %s bytes data size %zu exceeds max %zu.\n", - scontrol->name, scontrol->priv_size, - scontrol->max_size - sizeof(*control_data)); + if (scontrol->priv_size > scontrol->max_size) { + dev_err(sdev->dev, + "bytes control %s initial data size %zu exceeds max %zu.\n", + scontrol->name, scontrol->priv_size, scontrol->max_size); + return -EINVAL; + } + + if (scontrol->priv_size < sizeof(struct sof_abi_hdr)) { + dev_err(sdev->dev, + "bytes control %s initial data size %zu is insufficient.\n", + scontrol->name, scontrol->priv_size); return -EINVAL; }

- scontrol->size = sizeof(struct sof_ipc4_control_data) + scontrol->priv_size; + /* + * The used size behind the cdata pointer, which can be smaller than + * the maximum size + */ + scontrol->size = sizeof(*control_data) + scontrol->priv_size;

- scontrol->ipc_control_data = kzalloc(scontrol->max_size, GFP_KERNEL); + /* Allocate the cdata: local struct size + maximum payload size */ + scontrol->ipc_control_data = kzalloc(sizeof(*control_data) + scontrol->max_size, + GFP_KERNEL); if (!scontrol->ipc_control_data) return -ENOMEM;

When the bytes data is read from the firmware, the size of the payload can be different than what it was previously. For example when the topology did not contained payload data at all for the control, the data size was 0. For get operation allow maximum size of payload to be read and then update the sizes according to the completed message.

Similarly, keep the size in sync when updating the data in firmware.

With the change we will be able to read data from firmware for bytes controls which did not had initial payload defined in topology.

Fixes: a062c8899fed ("ASoC: SOF: ipc4-control: Add support for bytes control get and put") Cc: stable@vger.kernel.org Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/ipc4-control.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sof/ipc4-control.c b/sound/soc/sof/ipc4-control.c index 80111672c179..453ed1643b89 100644 --- a/sound/soc/sof/ipc4-control.c +++ b/sound/soc/sof/ipc4-control.c @@ -426,13 +426,21 @@ static int sof_ipc4_set_get_bytes_data(struct snd_sof_dev *sdev, msg->extension = SOF_IPC4_MOD_EXT_MSG_PARAM_ID(data->type);

msg->data_ptr = data->data; - msg->data_size = data->size; + if (set) + msg->data_size = data->size; + else + msg->data_size = scontrol->max_size - sizeof(*data);

ret = sof_ipc4_set_get_kcontrol_data(scontrol, set, lock); - if (ret < 0) + if (ret < 0) { dev_err(sdev->dev, "Failed to %s for %s\n", set ? "set bytes update" : "get bytes", scontrol->name); + } else if (!set) { + /* Update the sizes according to the received payload data */ + data->size = msg->data_size; + scontrol->size = sizeof(*cdata) + sizeof(*data) + data->size; + }

msg->data_ptr = NULL; msg->data_size = 0; @@ -448,6 +456,7 @@ static int sof_ipc4_bytes_put(struct snd_sof_control *scontrol, struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); struct sof_abi_hdr *data = cdata->data; size_t size; + int ret;

if (scontrol->max_size > sizeof(ucontrol->value.bytes.data)) { dev_err_ratelimited(scomp->dev, @@ -469,9 +478,12 @@ static int sof_ipc4_bytes_put(struct snd_sof_control *scontrol, /* copy from kcontrol */ memcpy(data, ucontrol->value.bytes.data, size);

- sof_ipc4_set_get_bytes_data(sdev, scontrol, true, true); + ret = sof_ipc4_set_get_bytes_data(sdev, scontrol, true, true); + if (!ret) + /* Update the cdata size */ + scontrol->size = sizeof(*cdata) + size;

- return 0; + return ret; }

static int sof_ipc4_bytes_get(struct snd_sof_control *scontrol, @@ -581,6 +593,9 @@ static int sof_ipc4_bytes_ext_put(struct snd_sof_control *scontrol, return -EFAULT; }

+ /* Update the cdata size */ + scontrol->size = sizeof(*cdata) + header.length; + return sof_ipc4_set_get_bytes_data(sdev, scontrol, true, true); }

There are message types when we would need to send a payload along with the LARGE_CONFIG_GET message to provide information to the firmware on what data is requested. Such cases are the ALSA Kcontrol related messages when the high level param_id tells only the type of the control, but the ID/index of the exact control is specified in the payload area.

The caller must place the payload for TX before calling the set_get_data() and this payload will be sent alongside with the message to the firmware.

The data area will be overwritten by the received data from firmware.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/ipc4.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/ipc4.c b/sound/soc/sof/ipc4.c index a4a090e6724a..20d723f48fff 100644 --- a/sound/soc/sof/ipc4.c +++ b/sound/soc/sof/ipc4.c @@ -15,6 +15,7 @@ #include "sof-audio.h" #include "ipc4-fw-reg.h" #include "ipc4-priv.h" +#include "ipc4-topology.h" #include "ipc4-telemetry.h" #include "ops.h"

@@ -433,6 +434,23 @@ static int sof_ipc4_tx_msg(struct snd_sof_dev *sdev, void *msg_data, size_t msg_ return ret; }

+static bool sof_ipc4_tx_payload_for_get_data(struct sof_ipc4_msg *tx) +{ + /* + * Messages that require TX payload with LARGE_CONFIG_GET. + * The TX payload is placed into the IPC message data section by caller, + * which needs to be copied to temporary buffer since the received data + * will overwrite it. + */ + switch (tx->extension & SOF_IPC4_MOD_EXT_MSG_PARAM_ID_MASK) { + case SOF_IPC4_MOD_EXT_MSG_PARAM_ID(SOF_IPC4_SWITCH_CONTROL_PARAM_ID): + case SOF_IPC4_MOD_EXT_MSG_PARAM_ID(SOF_IPC4_ENUM_CONTROL_PARAM_ID): + return true; + default: + return false; + } +} + static int sof_ipc4_set_get_data(struct snd_sof_dev *sdev, void *data, size_t payload_bytes, bool set) { @@ -444,6 +462,8 @@ static int sof_ipc4_set_get_data(struct snd_sof_dev *sdev, void *data, struct sof_ipc4_msg tx = {{ 0 }}; struct sof_ipc4_msg rx = {{ 0 }}; size_t remaining = payload_bytes; + void *tx_payload_for_get = NULL; + size_t tx_data_size = 0; size_t offset = 0; size_t chunk_size; int ret; @@ -469,10 +489,20 @@ static int sof_ipc4_set_get_data(struct snd_sof_dev *sdev, void *data,

tx.extension |= SOF_IPC4_MOD_EXT_MSG_FIRST_BLOCK(1);

+ if (sof_ipc4_tx_payload_for_get_data(&tx)) { + tx_data_size = min(ipc4_msg->data_size, payload_limit); + tx_payload_for_get = kmemdup(ipc4_msg->data_ptr, tx_data_size, + GFP_KERNEL); + if (!tx_payload_for_get) + return -ENOMEM; + } + /* ensure the DSP is in D0i0 before sending IPC */ ret = snd_sof_dsp_set_power_state(sdev, &target_state); - if (ret < 0) + if (ret < 0) { + kfree(tx_payload_for_get); return ret; + }

/* Serialise IPC TX */ mutex_lock(&sdev->ipc->tx_mutex); @@ -506,7 +536,15 @@ static int sof_ipc4_set_get_data(struct snd_sof_dev *sdev, void *data, rx.data_size = chunk_size; rx.data_ptr = ipc4_msg->data_ptr + offset;

- tx_size = 0; + if (tx_payload_for_get) { + tx_size = tx_data_size; + tx.data_size = tx_size; + tx.data_ptr = tx_payload_for_get; + } else { + tx_size = 0; + tx.data_size = 0; + tx.data_ptr = NULL; + } rx_size = chunk_size; }

@@ -553,6 +591,8 @@ static int sof_ipc4_set_get_data(struct snd_sof_dev *sdev, void *data,

mutex_unlock(&sdev->ipc->tx_mutex);

+ kfree(tx_payload_for_get); + return ret; }

The generic byte control can be used in cases when the bytes data can be changed by the firmware and it sends a notification about the change, similarly to the enum and switch controls.

The generic control support is needed as from the param_id itself it is not possible to know which control has changed. The needed information is only available via generic control change notification.

Generic bytes controls use param_id 202 and their change notification can contain payload with the change embedded or just the header message as notification.

Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/ipc4-control.c | 156 +++++++++++++++++++++++++++++++---- 1 file changed, 142 insertions(+), 14 deletions(-)

diff --git a/sound/soc/sof/ipc4-control.c b/sound/soc/sof/ipc4-control.c index 453ed1643b89..0500b690f9a3 100644 --- a/sound/soc/sof/ipc4-control.c +++ b/sound/soc/sof/ipc4-control.c @@ -284,6 +284,105 @@ static void sof_ipc4_refresh_generic_control(struct snd_sof_control *scontrol) kfree(data); }

+static int +sof_ipc4_set_bytes_control_data(struct snd_sof_control *scontrol, bool lock) +{ + struct sof_ipc4_control_data *cdata = scontrol->ipc_control_data; + struct snd_soc_component *scomp = scontrol->scomp; + struct sof_ipc4_control_msg_payload *msg_data; + struct sof_abi_hdr *data = cdata->data; + struct sof_ipc4_msg *msg = &cdata->msg; + size_t data_size; + int ret; + + data_size = struct_size(msg_data, data, data->size); + msg_data = kzalloc(data_size, GFP_KERNEL); + if (!msg_data) + return -ENOMEM; + + msg_data->id = cdata->index; + msg_data->num_elems = data->size; + memcpy(msg_data->data, data->data, data->size); + + msg->extension = SOF_IPC4_MOD_EXT_MSG_PARAM_ID(data->type); + + msg->data_ptr = msg_data; + msg->data_size = data_size; + + ret = sof_ipc4_set_get_kcontrol_data(scontrol, true, lock); + msg->data_ptr = NULL; + msg->data_size = 0; + if (ret < 0) + dev_err(scomp->dev, "%s: Failed to set control update for %s\n", + __func__, scontrol->name); + + kfree(msg_data); + + return ret; +} + +static int +sof_ipc4_refresh_bytes_control(struct snd_sof_control *scontrol, bool lock) +{ + struct sof_ipc4_control_data *cdata = scontrol->ipc_control_data; + struct snd_soc_component *scomp = scontrol->scomp; + struct sof_ipc4_control_msg_payload *msg_data; + struct sof_abi_hdr *data = cdata->data; + struct sof_ipc4_msg *msg = &cdata->msg; + size_t data_size; + int ret = 0; + + if (!scontrol->comp_data_dirty) + return 0; + + if (!pm_runtime_active(scomp->dev)) + return 0; + + data_size = scontrol->max_size - sizeof(*data); + if (data_size < sizeof(*msg_data)) + data_size = sizeof(*msg_data); + + msg_data = kzalloc(data_size, GFP_KERNEL); + if (!msg_data) + return -ENOMEM; + + msg->extension = SOF_IPC4_MOD_EXT_MSG_PARAM_ID(data->type); + + msg_data->id = cdata->index; + msg_data->num_elems = 0; /* ignored for bytes */ + + msg->data_ptr = msg_data; + msg->data_size = data_size; + + scontrol->comp_data_dirty = false; + ret = sof_ipc4_set_get_kcontrol_data(scontrol, false, lock); + if (!ret) { + if (msg->data_size > scontrol->max_size - sizeof(*data)) { + dev_err(scomp->dev, + "%s: no space for data in %s (%zu, %zu)\n", + __func__, scontrol->name, msg->data_size, + scontrol->max_size - sizeof(*data)); + goto out; + } + + data->size = msg->data_size; + scontrol->size = sizeof(*cdata) + sizeof(*data) + data->size; + memcpy(data->data, msg->data_ptr, data->size); + } else { + dev_err(scomp->dev, "Failed to read control data for %s\n", + scontrol->name); + scontrol->comp_data_dirty = true; + } + +out: + msg->data_ptr = NULL; + msg->data_size = 0; + + kfree(msg_data); + + return ret; +} + static bool sof_ipc4_switch_put(struct snd_sof_control *scontrol, struct snd_ctl_elem_value *ucontrol) { @@ -423,6 +522,13 @@ static int sof_ipc4_set_get_bytes_data(struct snd_sof_dev *sdev, } }

+ if (data->type == SOF_IPC4_BYTES_CONTROL_PARAM_ID) { + if (set) + return sof_ipc4_set_bytes_control_data(scontrol, lock); + else + return sof_ipc4_refresh_bytes_control(scontrol, lock); + } + msg->extension = SOF_IPC4_MOD_EXT_MSG_PARAM_ID(data->type);

msg->data_ptr = data->data; @@ -507,6 +613,8 @@ static int sof_ipc4_bytes_get(struct snd_sof_control *scontrol, return -EINVAL; }

+ sof_ipc4_refresh_bytes_control(scontrol, true); + size = data->size + sizeof(*data);

/* copy back to kcontrol */ @@ -661,6 +769,8 @@ static int sof_ipc4_bytes_ext_get(struct snd_sof_control *scontrol, const unsigned int __user *binary_data, unsigned int size) { + sof_ipc4_refresh_bytes_control(scontrol, true); + return _sof_ipc4_bytes_ext_get(scontrol, binary_data, size, false); }

@@ -714,6 +824,9 @@ static void sof_ipc4_control_update(struct snd_sof_dev *sdev, void *ipc_message) case SOF_IPC4_ENUM_CONTROL_PARAM_ID: type = SND_SOC_TPLG_TYPE_ENUM; break; + case SOF_IPC4_BYTES_CONTROL_PARAM_ID: + type = SND_SOC_TPLG_TYPE_BYTES; + break; default: dev_err(sdev->dev, "%s: Invalid control type for module %u.%u: %u\n", @@ -764,23 +877,38 @@ static void sof_ipc4_control_update(struct snd_sof_dev *sdev, void *ipc_message) * The message includes the updated value/data, update the * control's local cache using the received notification */ - for (i = 0; i < msg_data->num_elems; i++) { - u32 channel = msg_data->chanv[i].channel; + if (type == SND_SOC_TPLG_TYPE_BYTES) { + struct sof_abi_hdr *data = cdata->data;

- if (channel >= scontrol->num_channels) { + if (msg_data->num_elems > scontrol->max_size - sizeof(*data)) { dev_warn(sdev->dev, - "Invalid channel index for %s: %u\n", - scontrol->name, i); - - /* - * Mark the scontrol as dirty to force a refresh - * on next read - */ - scontrol->comp_data_dirty = true; - break; + "%s: no space for data in %s (%u, %zu)\n", + __func__, scontrol->name, msg_data->num_elems, + scontrol->max_size - sizeof(*data)); + } else { + memcpy(data->data, msg_data->data, msg_data->num_elems); + data->size = msg_data->num_elems; + scontrol->size = sizeof(*cdata) + sizeof(*data) + data->size; + } + } else { + for (i = 0; i < msg_data->num_elems; i++) { + u32 channel = msg_data->chanv[i].channel; + + if (channel >= scontrol->num_channels) { + dev_warn(sdev->dev, + "Invalid channel index for %s: %u\n", + scontrol->name, i); + + /* + * Mark the scontrol as dirty to force a refresh + * on next read + */ + scontrol->comp_data_dirty = true; + break; + } + + cdata->chanv[channel].value = msg_data->chanv[i].value; } - - cdata->chanv[channel].value = msg_data->chanv[i].value; } } else { /*