[PATCH v2 0/2] topology: Fix for handling widgets' references

[PATCH v2 0/2] topology: Fix for handling widgets' references

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

This series fixes an error when merging a widget's private data and an 
inaccurate output message.

History:
v2: Split v1 into 2 patches. And set the valid reference element value
    only on success.

Mengdong Lin (2):
  topology: Fix inaccurate message on failure to find a widgets's
    reference
  topology: Fix the missing referenced elem ptr when merging private
    data

 src/topology/dapm.c | 2 +-
 src/topology/data.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.5.0

[PATCH v2 1/2] topology: Fix inaccurate message on failure to find a widgets's reference

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

A widget may have references to control or data elements. So the message
should not only use "control" here.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/dapm.c b/src/topology/dapm.c
index 4d343b2..e3c90d8 100644
--- a/src/topology/dapm.c
+++ b/src/topology/dapm.c
@@ -201,7 +201,7 @@ static int tplg_build_widget(snd_tplg_t *tplg,
 		}
 
 		if (!ref->elem) {
-			SNDERR("error: cannot find control '%s'"
+			SNDERR("error: cannot find '%s'"
 				" referenced by widget '%s'\n",
 				ref->id, elem->id);
 			return -EINVAL;
-- 
2.5.0

[PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@linux.intel.com>

tplg_copy_data() should set the valid referenced data element pointer
on success. The caller will double check this pointer for all kinds of
references, including controls and data.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/src/topology/data.c b/src/topology/data.c
index 768fc27..e7793b2 100644
--- a/src/topology/data.c
+++ b/src/topology/data.c
@@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem,
 	ref_elem->compound_elem = 1;
 	memcpy(priv->data + old_priv_data_size,
 	       ref_elem->data->data, priv_data_size);
+
+	ref->elem = ref_elem;
 	return 0;
 }
 
-- 
2.5.0

Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

[Takashi Sakamoto]

On Jul 22 2016 10:47, mengdong.lin@linux.intel.com wrote:
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>
> tplg_copy_data() should set the valid referenced data element pointer
> on success. The caller will double check this pointer for all kinds of
> references, including controls and data.
>
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>
> diff --git a/src/topology/data.c b/src/topology/data.c
> index 768fc27..e7793b2 100644
> --- a/src/topology/data.c
> +++ b/src/topology/data.c
> @@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem,
>   	ref_elem->compound_elem = 1;
>   	memcpy(priv->data + old_priv_data_size,
>   	       ref_elem->data->data, priv_data_size);
> +
> +	ref->elem = ref_elem;
>   	return 0;
>   }

Is this really OK when the found topology element has no private data? 
In this case, ref->elem has no assignment at return.

ref_elem = tplg_elem_lookup(&tplg->pdata_list,
                              ref->id, SND_TPLG_TYPE_DATA);
...
/* overlook empty private data */
if (!ref_elem->data || !ref_elem->data->size)
         return 0;
...
ref->elem = ref_elem;
return 0;


Regards

Takashi Sakamoto

Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

[Lin, Mengdong]

> -----Original Message-----
> From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp]
> Sent: Friday, July 22, 2016 10:16 AM
> To: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org;
> broonie@kernel.org
> Cc: tiwai@suse.de; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
> Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr
> when merging private data
> 
> On Jul 22 2016 10:47, mengdong.lin@linux.intel.com wrote:
> > From: Mengdong Lin <mengdong.lin@linux.intel.com>
> >
> > tplg_copy_data() should set the valid referenced data element pointer
> > on success. The caller will double check this pointer for all kinds of
> > references, including controls and data.
> >
> > Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> >
> > diff --git a/src/topology/data.c b/src/topology/data.c index
> > 768fc27..e7793b2 100644
> > --- a/src/topology/data.c
> > +++ b/src/topology/data.c
> > @@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct
> tplg_elem *elem,
> >   	ref_elem->compound_elem = 1;
> >   	memcpy(priv->data + old_priv_data_size,
> >   	       ref_elem->data->data, priv_data_size);
> > +
> > +	ref->elem = ref_elem;
> >   	return 0;
> >   }
> 
> Is this really OK when the found topology element has no private data?
> In this case, ref->elem has no assignment at return.

No. This is my mistake. Thanks for finding this bug.

ref->elem should still be assigned for this case. I will make it like this:

/* overlook empty private data */
if (!ref_elem->data || !ref_elem->data->size) {
		ref->elem = ref_elem;
         return 0;
}

Thanks
Mengdong


> /* overlook empty private data */
> if (!ref_elem->data || !ref_elem->data->size)
>          return 0;

> 
> ref_elem = tplg_elem_lookup(&tplg->pdata_list,
>                               ref->id, SND_TPLG_TYPE_DATA); ...
> /* overlook empty private data */
> if (!ref_elem->data || !ref_elem->data->size)
>          return 0;
> ...
> ref->elem = ref_elem;
> return 0;
> 



