[PATCH 0/5] ASoC: Harden DAPM route checks and Intel fixes

[PATCH 0/5] ASoC: Harden DAPM route checks and Intel fixes

[Cezary Rojewski]

Set of loosely connected patches. Most impactful change is dropping any
permisiveness when snd_soc_dapm_add_routes() fails in soc-topology.c To
do it safely, disable route checks for all skylake-driver boards.

Relevant background:

Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
failures for Intel boards have been partially addressed by commit
a22ae72b86a4 ("ASoC: soc-core: isable route checks for legacy devices")
and its follow up but only skl_nau88l25_ssm4567.c is patched. The rest
of the boards still need fixing.

After that, removal of copy-pastas found in ssm4567.c and redundant code
in i2s_test.c for avs-boards.

Amadeusz Sławiński (1):
  ASoC: Intel: avs: i2s_test: Remove redundant dapm routes

Cezary Rojewski (4):
  ASoC: Intel: Disable route checks for Skylake boards
  ASoC: topology: Do not ignore route checks when parsing graphs
  ASoC: Intel: avs: ssm4567: Do not ignore route checks
  ASoC: Intel: avs: ssm4567: Board cleanup

 sound/soc/intel/avs/boards/i2s_test.c         | 79 -------------------
 sound/soc/intel/avs/boards/ssm4567.c          |  5 +-
 sound/soc/intel/boards/bxt_da7219_max98357a.c |  1 +
 sound/soc/intel/boards/bxt_rt298.c            |  1 +
 sound/soc/intel/boards/glk_rt5682_max98357a.c |  1 +
 sound/soc/intel/boards/kbl_da7219_max98357a.c |  1 +
 sound/soc/intel/boards/kbl_da7219_max98927.c  |  4 +
 sound/soc/intel/boards/kbl_rt5660.c           |  1 +
 sound/soc/intel/boards/kbl_rt5663_max98927.c  |  2 +
 .../intel/boards/kbl_rt5663_rt5514_max98927.c |  1 +
 sound/soc/intel/boards/skl_hda_dsp_generic.c  |  1 +
 .../soc/intel/boards/skl_nau88l25_max98357a.c |  1 +
 sound/soc/intel/boards/skl_rt286.c            |  1 +
 sound/soc/soc-topology.c                      |  5 +-
 14 files changed, 19 insertions(+), 85 deletions(-)

-- 
2.25.1

[PATCH 1/5] ASoC: Intel: Disable route checks for Skylake boards

[Cezary Rojewski]

Topology files that are propagated to the world and utilized by the
skylake-driver carry shortcomings in their SectionGraphs.

Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
failures for Intel boards have been partially addressed by commit
a22ae72b86a4 ("ASoC: soc-core: disable route checks for legacy devices")
and its follow up but only skl_nau88l25_ssm4567.c is patched. Fix the
problem for the rest of the boards.

Link: https://lore.kernel.org/all/20200309192744.18380-1-pierre-louis.bossart@linux.intel.com/
Fixes: daa480bde6b3 ("ASoC: soc-core: tidyup for snd_soc_dapm_add_routes()")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/boards/bxt_da7219_max98357a.c       | 1 +
 sound/soc/intel/boards/bxt_rt298.c                  | 1 +
 sound/soc/intel/boards/glk_rt5682_max98357a.c       | 1 +
 sound/soc/intel/boards/kbl_da7219_max98357a.c       | 1 +
 sound/soc/intel/boards/kbl_da7219_max98927.c        | 4 ++++
 sound/soc/intel/boards/kbl_rt5660.c                 | 1 +
 sound/soc/intel/boards/kbl_rt5663_max98927.c        | 2 ++
 sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 1 +
 sound/soc/intel/boards/skl_hda_dsp_generic.c        | 1 +
 sound/soc/intel/boards/skl_nau88l25_max98357a.c     | 1 +
 sound/soc/intel/boards/skl_rt286.c                  | 1 +
 11 files changed, 15 insertions(+)

diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c b/sound/soc/intel/boards/bxt_da7219_max98357a.c
index 540f7a29310a..3fe3f38c6cb6 100644
--- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
+++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
@@ -768,6 +768,7 @@ static struct snd_soc_card broxton_audio_card = {
 	.dapm_routes = audio_map,
 	.num_dapm_routes = ARRAY_SIZE(audio_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = bxt_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/bxt_rt298.c b/sound/soc/intel/boards/bxt_rt298.c
index c0eb65c14aa9..afc499be8db2 100644
--- a/sound/soc/intel/boards/bxt_rt298.c
+++ b/sound/soc/intel/boards/bxt_rt298.c
@@ -574,6 +574,7 @@ static struct snd_soc_card broxton_rt298 = {
 	.dapm_routes = broxton_rt298_map,
 	.num_dapm_routes = ARRAY_SIZE(broxton_rt298_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = bxt_card_late_probe,
 
 };
diff --git a/sound/soc/intel/boards/glk_rt5682_max98357a.c b/sound/soc/intel/boards/glk_rt5682_max98357a.c
index 657e4658234c..695ba9855233 100644
--- a/sound/soc/intel/boards/glk_rt5682_max98357a.c
+++ b/sound/soc/intel/boards/glk_rt5682_max98357a.c
@@ -613,6 +613,7 @@ static struct snd_soc_card glk_audio_card_rt5682_m98357a = {
 	.dapm_routes = geminilake_map,
 	.num_dapm_routes = ARRAY_SIZE(geminilake_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = glk_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/kbl_da7219_max98357a.c b/sound/soc/intel/boards/kbl_da7219_max98357a.c
index a5d8965303a8..9dbc15f9d1c9 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98357a.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98357a.c
@@ -639,6 +639,7 @@ static struct snd_soc_card kabylake_audio_card_da7219_m98357a = {
 	.dapm_routes = kabylake_map,
 	.num_dapm_routes = ARRAY_SIZE(kabylake_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index 98c11ec0adc0..e662da5af83b 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -1036,6 +1036,7 @@ static struct snd_soc_card kbl_audio_card_da7219_m98927 = {
 	.codec_conf = max98927_codec_conf,
 	.num_configs = ARRAY_SIZE(max98927_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
@@ -1054,6 +1055,7 @@ static struct snd_soc_card kbl_audio_card_max98927 = {
 	.codec_conf = max98927_codec_conf,
 	.num_configs = ARRAY_SIZE(max98927_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
@@ -1071,6 +1073,7 @@ static struct snd_soc_card kbl_audio_card_da7219_m98373 = {
 	.codec_conf = max98373_codec_conf,
 	.num_configs = ARRAY_SIZE(max98373_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
@@ -1088,6 +1091,7 @@ static struct snd_soc_card kbl_audio_card_max98373 = {
 	.codec_conf = max98373_codec_conf,
 	.num_configs = ARRAY_SIZE(max98373_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/kbl_rt5660.c b/sound/soc/intel/boards/kbl_rt5660.c
index 30e0aca161cd..894d127c482a 100644
--- a/sound/soc/intel/boards/kbl_rt5660.c
+++ b/sound/soc/intel/boards/kbl_rt5660.c
@@ -518,6 +518,7 @@ static struct snd_soc_card kabylake_audio_card_rt5660 = {
 	.dapm_routes = kabylake_rt5660_map,
 	.num_dapm_routes = ARRAY_SIZE(kabylake_rt5660_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/kbl_rt5663_max98927.c b/sound/soc/intel/boards/kbl_rt5663_max98927.c
index 9071b1f1cbd0..646e8ff8e961 100644
--- a/sound/soc/intel/boards/kbl_rt5663_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_max98927.c
@@ -966,6 +966,7 @@ static struct snd_soc_card kabylake_audio_card_rt5663_m98927 = {
 	.codec_conf = max98927_codec_conf,
 	.num_configs = ARRAY_SIZE(max98927_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
@@ -982,6 +983,7 @@ static struct snd_soc_card kabylake_audio_card_rt5663 = {
 	.dapm_routes = kabylake_5663_map,
 	.num_dapm_routes = ARRAY_SIZE(kabylake_5663_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index 178fe9c37df6..924d5d1de03a 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -791,6 +791,7 @@ static struct snd_soc_card kabylake_audio_card = {
 	.codec_conf = max98927_codec_conf,
 	.num_configs = ARRAY_SIZE(max98927_codec_conf),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = kabylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index 6e172719c979..54e8af2e10cc 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -100,6 +100,7 @@ static struct snd_soc_card hda_soc_card = {
 	.dapm_routes = skl_hda_map,
 	.add_dai_link = skl_hda_add_dai_link,
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = skl_hda_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/skl_nau88l25_max98357a.c b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
index 0e7025834594..e4630c33176e 100644
--- a/sound/soc/intel/boards/skl_nau88l25_max98357a.c
+++ b/sound/soc/intel/boards/skl_nau88l25_max98357a.c
@@ -654,6 +654,7 @@ static struct snd_soc_card skylake_audio_card = {
 	.dapm_routes = skylake_map,
 	.num_dapm_routes = ARRAY_SIZE(skylake_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = skylake_card_late_probe,
 };
 
diff --git a/sound/soc/intel/boards/skl_rt286.c b/sound/soc/intel/boards/skl_rt286.c
index c59c60e28091..9a8044274908 100644
--- a/sound/soc/intel/boards/skl_rt286.c
+++ b/sound/soc/intel/boards/skl_rt286.c
@@ -523,6 +523,7 @@ static struct snd_soc_card skylake_rt286 = {
 	.dapm_routes = skylake_rt286_map,
 	.num_dapm_routes = ARRAY_SIZE(skylake_rt286_map),
 	.fully_routed = true,
+	.disable_route_checks = true,
 	.late_probe = skylake_card_late_probe,
 };
 
-- 
2.25.1

Re: [PATCH 1/5] ASoC: Intel: Disable route checks for Skylake boards

[Pierre-Louis Bossart]

On 3/4/24 13:05, Cezary Rojewski wrote:
> Topology files that are propagated to the world and utilized by the
> skylake-driver carry shortcomings in their SectionGraphs.
> 
> Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
> snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
> failures for Intel boards have been partially addressed by commit
> a22ae72b86a4 ("ASoC: soc-core: disable route checks for legacy devices")
> and its follow up but only skl_nau88l25_ssm4567.c is patched. Fix the
> problem for the rest of the boards.
> 
> Link: https://lore.kernel.org/all/20200309192744.18380-1-pierre-louis.bossart@linux.intel.com/
> Fixes: daa480bde6b3 ("ASoC: soc-core: tidyup for snd_soc_dapm_add_routes()")
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/intel/boards/bxt_da7219_max98357a.c       | 1 +
>  sound/soc/intel/boards/bxt_rt298.c                  | 1 +
>  sound/soc/intel/boards/glk_rt5682_max98357a.c       | 1 +
>  sound/soc/intel/boards/kbl_da7219_max98357a.c       | 1 +
>  sound/soc/intel/boards/kbl_da7219_max98927.c        | 4 ++++
>  sound/soc/intel/boards/kbl_rt5660.c                 | 1 +
>  sound/soc/intel/boards/kbl_rt5663_max98927.c        | 2 ++
>  sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 1 +
>  sound/soc/intel/boards/skl_hda_dsp_generic.c        | 1 +

This HDAudio machine driver is shared with the SOF-based solutions and I
see no reason to change the route checking...

I don't agree with this change. Why can't you fix the broken topologies
instead, if indeed they 'carry shortcomings'?

Same for glk, this an SOF-based solution.

>  sound/soc/intel/boards/skl_nau88l25_max98357a.c     | 1 +
>  sound/soc/intel/boards/skl_rt286.c                  | 1 +
>  11 files changed, 15 insertions(+)

Re: [PATCH 1/5] ASoC: Intel: Disable route checks for Skylake boards

[Cezary Rojewski]

On 2024-03-04 8:28 PM, Pierre-Louis Bossart wrote:
> On 3/4/24 13:05, Cezary Rojewski wrote:
>> Topology files that are propagated to the world and utilized by the
>> skylake-driver carry shortcomings in their SectionGraphs.
>>
>> Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
>> snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
>> failures for Intel boards have been partially addressed by commit
>> a22ae72b86a4 ("ASoC: soc-core: disable route checks for legacy devices")
>> and its follow up but only skl_nau88l25_ssm4567.c is patched. Fix the
>> problem for the rest of the boards.
>>
>> Link: https://lore.kernel.org/all/20200309192744.18380-1-pierre-louis.bossart@linux.intel.com/
>> Fixes: daa480bde6b3 ("ASoC: soc-core: tidyup for snd_soc_dapm_add_routes()")
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/intel/boards/bxt_da7219_max98357a.c       | 1 +
>>   sound/soc/intel/boards/bxt_rt298.c                  | 1 +
>>   sound/soc/intel/boards/glk_rt5682_max98357a.c       | 1 +
>>   sound/soc/intel/boards/kbl_da7219_max98357a.c       | 1 +
>>   sound/soc/intel/boards/kbl_da7219_max98927.c        | 4 ++++
>>   sound/soc/intel/boards/kbl_rt5660.c                 | 1 +
>>   sound/soc/intel/boards/kbl_rt5663_max98927.c        | 2 ++
>>   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 1 +
>>   sound/soc/intel/boards/skl_hda_dsp_generic.c        | 1 +
> 
> This HDAudio machine driver is shared with the SOF-based solutions and I
> see no reason to change the route checking...
> 
> I don't agree with this change. Why can't you fix the broken topologies
> instead, if indeed they 'carry shortcomings'?
> 
> Same for glk, this an SOF-based solution.

Perhaps the flag could be set conditionally for those two?

Even when you address the problem in the topology file, you do not get 
any confirmation user replaced the invalid file. skylake-driver topology 
were not part of any firmware-alike package. Please note that I actually 
did all that I believe could be done to repair those topologies and 
provided valid references at avs-topology/for-skylake-driver [1].


[1]: 
https://github.com/thesofproject/avs-topology-xml/tree/for-skylake-driver

Re: [PATCH 1/5] ASoC: Intel: Disable route checks for Skylake boards

[Pierre-Louis Bossart]

On 3/4/24 14:40, Cezary Rojewski wrote:
> On 2024-03-04 8:28 PM, Pierre-Louis Bossart wrote:
>> On 3/4/24 13:05, Cezary Rojewski wrote:
>>> Topology files that are propagated to the world and utilized by the
>>> skylake-driver carry shortcomings in their SectionGraphs.
>>>
>>> Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
>>> snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
>>> failures for Intel boards have been partially addressed by commit
>>> a22ae72b86a4 ("ASoC: soc-core: disable route checks for legacy devices")
>>> and its follow up but only skl_nau88l25_ssm4567.c is patched. Fix the
>>> problem for the rest of the boards.
>>>
>>> Link:
>>> https://lore.kernel.org/all/20200309192744.18380-1-pierre-louis.bossart@linux.intel.com/
>>> Fixes: daa480bde6b3 ("ASoC: soc-core: tidyup for
>>> snd_soc_dapm_add_routes()")
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>   sound/soc/intel/boards/bxt_da7219_max98357a.c       | 1 +
>>>   sound/soc/intel/boards/bxt_rt298.c                  | 1 +
>>>   sound/soc/intel/boards/glk_rt5682_max98357a.c       | 1 +
>>>   sound/soc/intel/boards/kbl_da7219_max98357a.c       | 1 +
>>>   sound/soc/intel/boards/kbl_da7219_max98927.c        | 4 ++++
>>>   sound/soc/intel/boards/kbl_rt5660.c                 | 1 +
>>>   sound/soc/intel/boards/kbl_rt5663_max98927.c        | 2 ++
>>>   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 1 +
>>>   sound/soc/intel/boards/skl_hda_dsp_generic.c        | 1 +
>>
>> This HDAudio machine driver is shared with the SOF-based solutions and I
>> see no reason to change the route checking...
>>
>> I don't agree with this change. Why can't you fix the broken topologies
>> instead, if indeed they 'carry shortcomings'?
>>
>> Same for glk, this an SOF-based solution.
> 
> Perhaps the flag could be set conditionally for those two?
> 
> Even when you address the problem in the topology file, you do not get
> any confirmation user replaced the invalid file. skylake-driver topology
> were not part of any firmware-alike package. Please note that I actually
> did all that I believe could be done to repair those topologies and
> provided valid references at avs-topology/for-skylake-driver [1].

Humm, the commit daa480bde6b3 ("ASoC: soc-core: tidyup for
snd_soc_dapm_add_routes()") dates from August 2019 and was included in
kernel 5.4.

Why are we seeing issues on route checks with the Skylake driver in
2024? Are we saying that the card creation failed for the last 5 years?

I must be missing something here.

Re: [PATCH 1/5] ASoC: Intel: Disable route checks for Skylake boards

[Cezary Rojewski]

On 2024-03-04 10:02 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 3/4/24 14:40, Cezary Rojewski wrote:
>> On 2024-03-04 8:28 PM, Pierre-Louis Bossart wrote:
>>> On 3/4/24 13:05, Cezary Rojewski wrote:
>>>> Topology files that are propagated to the world and utilized by the
>>>> skylake-driver carry shortcomings in their SectionGraphs.
>>>>
>>>> Since commit daa480bde6b3 ("ASoC: soc-core: tidyup for
>>>> snd_soc_dapm_add_routes()") route checks are no longer permissive. Probe
>>>> failures for Intel boards have been partially addressed by commit
>>>> a22ae72b86a4 ("ASoC: soc-core: disable route checks for legacy devices")
>>>> and its follow up but only skl_nau88l25_ssm4567.c is patched. Fix the
>>>> problem for the rest of the boards.
>>>>
>>>> Link:
>>>> https://lore.kernel.org/all/20200309192744.18380-1-pierre-louis.bossart@linux.intel.com/
>>>> Fixes: daa480bde6b3 ("ASoC: soc-core: tidyup for
>>>> snd_soc_dapm_add_routes()")
>>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>>> ---
>>>>    sound/soc/intel/boards/bxt_da7219_max98357a.c       | 1 +
>>>>    sound/soc/intel/boards/bxt_rt298.c                  | 1 +
>>>>    sound/soc/intel/boards/glk_rt5682_max98357a.c       | 1 +
>>>>    sound/soc/intel/boards/kbl_da7219_max98357a.c       | 1 +
>>>>    sound/soc/intel/boards/kbl_da7219_max98927.c        | 4 ++++
>>>>    sound/soc/intel/boards/kbl_rt5660.c                 | 1 +
>>>>    sound/soc/intel/boards/kbl_rt5663_max98927.c        | 2 ++
>>>>    sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 1 +
>>>>    sound/soc/intel/boards/skl_hda_dsp_generic.c        | 1 +
>>>
>>> This HDAudio machine driver is shared with the SOF-based solutions and I
>>> see no reason to change the route checking...
>>>
>>> I don't agree with this change. Why can't you fix the broken topologies
>>> instead, if indeed they 'carry shortcomings'?
>>>
>>> Same for glk, this an SOF-based solution.
>>
>> Perhaps the flag could be set conditionally for those two?
>>
>> Even when you address the problem in the topology file, you do not get
>> any confirmation user replaced the invalid file. skylake-driver topology
>> were not part of any firmware-alike package. Please note that I actually
>> did all that I believe could be done to repair those topologies and
>> provided valid references at avs-topology/for-skylake-driver [1].
> 
> Humm, the commit daa480bde6b3 ("ASoC: soc-core: tidyup for
> snd_soc_dapm_add_routes()") dates from August 2019 and was included in
> kernel 5.4.
> 
> Why are we seeing issues on route checks with the Skylake driver in
> 2024? Are we saying that the card creation failed for the last 5 years?
> 
> I must be missing something here.

Nah, no customer would allow that kind of delay :D

Unfortunately, nothing out of ordinary. In 2018/19/20 we had to fix 
everything "immediately" and not everything got pushed to upstream-users.

[PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Cezary Rojewski]

One of the framework responsibilities is to ensure that the enumerated
DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While
the are checks in soc-core.c and soc-pcm.c that verify this, a component
driver may attempt to workaround this by loading an invalid graph
through the topology file.

Be strict and fail topology loading when invalid graph is encountered.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/soc-topology.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index d6d368837235..778f539d9ff5 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1083,8 +1083,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 			break;
 		}
 
-		/* add route, but keep going if some fail */
-		snd_soc_dapm_add_routes(dapm, route, 1);
+		ret = snd_soc_dapm_add_routes(dapm, route, 1);
+		if (ret && !dapm->card->disable_route_checks)
+			break;
 	}
 
 	return ret;
-- 
2.25.1

Re: [PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Pierre-Louis Bossart]

On 3/4/24 13:05, Cezary Rojewski wrote:
> One of the framework responsibilities is to ensure that the enumerated
> DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While
> the are checks in soc-core.c and soc-pcm.c that verify this, a component
> driver may attempt to workaround this by loading an invalid graph
> through the topology file.
> 
> Be strict and fail topology loading when invalid graph is encountered.

This is very invasive, it's perfectly possible that we have a number of
'broken' topologies where one path is 'invalid' but it doesn't impact
functionality.

This should be an opt-in behavior IMHO, not a blanket change.

> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  sound/soc/soc-topology.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index d6d368837235..778f539d9ff5 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1083,8 +1083,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>  			break;
>  		}
>  
> -		/* add route, but keep going if some fail */
> -		snd_soc_dapm_add_routes(dapm, route, 1);
> +		ret = snd_soc_dapm_add_routes(dapm, route, 1);
> +		if (ret && !dapm->card->disable_route_checks)
> +			break;
>  	}
>  
>  	return ret;

Re: [PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Cezary Rojewski]

On 2024-03-04 8:32 PM, Pierre-Louis Bossart wrote:
> On 3/4/24 13:05, Cezary Rojewski wrote:
>> One of the framework responsibilities is to ensure that the enumerated
>> DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While
>> the are checks in soc-core.c and soc-pcm.c that verify this, a component
>> driver may attempt to workaround this by loading an invalid graph
>> through the topology file.
>>
>> Be strict and fail topology loading when invalid graph is encountered.
> 
> This is very invasive, it's perfectly possible that we have a number of
> 'broken' topologies where one path is 'invalid' but it doesn't impact
> functionality.
> 
> This should be an opt-in behavior IMHO, not a blanket change.

To my best knowledge, soc-topology.c' first "customer" was the 
skylake-driver and the final details were cloudy at best back then.

Right now sound-drivers utilizing the topology feature do so in more 
refined fashion. Next, in ASoC we have three locations where 
snd_soc_dapm_add_routes() is called but error-checks are done only in 
2/3 of them. This is bogus.

If the intended way of using snd_soc_dapm_add_routes() is to ignore the 
return, it should be converted to void and flag ->disable_route_checks 
removed.


Kind regards,
Czarek


>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/soc-topology.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index d6d368837235..778f539d9ff5 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -1083,8 +1083,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>   			break;
>>   		}
>>   
>> -		/* add route, but keep going if some fail */
>> -		snd_soc_dapm_add_routes(dapm, route, 1);
>> +		ret = snd_soc_dapm_add_routes(dapm, route, 1);
>> +		if (ret && !dapm->card->disable_route_checks)
>> +			break;
>>   	}
>>   
>>   	return ret;

Re: [PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Pierre-Louis Bossart]

On 3/4/24 14:50, Cezary Rojewski wrote:
> On 2024-03-04 8:32 PM, Pierre-Louis Bossart wrote:
>> On 3/4/24 13:05, Cezary Rojewski wrote:
>>> One of the framework responsibilities is to ensure that the enumerated
>>> DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While
>>> the are checks in soc-core.c and soc-pcm.c that verify this, a component
>>> driver may attempt to workaround this by loading an invalid graph
>>> through the topology file.
>>>
>>> Be strict and fail topology loading when invalid graph is encountered.
>>
>> This is very invasive, it's perfectly possible that we have a number of
>> 'broken' topologies where one path is 'invalid' but it doesn't impact
>> functionality.
>>
>> This should be an opt-in behavior IMHO, not a blanket change.
> 
> To my best knowledge, soc-topology.c' first "customer" was the
> skylake-driver and the final details were cloudy at best back then.
> 
> Right now sound-drivers utilizing the topology feature do so in more
> refined fashion. Next, in ASoC we have three locations where
> snd_soc_dapm_add_routes() is called but error-checks are done only in
> 2/3 of them. This is bogus.

I don't disagree that it was a mistake to omit the check on the returned
value, but now that we have topologies in the wild we can't change the
error handling without a risk of breaking "working" solutions. Exhibit A
is what happened in the other places where this error check was added...

> If the intended way of using snd_soc_dapm_add_routes() is to ignore the
> return, it should be converted to void and flag ->disable_route_checks
> removed.

Now that would go back to an "anything goes" mode, not necessarily a
great step.

>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>   sound/soc/soc-topology.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>>> index d6d368837235..778f539d9ff5 100644
>>> --- a/sound/soc/soc-topology.c
>>> +++ b/sound/soc/soc-topology.c
>>> @@ -1083,8 +1083,9 @@ static int
>>> soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>>               break;
>>>           }
>>>   -        /* add route, but keep going if some fail */
>>> -        snd_soc_dapm_add_routes(dapm, route, 1);
>>> +        ret = snd_soc_dapm_add_routes(dapm, route, 1);
>>> +        if (ret && !dapm->card->disable_route_checks)
>>> +            break;

you could alternatively follow the example in soc-core.c, with a
dev_info() thrown if the route_checks is disabled and a dev_err() thrown
otherwise. At least this would expose the reason for the failure after a
change in error handling, and a means to 'restore' functionality for
specific cards if the topology cannot be updated.

>>>       }
>>>         return ret;

Re: [PATCH 2/5] ASoC: topology: Do not ignore route checks when parsing graphs

[Cezary Rojewski]

On 2024-03-04 10:25 PM, Pierre-Louis Bossart wrote:
> On 3/4/24 14:50, Cezary Rojewski wrote:
>> On 2024-03-04 8:32 PM, Pierre-Louis Bossart wrote:
>>> On 3/4/24 13:05, Cezary Rojewski wrote:
>>>> One of the framework responsibilities is to ensure that the enumerated
>>>> DPCMs are valid i.e.: a valid BE is connected to a valid FE DAI. While
>>>> the are checks in soc-core.c and soc-pcm.c that verify this, a component
>>>> driver may attempt to workaround this by loading an invalid graph
>>>> through the topology file.
>>>>
>>>> Be strict and fail topology loading when invalid graph is encountered.
>>>
>>> This is very invasive, it's perfectly possible that we have a number of
>>> 'broken' topologies where one path is 'invalid' but it doesn't impact
>>> functionality.
>>>
>>> This should be an opt-in behavior IMHO, not a blanket change.
>>
>> To my best knowledge, soc-topology.c' first "customer" was the
>> skylake-driver and the final details were cloudy at best back then.
>>
>> Right now sound-drivers utilizing the topology feature do so in more
>> refined fashion. Next, in ASoC we have three locations where
>> snd_soc_dapm_add_routes() is called but error-checks are done only in
>> 2/3 of them. This is bogus.
> 
> I don't disagree that it was a mistake to omit the check on the returned
> value, but now that we have topologies in the wild we can't change the
> error handling without a risk of breaking "working" solutions. Exhibit A
> is what happened in the other places where this error check was added...
> 
>> If the intended way of using snd_soc_dapm_add_routes() is to ignore the
>> return, it should be converted to void and flag ->disable_route_checks
>> removed.
> 
> Now that would go back to an "anything goes" mode, not necessarily a
> great step.
> 
>>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>>> ---
>>>>    sound/soc/soc-topology.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>>>> index d6d368837235..778f539d9ff5 100644
>>>> --- a/sound/soc/soc-topology.c
>>>> +++ b/sound/soc/soc-topology.c
>>>> @@ -1083,8 +1083,9 @@ static int
>>>> soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>>>                break;
>>>>            }
>>>>    -        /* add route, but keep going if some fail */
>>>> -        snd_soc_dapm_add_routes(dapm, route, 1);
>>>> +        ret = snd_soc_dapm_add_routes(dapm, route, 1);
>>>> +        if (ret && !dapm->card->disable_route_checks)
>>>> +            break;
> 
> you could alternatively follow the example in soc-core.c, with a
> dev_info() thrown if the route_checks is disabled and a dev_err() thrown
> otherwise. At least this would expose the reason for the failure after a
> change in error handling, and a means to 'restore' functionality for
> specific cards if the topology cannot be updated.

Sure, in the next revision I'll mimic the behaviour found in soc-core.c.

[PATCH 3/5] ASoC: Intel: avs: ssm4567: Do not ignore route checks

[Cezary Rojewski]

A copy-paste from intel/boards/skl_nau88l25_ssm4567.c made the avs's
equivalent disable route checks as well. Such behavior is not desired.

Fixes: 69ea14efe99b ("ASoC: Intel: avs: Add ssm4567 machine board")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/boards/ssm4567.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/intel/avs/boards/ssm4567.c b/sound/soc/intel/avs/boards/ssm4567.c
index 4a0e136835ff..b64be685dc23 100644
--- a/sound/soc/intel/avs/boards/ssm4567.c
+++ b/sound/soc/intel/avs/boards/ssm4567.c
@@ -172,7 +172,6 @@ static int avs_ssm4567_probe(struct platform_device *pdev)
 	card->dapm_routes = card_base_routes;
 	card->num_dapm_routes = ARRAY_SIZE(card_base_routes);
 	card->fully_routed = true;
-	card->disable_route_checks = true;
 
 	ret = snd_soc_fixup_dai_links_platform_name(card, pname);
 	if (ret)
-- 
2.25.1

[PATCH 4/5] ASoC: Intel: avs: ssm4567: Board cleanup

[Cezary Rojewski]

The card-name suffix and the DP-widgets are an unintended copy-paste
from skl_nau88215_ssm4567.c. Both are redundant.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/boards/ssm4567.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/intel/avs/boards/ssm4567.c b/sound/soc/intel/avs/boards/ssm4567.c
index b64be685dc23..abb87bb88fff 100644
--- a/sound/soc/intel/avs/boards/ssm4567.c
+++ b/sound/soc/intel/avs/boards/ssm4567.c
@@ -37,8 +37,6 @@ static const struct snd_kcontrol_new card_controls[] = {
 static const struct snd_soc_dapm_widget card_widgets[] = {
 	SND_SOC_DAPM_SPK("Left Speaker", NULL),
 	SND_SOC_DAPM_SPK("Right Speaker", NULL),
-	SND_SOC_DAPM_SPK("DP1", NULL),
-	SND_SOC_DAPM_SPK("DP2", NULL),
 };
 
 static const struct snd_soc_dapm_route card_base_routes[] = {
@@ -158,7 +156,7 @@ static int avs_ssm4567_probe(struct platform_device *pdev)
 	if (!card)
 		return -ENOMEM;
 
-	card->name = "avs_ssm4567-adi";
+	card->name = "avs_ssm4567";
 	card->dev = dev;
 	card->owner = THIS_MODULE;
 	card->dai_link = dai_link;
-- 
2.25.1

[PATCH 5/5] ASoC: Intel: avs: i2s_test: Remove redundant dapm routes

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Remove unnecessary widgets and routes as they are created by
snd_soc_dapm_connect_dai_link_widgets() automatically.

Link: https://lore.kernel.org/all/20230612110958.592674-1-brent.lu@intel.com/
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/boards/i2s_test.c | 79 ---------------------------
 1 file changed, 79 deletions(-)

diff --git a/sound/soc/intel/avs/boards/i2s_test.c b/sound/soc/intel/avs/boards/i2s_test.c
index 28f254eb0d03..282256d18cc6 100644
--- a/sound/soc/intel/avs/boards/i2s_test.c
+++ b/sound/soc/intel/avs/boards/i2s_test.c
@@ -54,76 +54,13 @@ static int avs_create_dai_link(struct device *dev, const char *platform_name, in
 	return 0;
 }
 
-static int avs_create_dapm_routes(struct device *dev, int ssp_port, int tdm_slot,
-				  struct snd_soc_dapm_route **routes, int *num_routes)
-{
-	struct snd_soc_dapm_route *dr;
-	const int num_dr = 2;
-
-	dr = devm_kcalloc(dev, num_dr, sizeof(*dr), GFP_KERNEL);
-	if (!dr)
-		return -ENOMEM;
-
-	dr[0].sink = devm_kasprintf(dev, GFP_KERNEL,
-				    AVS_STRING_FMT("ssp", "pb", ssp_port, tdm_slot));
-	dr[0].source = devm_kasprintf(dev, GFP_KERNEL,
-				      AVS_STRING_FMT("ssp", " Tx", ssp_port, tdm_slot));
-	if (!dr[0].sink || !dr[0].source)
-		return -ENOMEM;
-
-	dr[1].sink = devm_kasprintf(dev, GFP_KERNEL,
-				    AVS_STRING_FMT("ssp", " Rx", ssp_port, tdm_slot));
-	dr[1].source = devm_kasprintf(dev, GFP_KERNEL,
-				      AVS_STRING_FMT("ssp", "cp", ssp_port, tdm_slot));
-	if (!dr[1].sink || !dr[1].source)
-		return -ENOMEM;
-
-	*routes = dr;
-	*num_routes = num_dr;
-
-	return 0;
-}
-
-static int avs_create_dapm_widgets(struct device *dev, int ssp_port, int tdm_slot,
-				   struct snd_soc_dapm_widget **widgets, int *num_widgets)
-{
-	struct snd_soc_dapm_widget *dw;
-	const int num_dw = 2;
-
-	dw = devm_kcalloc(dev, num_dw, sizeof(*dw), GFP_KERNEL);
-	if (!dw)
-		return -ENOMEM;
-
-	dw[0].id = snd_soc_dapm_hp;
-	dw[0].reg = SND_SOC_NOPM;
-	dw[0].name = devm_kasprintf(dev, GFP_KERNEL,
-				    AVS_STRING_FMT("ssp", "pb", ssp_port, tdm_slot));
-	if (!dw[0].name)
-		return -ENOMEM;
-
-	dw[1].id = snd_soc_dapm_mic;
-	dw[1].reg = SND_SOC_NOPM;
-	dw[1].name = devm_kasprintf(dev, GFP_KERNEL,
-				    AVS_STRING_FMT("ssp", "cp", ssp_port, tdm_slot));
-	if (!dw[1].name)
-		return -ENOMEM;
-
-	*widgets = dw;
-	*num_widgets = num_dw;
-
-	return 0;
-}
-
 static int avs_i2s_test_probe(struct platform_device *pdev)
 {
-	struct snd_soc_dapm_widget *widgets;
-	struct snd_soc_dapm_route *routes;
 	struct snd_soc_dai_link *dai_link;
 	struct snd_soc_acpi_mach *mach;
 	struct snd_soc_card *card;
 	struct device *dev = &pdev->dev;
 	const char *pname;
-	int num_routes, num_widgets;
 	int ssp_port, tdm_slot, ret;
 
 	mach = dev_get_platdata(dev);
@@ -156,26 +93,10 @@ static int avs_i2s_test_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = avs_create_dapm_routes(dev, ssp_port, tdm_slot, &routes, &num_routes);
-	if (ret) {
-		dev_err(dev, "Failed to create dapm routes: %d\n", ret);
-		return ret;
-	}
-
-	ret = avs_create_dapm_widgets(dev, ssp_port, tdm_slot, &widgets, &num_widgets);
-	if (ret) {
-		dev_err(dev, "Failed to create dapm widgets: %d\n", ret);
-		return ret;
-	}
-
 	card->dev = dev;
 	card->owner = THIS_MODULE;
 	card->dai_link = dai_link;
 	card->num_links = 1;
-	card->dapm_routes = routes;
-	card->num_dapm_routes = num_routes;
-	card->dapm_widgets = widgets;
-	card->num_dapm_widgets = num_widgets;
 	card->fully_routed = true;
 
 	ret = snd_soc_fixup_dai_links_platform_name(card, pname);
-- 
2.25.1