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

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

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