On 27/04/2022 14:54, Sergey Senozhatsky wrote:
On (22/04/27 14:33), Péter Ujfalusi wrote:
@@ -2100,6 +2101,7 @@ static int sof_get_control_data(struct snd_soc_component *scomp, size_t *size) { const struct snd_kcontrol_new *kc;
- struct sof_ipc_ctrl_data *cdata; struct soc_mixer_control *sm; struct soc_bytes_ext *sbe; struct soc_enum *se;
@@ -2136,16 +2138,28 @@ static int sof_get_control_data(struct snd_soc_component *scomp, return -EINVAL; }
wdata[i].pdata = wdata[i].control->control_data->data;if (!wdata[i].pdata)return -EINVAL;
cdata = wdata[i].control->control_data;if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) {if ((void *)cdata->data == NULL)Is there a need for casting it to void*?
clang appears to be unhappy otherwise.
error: comparison of array 'cdata->data' equal to a null pointer is always false
Changing this into `if (!cdata->data)` is a little bit better as now 'always false' becomes 'always true'
error: address of array 'cdata->data' will always evaluate to 'true'
Hrm, uhm. clang is right. The check is (and was) bogus...
cdata->data is a pointer (to cdata->data[0]) which is always: cdata + sizeof(struct sof_ipc_ctrl_data). Checking if it is NULL or not is irrelevant and wrong. If we do not have additional data then cdata->data points to memory which is outside of the struct and it can be random data (might be 0, might not be).
I think we can just drop this check as we would not be here if additional data was not allocated for the payload prior?
return -EINVAL;
/* make sure data is valid - data can be updated at runtime */if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES &&wdata[i].pdata->magic != SOF_ABI_MAGIC)return -EINVAL;
if (cdata->data->magic != SOF_ABI_MAGIC)return -EINVAL;wdata[i].pdata = cdata->data;you want to save the cdata->data->data, so w/o the abi header stuff.
Oh... good catch!
I used `wdata[i].control->control_data->data` for tests, converted this to cdata the very last moment.
So something like this then
sound/soc/sof/topology.c | 42 +++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index c1fc7bcf4eb5..60b4b0053e98 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -50,7 +50,8 @@ struct sof_widget_data { int ctrl_type; int ipc_cmd;
- struct sof_abi_hdr *pdata;
- void *pdata;
- size_t pdata_size; struct snd_sof_control *control;
};
@@ -2100,6 +2101,7 @@ static int sof_get_control_data(struct snd_soc_component *scomp, size_t *size) { const struct snd_kcontrol_new *kc;
- struct sof_ipc_ctrl_data *cdata; struct soc_mixer_control *sm; struct soc_bytes_ext *sbe; struct soc_enum *se;
@@ -2136,16 +2138,28 @@ static int sof_get_control_data(struct snd_soc_component *scomp, return -EINVAL; }
wdata[i].pdata = wdata[i].control->control_data->data;if (!wdata[i].pdata)return -EINVAL;
cdata = wdata[i].control->control_data;if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) {if ((void *)cdata->data == NULL)return -EINVAL;
/* make sure data is valid - data can be updated at runtime */if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES &&wdata[i].pdata->magic != SOF_ABI_MAGIC)return -EINVAL;
if (cdata->data->magic != SOF_ABI_MAGIC)return -EINVAL;wdata[i].pdata = cdata->data->data;wdata[i].pdata_size = cdata->data->size;} else {/* points to the control data union */wdata[i].pdata = cdata->chanv;/** wdata[i].control->size is calculated with struct_size* and includes the size of struct sof_ipc_ctrl_data*/wdata[i].pdata_size = wdata[i].control->size -sizeof(struct sof_ipc_ctrl_data);}
*size += wdata[i].pdata->size;
*size += wdata[i].pdata_size;/* get data type */ switch (wdata[i].control->cmd) {
@@ -2236,10 +2250,12 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, */ if (ipc_data_size) { for (i = 0; i < widget->num_kcontrols; i++) {
memcpy(&process->data + offset,wdata[i].pdata->data,wdata[i].pdata->size);offset += wdata[i].pdata->size;
if (!wdata[i].pdata_size)continue;memcpy(&process->data[offset], wdata[i].pdata,wdata[i].pdata_size);offset += wdata[i].pdata_size;