[alsa-devel] [PATCH v2 0/2] topology: Fix for handling widgets' references
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(-)
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;
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; }
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
-----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
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
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
participants (4)
-
Lin, Mengdong -
Mengdong Lin -
mengdong.lin＠linux.intel.com
-
Takashi Sakamoto