> 
> Regards
> 
> Takashi Sakamoto

Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

[Takashi Sakamoto]

On Jul 22 2016 11:39, Lin, Mengdong wrote:
>> -----Original Message-----
>> From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp]
>> Sent: Friday, July 22, 2016 10:16 AM
>> To: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org;
>> broonie@kernel.org
>> Cc: tiwai@suse.de; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
>> Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr
>> when merging private data
>>
>> On Jul 22 2016 10:47, mengdong.lin@linux.intel.com wrote:
>>> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>
>>> tplg_copy_data() should set the valid referenced data element pointer
>>> on success. The caller will double check this pointer for all kinds of
>>> references, including controls and data.
>>>
>>> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>
>>> diff --git a/src/topology/data.c b/src/topology/data.c index
>>> 768fc27..e7793b2 100644
>>> --- a/src/topology/data.c
>>> +++ b/src/topology/data.c
>>> @@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct
>> tplg_elem *elem,
>>>    	ref_elem->compound_elem = 1;
>>>    	memcpy(priv->data + old_priv_data_size,
>>>    	       ref_elem->data->data, priv_data_size);
>>> +
>>> +	ref->elem = ref_elem;
>>>    	return 0;
>>>    }
>>
>> Is this really OK when the found topology element has no private data?
>> In this case, ref->elem has no assignment at return.
>
> No. This is my mistake. Thanks for finding this bug.
>
> ref->elem should still be assigned for this case. I will make it like this:
>
> /* overlook empty private data */
> if (!ref_elem->data || !ref_elem->data->size) {
> 		ref->elem = ref_elem;
>           return 0;
> }

Let us split data merging code to an explicit function? Then:

if (ref_elem->data != NULL && ref_elem->data->size > 0) {
     err = merge_private_data(elem, ref_elem);
     if (err < 0)
         return err;
}
ref->elem = ref_elem;
return 0;

(Here, I expect compiler optimization to merge the new function to 
caller implicitly.)


Regards

Takashi Sakamoto

Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

[Mengdong Lin]

Hi,

I submitted a new v4 patch to solve this issue, which will return 
-EINVAL right after a failure to find an element's reference. I hope 
will make the code more clear to read. So there is no need to modify 
tplg_copy_data() now, checking its return value is enough.

The new patch name is [PATCH v4] topology: Return -EINVAL at once on 
failure to find a reference. Please review.

Thanks
Mengdong

On 07/22/2016 11:28 AM, Takashi Sakamoto wrote:
> On Jul 22 2016 11:39, Lin, Mengdong wrote:
>>> -----Original Message-----
>>> From: Takashi Sakamoto [mailto:o-takashi@sakamocchi.jp]
>>> Sent: Friday, July 22, 2016 10:16 AM
>>> To: mengdong.lin@linux.intel.com; alsa-devel@alsa-project.org;
>>> broonie@kernel.org
>>> Cc: tiwai@suse.de; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
>>> Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem
>>> ptr
>>> when merging private data
>>>
>>> On Jul 22 2016 10:47, mengdong.lin@linux.intel.com wrote:
>>>> From: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>>
>>>> tplg_copy_data() should set the valid referenced data element pointer
>>>> on success. The caller will double check this pointer for all kinds of
>>>> references, including controls and data.
>>>>
>>>> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
>>>>
>>>> diff --git a/src/topology/data.c b/src/topology/data.c index
>>>> 768fc27..e7793b2 100644
>>>> --- a/src/topology/data.c
>>>> +++ b/src/topology/data.c
>>>> @@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct
>>> tplg_elem *elem,
>>>>        ref_elem->compound_elem = 1;
>>>>        memcpy(priv->data + old_priv_data_size,
>>>>               ref_elem->data->data, priv_data_size);
>>>> +
>>>> +    ref->elem = ref_elem;
>>>>        return 0;
>>>>    }
>>>
>>> Is this really OK when the found topology element has no private data?
>>> In this case, ref->elem has no assignment at return.
>>
>> No. This is my mistake. Thanks for finding this bug.
>>
>> ref->elem should still be assigned for this case. I will make it like
>> this:
>>
>> /* overlook empty private data */
>> if (!ref_elem->data || !ref_elem->data->size) {
>>         ref->elem = ref_elem;
>>           return 0;
>> }
>
> Let us split data merging code to an explicit function? Then:
>
> if (ref_elem->data != NULL && ref_elem->data->size > 0) {
>      err = merge_private_data(elem, ref_elem);
>      if (err < 0)
>          return err;
> }
> ref->elem = ref_elem;
> return 0;
>
> (Here, I expect compiler optimization to merge the new function to
> caller implicitly.)
>
>
> Regards
>
> Takashi Sakamoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>