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;
> }
> }
>
--
Péter
