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

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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'

> > +				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;
 		}
 	}
 
-- 
2.31.0