Re: [PATCH] ASoC: SOF: ipc3-topology: Correct get_control_data for non bytes payload

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (22/04/27 11:50), Peter Ujfalusi wrote:
> @@ -784,16 +785,23 @@ static int sof_get_control_data(struct snd_soc_component *scomp,
>  		}
>  
>  		cdata = wdata[i].control->ipc_control_data;
> -		wdata[i].pdata = cdata->data;
> -		if (!wdata[i].pdata)
> -			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 (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) {
> +			if (!cdata->data)
> +				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 {
> +			wdata[i].pdata = cdata->chanv; /* points to the control data union */
> +			wdata[i].pdata_size = wdata[i].control->size;
				^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
A question, so here wdata[i].control->size is

	scontrol->size = struct_size(scontrol->control_data, chanv,
					le32_to_cpu(mc->num_channels));

Right?

If so, then is this really what we memcpy()? We memcpy() control->data->chanv
and its size is `sizeof(chanv) * le32_to_cpu(mc->num_channels)`, isn't it?

[..]
>  	if (ipc_data_size) {
>  		for (i = 0; i < widget->num_kcontrols; i++) {
> +			if (!wdata[i].pdata_size)
> +				continue;
> +
> +			memcpy(&process->data[offset], wdata[i].pdata,
> +			       wdata[i].pdata_size);
> +			offset += wdata[i].pdata_size;
>  		}
>  	}

So should sof_get_control_data() have instead of this

	wdata[i].pdata_size = wdata[i].control->size;

something like this

	wdata[i].pdata_size = wdata[i].control->size - sizeof(struct sof_ipc_ctrl_data);

So that we will copy payload data, which is a bunch of chanv structs 8
bytes each.