All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass
@ 2020-05-12 18:23 Keyon Jie
  2020-05-12 18:23 ` [PATCH 2/2] ASoC: topology: remove the redundant pass checks Keyon Jie
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Keyon Jie @ 2020-05-12 18:23 UTC (permalink / raw)
  To: alsa-devel; +Cc: tiwai, broonie, Keyon Jie, pierre-louis.bossart

The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
comparing two different enums, refine the element loading functions, and
log the information when the header is being parsed in the corresponding
parsing pass.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
---
 sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 49875978a1ce..b3dae319c108 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	return ret;
 }
 
+int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
+	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
+	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
+	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,
+	[SOC_TPLG_PASS_WIDGET]		= soc_tplg_dapm_widget_elems_load,
+	[SOC_TPLG_PASS_PCM_DAI]		= soc_tplg_pcm_elems_load,
+	[SOC_TPLG_PASS_GRAPH]		= soc_tplg_dapm_graph_elems_load,
+	[SOC_TPLG_PASS_BE_DAI]		= soc_tplg_dai_elems_load,
+	[SOC_TPLG_PASS_LINK]		= soc_tplg_link_elems_load,
+};
+
 /* validate header magic, size and type */
 static int soc_valid_header(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
@@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg *tplg,
 		return -EINVAL;
 	}
 
-	if (tplg->pass == le32_to_cpu(hdr->type))
+	return 1;
+}
+
+/* check and load the element for the current pass */
+static int soc_pass_load(struct soc_tplg *tplg,
+			 struct snd_soc_tplg_hdr *hdr,
+			 unsigned int hdr_pass)
+{
+	if (tplg->pass == hdr_pass) {
 		dev_dbg(tplg->dev,
 			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
 			hdr->payload_size, hdr->type, hdr->version,
 			hdr->vendor_type, tplg->pass);
+		return elem_load[hdr_pass](tplg, hdr);
+	}
 
-	return 1;
+	return 0;
 }
 
 /* check header type and call appropriate handler */
 static int soc_tplg_load_header(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
+	unsigned int hdr_pass;
+
 	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
 
 	/* check for matching ID */
@@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct soc_tplg *tplg,
 	case SND_SOC_TPLG_TYPE_MIXER:
 	case SND_SOC_TPLG_TYPE_ENUM:
 	case SND_SOC_TPLG_TYPE_BYTES:
-		return soc_tplg_kcontrol_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_MIXER;
+		break;
 	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
-		return soc_tplg_dapm_graph_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_GRAPH;
+		break;
 	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
-		return soc_tplg_dapm_widget_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_WIDGET;
+		break;
 	case SND_SOC_TPLG_TYPE_PCM:
-		return soc_tplg_pcm_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
+		break;
 	case SND_SOC_TPLG_TYPE_DAI:
-		return soc_tplg_dai_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_BE_DAI;
+		break;
 	case SND_SOC_TPLG_TYPE_DAI_LINK:
 	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
 		/* physical link configurations */
-		return soc_tplg_link_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_LINK;
+		break;
 	case SND_SOC_TPLG_TYPE_MANIFEST:
-		return soc_tplg_manifest_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_MANIFEST;
+		break;
 	default:
 		/* bespoke vendor data object */
-		return soc_tplg_vendor_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_VENDOR;
+		break;
 	}
 
-	return 0;
+	return soc_pass_load(tplg, hdr, hdr_pass);
 }
 
 /* process the topology file headers */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] ASoC: topology: remove the redundant pass checks
  2020-05-12 18:23 [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass Keyon Jie
@ 2020-05-12 18:23 ` Keyon Jie
  2020-05-12 19:40   ` Pierre-Louis Bossart
  2020-05-13  2:43   ` kbuild test robot
  2020-05-13  2:43   ` kbuild test robot
  2 siblings, 1 reply; 9+ messages in thread
From: Keyon Jie @ 2020-05-12 18:23 UTC (permalink / raw)
  To: alsa-devel; +Cc: tiwai, broonie, Keyon Jie, pierre-louis.bossart

As we have check the 'pass' in the soc_elem_pass_load(), so no need to
check it again in each specific elem_load function, at the same time,
the tplg->pos will be reset to the next header base when the pass is
mismatched, so the increasing of the tplg->pos in these cases made no
sense. Here remove all of them.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
---
 sound/soc/soc-topology.c | 46 ++--------------------------------------
 1 file changed, 2 insertions(+), 44 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index b3dae319c108..682ffaa85e9e 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg *tplg,
 }
 
 /* pass vendor data to component driver for processing */
-static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
-	struct snd_soc_tplg_hdr *hdr)
+static int soc_tplg_vendor_load(struct soc_tplg *tplg,
+				struct snd_soc_tplg_hdr *hdr)
 {
 	int ret = 0;
 
@@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
 	return ret;
 }
 
-/* pass vendor data to component driver for processing */
-static int soc_tplg_vendor_load(struct soc_tplg *tplg,
-	struct snd_soc_tplg_hdr *hdr)
-{
-	if (tplg->pass != SOC_TPLG_PASS_VENDOR)
-		return 0;
-
-	return soc_tplg_vendor_load_(tplg, hdr);
-}
-
 /* optionally pass new dynamic widget to component driver. This is mainly for
  * external widgets where we can assign private data/ops */
 static int soc_tplg_widget_load(struct soc_tplg *tplg,
@@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
 	int ret;
 	int i;
 
-	if (tplg->pass != SOC_TPLG_PASS_MIXER) {
-		tplg->pos += le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-		return 0;
-	}
-
 	dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", hdr->count,
 		soc_tplg_get_offset(tplg));
 
@@ -1204,14 +1188,6 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
-		tplg->pos +=
-			le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-
-		return 0;
-	}
-
 	if (soc_tplg_check_elem_count(tplg,
 		sizeof(struct snd_soc_tplg_dapm_graph_elem),
 		count, le32_to_cpu(hdr->payload_size), "graph")) {
@@ -1741,9 +1717,6 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_WIDGET)
-		return 0;
-
 	dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
 
 	for (i = 0; i < count; i++) {
@@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
-		return 0;
-
 	/* check the element size and count */
 	pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
 	size = le32_to_cpu(pcm->size);
@@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_LINK) {
-		tplg->pos += le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-		return 0;
-	}
-
 	/* check the element size and count */
 	link = (struct snd_soc_tplg_link_config *)tplg->pos;
 	size = le32_to_cpu(link->size);
@@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
-		return 0;
-
 	/* config the existing BE DAIs */
 	for (i = 0; i < count; i++) {
 		dai = (struct snd_soc_tplg_dai *)tplg->pos;
@@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	bool abi_match;
 	int ret = 0;
 
-	if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
-		return 0;
-
 	manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
 
 	/* check ABI version by size, create a new manifest if abi not match */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: topology: remove the redundant pass checks
  2020-05-12 18:23 ` [PATCH 2/2] ASoC: topology: remove the redundant pass checks Keyon Jie
@ 2020-05-12 19:40   ` Pierre-Louis Bossart
  2020-05-13  2:36     ` Keyon Jie
  0 siblings, 1 reply; 9+ messages in thread
From: Pierre-Louis Bossart @ 2020-05-12 19:40 UTC (permalink / raw)
  To: Keyon Jie, alsa-devel; +Cc: tiwai, broonie



On 5/12/20 1:23 PM, Keyon Jie wrote:
> As we have check the 'pass' in the soc_elem_pass_load(), so no need to
> check it again in each specific elem_load function, at the same time,
> the tplg->pos will be reset to the next header base when the pass is
> mismatched, so the increasing of the tplg->pos in these cases made no
> sense. Here remove all of them.

Sorry Keyon, I am not comfortable with the changes. This started as a 
removal of a debug log and we ended-up changing the execution flow in 
one of the more complex parts of the ASoC framework.
I spent 30mn looking at the code and I don't have a feeling all corner 
cases are covered. The bar for core changes is higher, it'd help to have 
additional explanations or evidence that your proposal was tested 
extensively. You may be right but we should be careful with topology 
changes.

> 
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
> ---
>   sound/soc/soc-topology.c | 46 ++--------------------------------------
>   1 file changed, 2 insertions(+), 44 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index b3dae319c108..682ffaa85e9e 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg *tplg,
>   }
>   
>   /* pass vendor data to component driver for processing */
> -static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
> -	struct snd_soc_tplg_hdr *hdr)
> +static int soc_tplg_vendor_load(struct soc_tplg *tplg,
> +				struct snd_soc_tplg_hdr *hdr)
>   {
>   	int ret = 0;
>   
> @@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
>   	return ret;
>   }
>   
> -/* pass vendor data to component driver for processing */
> -static int soc_tplg_vendor_load(struct soc_tplg *tplg,
> -	struct snd_soc_tplg_hdr *hdr)
> -{
> -	if (tplg->pass != SOC_TPLG_PASS_VENDOR)
> -		return 0;
> -
> -	return soc_tplg_vendor_load_(tplg, hdr);
> -}
> -
>   /* optionally pass new dynamic widget to component driver. This is mainly for
>    * external widgets where we can assign private data/ops */
>   static int soc_tplg_widget_load(struct soc_tplg *tplg,
> @@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
>   	int ret;
>   	int i;
>   
> -	if (tplg->pass != SOC_TPLG_PASS_MIXER) {
> -		tplg->pos += le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -		return 0;
> -	}
> -
>   	dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", hdr->count,
>   		soc_tplg_get_offset(tplg));
>   
> @@ -1204,14 +1188,6 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
> -		tplg->pos +=
> -			le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -
> -		return 0;
> -	}
> -
>   	if (soc_tplg_check_elem_count(tplg,
>   		sizeof(struct snd_soc_tplg_dapm_graph_elem),
>   		count, le32_to_cpu(hdr->payload_size), "graph")) {
> @@ -1741,9 +1717,6 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_WIDGET)
> -		return 0;
> -
>   	dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
>   
>   	for (i = 0; i < count; i++) {
> @@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
> -		return 0;
> -
>   	/* check the element size and count */
>   	pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
>   	size = le32_to_cpu(pcm->size);
> @@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_LINK) {
> -		tplg->pos += le32_to_cpu(hdr->size) +
> -			le32_to_cpu(hdr->payload_size);
> -		return 0;
> -	}
> -
>   	/* check the element size and count */
>   	link = (struct snd_soc_tplg_link_config *)tplg->pos;
>   	size = le32_to_cpu(link->size);
> @@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg,
>   
>   	count = le32_to_cpu(hdr->count);
>   
> -	if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
> -		return 0;
> -
>   	/* config the existing BE DAIs */
>   	for (i = 0; i < count; i++) {
>   		dai = (struct snd_soc_tplg_dai *)tplg->pos;
> @@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
>   	bool abi_match;
>   	int ret = 0;
>   
> -	if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
> -		return 0;
> -
>   	manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
>   
>   	/* check ABI version by size, create a new manifest if abi not match */
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: topology: remove the redundant pass checks
  2020-05-12 19:40   ` Pierre-Louis Bossart
@ 2020-05-13  2:36     ` Keyon Jie
  2020-05-18  9:40       ` Keyon Jie
  0 siblings, 1 reply; 9+ messages in thread
From: Keyon Jie @ 2020-05-13  2:36 UTC (permalink / raw)
  To: Pierre-Louis Bossart, alsa-devel; +Cc: tiwai, broonie



On 5/13/20 3:40 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 5/12/20 1:23 PM, Keyon Jie wrote:
>> As we have check the 'pass' in the soc_elem_pass_load(), so no need to
>> check it again in each specific elem_load function, at the same time,
>> the tplg->pos will be reset to the next header base when the pass is
>> mismatched, so the increasing of the tplg->pos in these cases made no
>> sense. Here remove all of them.
> 
> Sorry Keyon, I am not comfortable with the changes. This started as a 
> removal of a debug log and we ended-up changing the execution flow in 
> one of the more complex parts of the ASoC framework.
> I spent 30mn looking at the code and I don't have a feeling all corner 
> cases are covered. The bar for core changes is higher, it'd help to have 
> additional explanations or evidence that your proposal was tested 
> extensively. You may be right but we should be careful with topology 
> changes.

Maybe I should send these 2 patches separately, this 2nd one is actually 
fix for a long existed issue and it is exposed to me when writing the 
1st commit:
1. the check of "if (tplg->pass != SOC_TPLG_PASS_xxx" was good and it 
acted as the correct filter of the pass, but
2. the increment "tplg->pos += le32_to_cpu(hdr->size) + 
le32_to_cpu(hdr->payload_size);" was actually wrong when it the pass is 
unmatched in #1, it should be "tplg->pos += 
le32_to_cpu(hdr->payload_size);" as the "hdr->size" was already 
calculated in the upper layer in soc_tplg_load_header():
"tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);".
3. the exited mistake described in #2 didn't triggered any real problems 
as the "tplg->pos" will be reset to new value in soc_tplg_load_header() 
when the next header comes, so increasing the "tplg->pos" here at the 
end of the current header check actually make no sense, we can just 
remove them.

The code changes here might make you nervous but the logic here is not 
that complicated, if you rule out the issue I mentioned above in #2 and 
#3, the remained changes here is only removing the checks "if 
(tplg->pass != SOC_TPLG_PASS_xxx" as they are already covered by the 1st 
commit.

I have tested these changes with 3 typical SOF topology files (nocodec, 
tgl i2s + dsm, hda-generic) and it works all fine there and the correct 
header information are logged with the changes, agree we should test 
more extensively and widely, so tests on SST driver are very welcomed.

So in short, I did these changes together since I am already here, but 
if you and Mark preferred, I can split out the fix for the "tplg->pos" 
increment and send it ahead.

Thanks,
~Keyon

> 
>>
>> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
>> ---
>>   sound/soc/soc-topology.c | 46 ++--------------------------------------
>>   1 file changed, 2 insertions(+), 44 deletions(-)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index b3dae319c108..682ffaa85e9e 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg 
>> *tplg,
>>   }
>>   /* pass vendor data to component driver for processing */
>> -static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
>> -    struct snd_soc_tplg_hdr *hdr)
>> +static int soc_tplg_vendor_load(struct soc_tplg *tplg,
>> +                struct snd_soc_tplg_hdr *hdr)
>>   {
>>       int ret = 0;
>> @@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg 
>> *tplg,
>>       return ret;
>>   }
>> -/* pass vendor data to component driver for processing */
>> -static int soc_tplg_vendor_load(struct soc_tplg *tplg,
>> -    struct snd_soc_tplg_hdr *hdr)
>> -{
>> -    if (tplg->pass != SOC_TPLG_PASS_VENDOR)
>> -        return 0;
>> -
>> -    return soc_tplg_vendor_load_(tplg, hdr);
>> -}
>> -
>>   /* optionally pass new dynamic widget to component driver. This is 
>> mainly for
>>    * external widgets where we can assign private data/ops */
>>   static int soc_tplg_widget_load(struct soc_tplg *tplg,
>> @@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct 
>> soc_tplg *tplg,
>>       int ret;
>>       int i;
>> -    if (tplg->pass != SOC_TPLG_PASS_MIXER) {
>> -        tplg->pos += le32_to_cpu(hdr->size) +
>> -            le32_to_cpu(hdr->payload_size);
>> -        return 0;
>> -    }
>> -
>>       dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", 
>> hdr->count,
>>           soc_tplg_get_offset(tplg));
>> @@ -1204,14 +1188,6 @@ static int 
>> soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>       count = le32_to_cpu(hdr->count);
>> -    if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
>> -        tplg->pos +=
>> -            le32_to_cpu(hdr->size) +
>> -            le32_to_cpu(hdr->payload_size);
>> -
>> -        return 0;
>> -    }
>> -
>>       if (soc_tplg_check_elem_count(tplg,
>>           sizeof(struct snd_soc_tplg_dapm_graph_elem),
>>           count, le32_to_cpu(hdr->payload_size), "graph")) {
>> @@ -1741,9 +1717,6 @@ static int 
>> soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
>>       count = le32_to_cpu(hdr->count);
>> -    if (tplg->pass != SOC_TPLG_PASS_WIDGET)
>> -        return 0;
>> -
>>       dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
>>       for (i = 0; i < count; i++) {
>> @@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct 
>> soc_tplg *tplg,
>>       count = le32_to_cpu(hdr->count);
>> -    if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
>> -        return 0;
>> -
>>       /* check the element size and count */
>>       pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
>>       size = le32_to_cpu(pcm->size);
>> @@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct 
>> soc_tplg *tplg,
>>       count = le32_to_cpu(hdr->count);
>> -    if (tplg->pass != SOC_TPLG_PASS_LINK) {
>> -        tplg->pos += le32_to_cpu(hdr->size) +
>> -            le32_to_cpu(hdr->payload_size);
>> -        return 0;
>> -    }
>> -
>>       /* check the element size and count */
>>       link = (struct snd_soc_tplg_link_config *)tplg->pos;
>>       size = le32_to_cpu(link->size);
>> @@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct 
>> soc_tplg *tplg,
>>       count = le32_to_cpu(hdr->count);
>> -    if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
>> -        return 0;
>> -
>>       /* config the existing BE DAIs */
>>       for (i = 0; i < count; i++) {
>>           dai = (struct snd_soc_tplg_dai *)tplg->pos;
>> @@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct 
>> soc_tplg *tplg,
>>       bool abi_match;
>>       int ret = 0;
>> -    if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
>> -        return 0;
>> -
>>       manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
>>       /* check ABI version by size, create a new manifest if abi not 
>> match */
>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass
  2020-05-12 18:23 [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass Keyon Jie
@ 2020-05-13  2:43   ` kbuild test robot
  2020-05-13  2:43   ` kbuild test robot
  2020-05-13  2:43   ` kbuild test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-05-13  2:43 UTC (permalink / raw)
  To: Keyon Jie, alsa-devel
  Cc: tiwai, Keyon Jie, broonie, kbuild-all, pierre-louis.bossart

Hi Keyon,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Keyon-Jie/ASoC-topology-refine-and-log-the-header-in-the-correct-pass/20200513-021507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> sound/soc/soc-topology.c:2639:5: sparse: sparse: symbol 'elem_load' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass
@ 2020-05-13  2:43   ` kbuild test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-05-13  2:43 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Hi Keyon,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on v5.7-rc5 next-20200512]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Keyon-Jie/ASoC-topology-refine-and-log-the-header-in-the-correct-pass/20200513-021507
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-191-gc51a0382-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> sound/soc/soc-topology.c:2639:5: sparse: sparse: symbol 'elem_load' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH] ASoC: topology: elem_load[] can be static
  2020-05-12 18:23 [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass Keyon Jie
@ 2020-05-13  2:43   ` kbuild test robot
  2020-05-13  2:43   ` kbuild test robot
  2020-05-13  2:43   ` kbuild test robot
  2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-05-13  2:43 UTC (permalink / raw)
  To: Keyon Jie, alsa-devel
  Cc: tiwai, Keyon Jie, broonie, kbuild-all, pierre-louis.bossart


Signed-off-by: kbuild test robot <lkp@intel.com>
---
 soc-topology.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index b3dae319c1086..70c0ff7ce13ff 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -2636,7 +2636,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	return ret;
 }
 
-int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
+static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
 	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
 	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
 	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [RFC PATCH] ASoC: topology: elem_load[] can be static
@ 2020-05-13  2:43   ` kbuild test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2020-05-13  2:43 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]


Signed-off-by: kbuild test robot <lkp@intel.com>
---
 soc-topology.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index b3dae319c1086..70c0ff7ce13ff 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -2636,7 +2636,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	return ret;
 }
 
-int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
+static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
 	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
 	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
 	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: topology: remove the redundant pass checks
  2020-05-13  2:36     ` Keyon Jie
@ 2020-05-18  9:40       ` Keyon Jie
  0 siblings, 0 replies; 9+ messages in thread
From: Keyon Jie @ 2020-05-18  9:40 UTC (permalink / raw)
  To: Pierre-Louis Bossart, alsa-devel; +Cc: tiwai, Cezary Rojewski, broonie



On 5/13/20 10:36 AM, Keyon Jie wrote:
> 
> 
> On 5/13/20 3:40 AM, Pierre-Louis Bossart wrote:
>>
>>
>> On 5/12/20 1:23 PM, Keyon Jie wrote:
>>> As we have check the 'pass' in the soc_elem_pass_load(), so no need to
>>> check it again in each specific elem_load function, at the same time,
>>> the tplg->pos will be reset to the next header base when the pass is
>>> mismatched, so the increasing of the tplg->pos in these cases made no
>>> sense. Here remove all of them.
>>
>> Sorry Keyon, I am not comfortable with the changes. This started as a 
>> removal of a debug log and we ended-up changing the execution flow in 
>> one of the more complex parts of the ASoC framework.
>> I spent 30mn looking at the code and I don't have a feeling all corner 
>> cases are covered. The bar for core changes is higher, it'd help to 
>> have additional explanations or evidence that your proposal was tested 
>> extensively. You may be right but we should be careful with topology 
>> changes.
> 
> Maybe I should send these 2 patches separately, this 2nd one is actually 
> fix for a long existed issue and it is exposed to me when writing the 
> 1st commit:
> 1. the check of "if (tplg->pass != SOC_TPLG_PASS_xxx" was good and it 
> acted as the correct filter of the pass, but
> 2. the increment "tplg->pos += le32_to_cpu(hdr->size) + 
> le32_to_cpu(hdr->payload_size);" was actually wrong when it the pass is 
> unmatched in #1, it should be "tplg->pos += 
> le32_to_cpu(hdr->payload_size);" as the "hdr->size" was already 
> calculated in the upper layer in soc_tplg_load_header():
> "tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);".
> 3. the exited mistake described in #2 didn't triggered any real problems 
> as the "tplg->pos" will be reset to new value in soc_tplg_load_header() 
> when the next header comes, so increasing the "tplg->pos" here at the 
> end of the current header check actually make no sense, we can just 
> remove them.
> 
> The code changes here might make you nervous but the logic here is not 
> that complicated, if you rule out the issue I mentioned above in #2 and 
> #3, the remained changes here is only removing the checks "if 
> (tplg->pass != SOC_TPLG_PASS_xxx" as they are already covered by the 1st 
> commit.
> 
> I have tested these changes with 3 typical SOF topology files (nocodec, 
> tgl i2s + dsm, hda-generic) and it works all fine there and the correct 
> header information are logged with the changes, agree we should test 
> more extensively and widely, so tests on SST driver are very welcomed.

+ Cezary from the SST driver side.

Hi Cezary, can you help to check this from the SST driver side?

Thanks,
~Keyon

> 
> So in short, I did these changes together since I am already here, but 
> if you and Mark preferred, I can split out the fix for the "tplg->pos" 
> increment and send it ahead.
> 
> Thanks,
> ~Keyon
> 
>>
>>>
>>> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
>>> ---
>>>   sound/soc/soc-topology.c | 46 ++--------------------------------------
>>>   1 file changed, 2 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>>> index b3dae319c108..682ffaa85e9e 100644
>>> --- a/sound/soc/soc-topology.c
>>> +++ b/sound/soc/soc-topology.c
>>> @@ -246,8 +246,8 @@ static inline void soc_control_err(struct 
>>> soc_tplg *tplg,
>>>   }
>>>   /* pass vendor data to component driver for processing */
>>> -static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
>>> -    struct snd_soc_tplg_hdr *hdr)
>>> +static int soc_tplg_vendor_load(struct soc_tplg *tplg,
>>> +                struct snd_soc_tplg_hdr *hdr)
>>>   {
>>>       int ret = 0;
>>> @@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg 
>>> *tplg,
>>>       return ret;
>>>   }
>>> -/* pass vendor data to component driver for processing */
>>> -static int soc_tplg_vendor_load(struct soc_tplg *tplg,
>>> -    struct snd_soc_tplg_hdr *hdr)
>>> -{
>>> -    if (tplg->pass != SOC_TPLG_PASS_VENDOR)
>>> -        return 0;
>>> -
>>> -    return soc_tplg_vendor_load_(tplg, hdr);
>>> -}
>>> -
>>>   /* optionally pass new dynamic widget to component driver. This is 
>>> mainly for
>>>    * external widgets where we can assign private data/ops */
>>>   static int soc_tplg_widget_load(struct soc_tplg *tplg,
>>> @@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct 
>>> soc_tplg *tplg,
>>>       int ret;
>>>       int i;
>>> -    if (tplg->pass != SOC_TPLG_PASS_MIXER) {
>>> -        tplg->pos += le32_to_cpu(hdr->size) +
>>> -            le32_to_cpu(hdr->payload_size);
>>> -        return 0;
>>> -    }
>>> -
>>>       dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", 
>>> hdr->count,
>>>           soc_tplg_get_offset(tplg));
>>> @@ -1204,14 +1188,6 @@ static int 
>>> soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>>       count = le32_to_cpu(hdr->count);
>>> -    if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
>>> -        tplg->pos +=
>>> -            le32_to_cpu(hdr->size) +
>>> -            le32_to_cpu(hdr->payload_size);
>>> -
>>> -        return 0;
>>> -    }
>>> -
>>>       if (soc_tplg_check_elem_count(tplg,
>>>           sizeof(struct snd_soc_tplg_dapm_graph_elem),
>>>           count, le32_to_cpu(hdr->payload_size), "graph")) {
>>> @@ -1741,9 +1717,6 @@ static int 
>>> soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
>>>       count = le32_to_cpu(hdr->count);
>>> -    if (tplg->pass != SOC_TPLG_PASS_WIDGET)
>>> -        return 0;
>>> -
>>>       dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
>>>       for (i = 0; i < count; i++) {
>>> @@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct 
>>> soc_tplg *tplg,
>>>       count = le32_to_cpu(hdr->count);
>>> -    if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
>>> -        return 0;
>>> -
>>>       /* check the element size and count */
>>>       pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
>>>       size = le32_to_cpu(pcm->size);
>>> @@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct 
>>> soc_tplg *tplg,
>>>       count = le32_to_cpu(hdr->count);
>>> -    if (tplg->pass != SOC_TPLG_PASS_LINK) {
>>> -        tplg->pos += le32_to_cpu(hdr->size) +
>>> -            le32_to_cpu(hdr->payload_size);
>>> -        return 0;
>>> -    }
>>> -
>>>       /* check the element size and count */
>>>       link = (struct snd_soc_tplg_link_config *)tplg->pos;
>>>       size = le32_to_cpu(link->size);
>>> @@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct 
>>> soc_tplg *tplg,
>>>       count = le32_to_cpu(hdr->count);
>>> -    if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
>>> -        return 0;
>>> -
>>>       /* config the existing BE DAIs */
>>>       for (i = 0; i < count; i++) {
>>>           dai = (struct snd_soc_tplg_dai *)tplg->pos;
>>> @@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct 
>>> soc_tplg *tplg,
>>>       bool abi_match;
>>>       int ret = 0;
>>> -    if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
>>> -        return 0;
>>> -
>>>       manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
>>>       /* check ABI version by size, create a new manifest if abi not 
>>> match */
>>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-18  9:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12 18:23 [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass Keyon Jie
2020-05-12 18:23 ` [PATCH 2/2] ASoC: topology: remove the redundant pass checks Keyon Jie
2020-05-12 19:40   ` Pierre-Louis Bossart
2020-05-13  2:36     ` Keyon Jie
2020-05-18  9:40       ` Keyon Jie
2020-05-13  2:43 ` [PATCH 1/2] ASoC: topology: refine and log the header in the correct pass kbuild test robot
2020-05-13  2:43   ` kbuild test robot
2020-05-13  2:43 ` [RFC PATCH] ASoC: topology: elem_load[] can be static kbuild test robot
2020-05-13  2:43   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.