Skip to contentSkip to conversation

topology: Fix for handling widgets' references

PATCH Superseded [email protected] 3 participants 7 messages Jul 22, 2016 – Jul 27, 2016

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

v2 cover topology: Fix for handling widgets' references

From: Mengdong Lin <[email protected]>

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(-)
Signature · 1 line
2.5.0
author
#1 lore raw

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

From: Mengdong Lin <[email protected]>

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

Signed-off-by Mengdong Lin <[email protected]>
src/topology/dapm.c +1 -1
index 4d343b2..e3c90d8 100644
@@ -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;
Signature · 1 line
2.5.0
author
#2 lore raw

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

In reply to [email protected]

From: Mengdong Lin <[email protected]>

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 <[email protected]>
src/topology/data.c +2 -0
index 768fc27..e7793b2 100644
@@ -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;
 }
Signature · 1 line
2.5.0
#3 lore raw

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

Quoted 13 lines · On Jul 22 2016 10:47, [email protected] wrote:

From: Mengdong Lin <[email protected]>

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 <[email protected]>
src/topology/data.c +0 -0
index 768fc27..e7793b2 100644

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, · line 1084

Quoted 7 lines
src/topology/data.c +2 -0
@@ -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

#4 lore raw

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

Quoted 24 lines

-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Friday, July 22, 2016 10:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct

Quoted 13 lines · On Jul 22 2016 10:47, [email protected] wrote:

From: Mengdong Lin <[email protected]>

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 <[email protected]>
src/topology/data.c index renamed from src/topology/data.c +0 -0

768fc27..e7793b2 100644

src/topology/data.c +0 -0
@@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct

tplg_elem *elem,

Quoted 10 lines

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, · line 1084

Quoted 7 lines
src/topology/data.c +2 -0
  	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

Quoted 21 lines

/* 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

#5 lore raw

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

Quoted 37 lines · Mengdong
Quoted 24 lines

-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Friday, July 22, 2016 10:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr when merging private data

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct

Quoted 13 lines · On Jul 22 2016 10:47, [email protected] wrote:

From: Mengdong Lin <[email protected]>

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 <[email protected]>
src/topology/data.c index renamed from src/topology/data.c +0 -0

768fc27..e7793b2 100644

src/topology/data.c +0 -0
@@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct

tplg_elem *elem,

Quoted 10 lines

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, · line 1084

Quoted 7 lines
src/topology/data.c +2 -0
   	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.

Quoted 7 lines

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

author
#6 lore raw

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

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

Quoted 70 lines · Takashi Sakamoto
Quoted 38 lines · Mengdong
Quoted 33 lines

-----Original Message-----
From: Takashi Sakamoto [mailto:[email protected]]
Sent: Friday, July 22, 2016 10:16 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; Lin, Mengdong; Girdwood, Liam R; Nc, Shreyas
Subject: Re: [PATCH v2 2/2] topology: Fix the missing referenced elem ptr
when merging private data

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct

Quoted 13 lines · On Jul 22 2016 10:47, [email protected] wrote:

From: Mengdong Lin <[email protected]>

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 <[email protected]>
src/topology/data.c index renamed from src/topology/data.c +0 -0

768fc27..e7793b2 100644

src/topology/data.c +0 -0
@@ -1078,6 +1078,8 @@ int tplg_copy_data(snd_tplg_t *tplg, struct

tplg_elem *elem,

src/topology/data.c int tplg_copy_data(snd_tplg_t *tplg, struct tplg_elem *elem, · line 1084

Quoted 7 lines
src/topology/data.c +2 -0
       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;
   }
Quoted 2 lines

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.

Quoted 8 lines

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
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel