ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
diff mbox series

Message ID 20200122181254.22801-1-cezary.rojewski@intel.com
State Accepted
Commit 15adb20f64c302b31e10ad50f22bb224052ce1df
Headers show
Series
  • ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
Related show

Commit Message

Cezary Rojewski Jan. 22, 2020, 6:12 p.m. UTC
Definitions for idisp snd_soc_dai_links within skl_hda_dsp_common are
missing platform component. Add it to address following bug reported by
KASAN:

[   10.538502] BUG: KASAN: global-out-of-bounds in skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
[   10.538509] Write of size 8 at addr ffffffffc0606840 by task systemd-udevd/299
(...)
[   10.538519] Call Trace:
[   10.538524]  dump_stack+0x62/0x95
[   10.538528]  print_address_description+0x2f5/0x3b0
[   10.538532]  ? skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
[   10.538535]  __kasan_report+0x134/0x191
[   10.538538]  ? skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
[   10.538542]  ? skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
[   10.538544]  kasan_report+0x12/0x20
[   10.538546]  __asan_store8+0x57/0x90
[   10.538550]  skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
[   10.538553]  platform_drv_probe+0x51/0xb0
[   10.538556]  really_probe+0x311/0x600
[   10.538559]  driver_probe_device+0x87/0x1b0
[   10.538562]  device_driver_attach+0x8f/0xa0
[   10.538565]  ? device_driver_attach+0xa0/0xa0
[   10.538567]  __driver_attach+0x102/0x1a0
[   10.538569]  ? device_driver_attach+0xa0/0xa0
[   10.538572]  bus_for_each_dev+0xe8/0x160
[   10.538574]  ? subsys_dev_iter_exit+0x10/0x10
[   10.538577]  ? preempt_count_sub+0x18/0xc0
[   10.538580]  ? _raw_write_unlock+0x1f/0x40
[   10.538582]  driver_attach+0x2b/0x30
[   10.538585]  bus_add_driver+0x251/0x340
[   10.538588]  driver_register+0xd3/0x1c0
[   10.538590]  __platform_driver_register+0x6c/0x80
[   10.538592]  ? 0xffffffffc03e8000
[   10.538595]  skl_hda_audio_init+0x1c/0x1000 [snd_soc_skl_hda_dsp]
[   10.538598]  do_one_initcall+0xd0/0x36a
[   10.538600]  ? trace_event_raw_event_initcall_finish+0x160/0x160
[   10.538602]  ? kasan_unpoison_shadow+0x36/0x50
[   10.538605]  ? __kasan_kmalloc+0xcc/0xe0
[   10.538607]  ? kasan_unpoison_shadow+0x36/0x50
[   10.538609]  ? kasan_poison_shadow+0x2f/0x40
[   10.538612]  ? __asan_register_globals+0x65/0x80
[   10.538615]  do_init_module+0xf9/0x36f
[   10.538619]  load_module+0x398e/0x4590
[   10.538625]  ? module_frob_arch_sections+0x20/0x20
[   10.538628]  ? __kasan_check_write+0x14/0x20
[   10.538630]  ? kernel_read+0x9a/0xc0
[   10.538632]  ? __kasan_check_write+0x14/0x20
[   10.538634]  ? kernel_read_file+0x1d3/0x3c0
[   10.538638]  ? cap_capable+0xca/0x110
[   10.538642]  __do_sys_finit_module+0x190/0x1d0
[   10.538644]  ? __do_sys_finit_module+0x190/0x1d0
[   10.538646]  ? __x64_sys_init_module+0x50/0x50
[   10.538649]  ? expand_files+0x380/0x380
[   10.538652]  ? __kasan_check_write+0x14/0x20
[   10.538654]  ? fput_many+0x20/0xc0
[   10.538658]  __x64_sys_finit_module+0x43/0x50
[   10.538660]  do_syscall_64+0xce/0x700
[   10.538662]  ? syscall_return_slowpath+0x230/0x230
[   10.538665]  ? __do_page_fault+0x51e/0x640
[   10.538668]  ? __kasan_check_read+0x11/0x20
[   10.538670]  ? prepare_exit_to_usermode+0xc7/0x200
[   10.538673]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: a78959f407e6 ("ASoC: Intel: skl_hda_dsp_common: use modern dai_link style")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/boards/skl_hda_dsp_common.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

Comments

Cezary Rojewski Jan. 22, 2020, 6:27 p.m. UTC | #1
For the last few days we have been playing with "vanilla" 5.5 kernel - 
one without ton of /skylake patches - to find out how could hda-dsp be 
enabled on skl/ kbl+ with the least amount of changes pulled from our 
branch possible.

Turned out the addition of this single patch AND topology binary update 
got the job done.

Now, how can we proceed with such solution. Can share the topology 
binary/ .conf if needed, so anyone interested can check it out.

Regards,
Czarek
Pierre-Louis Bossart Jan. 22, 2020, 7:52 p.m. UTC | #2
On 1/22/20 12:27 PM, Cezary Rojewski wrote:
> For the last few days we have been playing with "vanilla" 5.5 kernel - 
> one without ton of /skylake patches - to find out how could hda-dsp be 
> enabled on skl/ kbl+ with the least amount of changes pulled from our 
> branch possible.
> 
> Turned out the addition of this single patch AND topology binary update 
> got the job done.
> 
> Now, how can we proceed with such solution. Can share the topology 
> binary/ .conf if needed, so anyone interested can check it out.

I am personally interested for tests but I doubt this option is usable 
by anyone outside of Intel - additional issues with probe race 
conditions with i915, e.g. on Linus' Dell XPS 9350, no DMIC support and 
not selected anyways by Jaroslav's new logic, no UCM, and no plans for 
the use of the HDMI common codec.

In case you didn't see it, the Skylake driver 'HDaudio codec' option is 
suggested as one of the 'unsupported' features here:
https://github.com/thesofproject/linux/pull/1742

-Pierre
Pierre-Louis Bossart Jan. 22, 2020, 7:55 p.m. UTC | #3
On 1/22/20 12:12 PM, Cezary Rojewski wrote:
> Definitions for idisp snd_soc_dai_links within skl_hda_dsp_common are
> missing platform component. Add it to address following bug reported by
> KASAN:
> 
> [   10.538502] BUG: KASAN: global-out-of-bounds in skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
> [   10.538509] Write of size 8 at addr ffffffffc0606840 by task systemd-udevd/299
> (...)

You could probably skip the call trace, it doesn't really provide much 
information.

Kai and Ranjani, do you think this impacts SOF as well? Or does our BE 
override somehow suppresses the problem?

>   sound/soc/intel/boards/skl_hda_dsp_common.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
> index eb419e1ec42b..78ff5f24c40e 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_common.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
> @@ -41,16 +41,19 @@ int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
>   	return 0;
>   }
>   
> -SND_SOC_DAILINK_DEFS(idisp1,
> -	DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin")),
> +SND_SOC_DAILINK_DEF(idisp1_cpu,
> +	DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin")));
> +SND_SOC_DAILINK_DEF(idisp1_codec,
>   	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi1")));
>   
> -SND_SOC_DAILINK_DEFS(idisp2,
> -	DAILINK_COMP_ARRAY(COMP_CPU("iDisp2 Pin")),
> +SND_SOC_DAILINK_DEF(idisp2_cpu,
> +	DAILINK_COMP_ARRAY(COMP_CPU("iDisp2 Pin")));
> +SND_SOC_DAILINK_DEF(idisp2_codec,
>   	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi2")));
>   
> -SND_SOC_DAILINK_DEFS(idisp3,
> -	DAILINK_COMP_ARRAY(COMP_CPU("iDisp3 Pin")),
> +SND_SOC_DAILINK_DEF(idisp3_cpu,
> +	DAILINK_COMP_ARRAY(COMP_CPU("iDisp3 Pin")));
> +SND_SOC_DAILINK_DEF(idisp3_codec,
>   	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi3")));
>   
>   SND_SOC_DAILINK_DEF(analog_cpu,
> @@ -83,21 +86,21 @@ struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
>   		.id = 1,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
> -		SND_SOC_DAILINK_REG(idisp1),
> +		SND_SOC_DAILINK_REG(idisp1_cpu, idisp1_codec, platform),
>   	},
>   	{
>   		.name = "iDisp2",
>   		.id = 2,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
> -		SND_SOC_DAILINK_REG(idisp2),
> +		SND_SOC_DAILINK_REG(idisp2_cpu, idisp2_codec, platform),
>   	},
>   	{
>   		.name = "iDisp3",
>   		.id = 3,
>   		.dpcm_playback = 1,
>   		.no_pcm = 1,
> -		SND_SOC_DAILINK_REG(idisp3),
> +		SND_SOC_DAILINK_REG(idisp3_cpu, idisp3_codec, platform),
>   	},
>   	{
>   		.name = "Analog Playback and Capture",
>
Sridharan, Ranjani Jan. 22, 2020, 9:30 p.m. UTC | #4
On Wed, Jan 22, 2020 at 11:58 AM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

>
>
> On 1/22/20 12:12 PM, Cezary Rojewski wrote:
> > Definitions for idisp snd_soc_dai_links within skl_hda_dsp_common are
> > missing platform component. Add it to address following bug reported by
> > KASAN:
> >
> > [   10.538502] BUG: KASAN: global-out-of-bounds in
> skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
> > [   10.538509] Write of size 8 at addr ffffffffc0606840 by task
> systemd-udevd/299
> > (...)
>
> You could probably skip the call trace, it doesn't really provide much
> information.
>
> Kai and Ranjani, do you think this impacts SOF as well? Or does our BE
> override somehow suppresses the problem?
>
Hi Pierre/Cezary,

SOF does have the same problem too but I thought we're allowed to have dai
links without platform component? An alternative to adding the platform
component would be to do something like below.

Thanks,
Ranjani
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index 11eaee9ae41f..dacf8014b870 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -112,6 +112,7 @@ static char hda_soc_components[30];

 static int skl_hda_fill_card_info(struct snd_soc_acpi_mach_params
*mach_params)
 {
+       struct snd_soc_dai_link_component *platform;
        struct snd_soc_card *card = &hda_soc_card;
        struct snd_soc_dai_link *dai_link;
        u32 codec_count, codec_mask;
@@ -148,7 +149,8 @@ static int skl_hda_fill_card_info(struct
snd_soc_acpi_mach_params *mach_params)
        card->num_dapm_routes = num_route;

        for_each_card_prelinks(card, i, dai_link)
-               dai_link->platforms->name = mach_params->platform;
+               for_each_link_platforms(dai_link, i, platform)
+                       platform->name = mach_params->platform;

        return 0;
 }
Mark Brown Jan. 22, 2020, 9:32 p.m. UTC | #5
On Wed, Jan 22, 2020 at 01:30:04PM -0800, Sridharan, Ranjani wrote:

> SOF does have the same problem too but I thought we're allowed to have dai
> links without platform component? An alternative to adding the platform
> component would be to do something like below.

CODEC to CODEC links are supported.
Pierre-Louis Bossart Jan. 22, 2020, 9:54 p.m. UTC | #6
>> Kai and Ranjani, do you think this impacts SOF as well? Or does our BE
>> override somehow suppresses the problem?
>>
> Hi Pierre/Cezary,
> 
> SOF does have the same problem too but I thought we're allowed to have dai
> links without platform component? An alternative to adding the platform
> component would be to do something like below.
> 
> Thanks,
> Ranjani
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> index 11eaee9ae41f..dacf8014b870 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -112,6 +112,7 @@ static char hda_soc_components[30];
> 
>   static int skl_hda_fill_card_info(struct snd_soc_acpi_mach_params
> *mach_params)
>   {
> +       struct snd_soc_dai_link_component *platform;
>          struct snd_soc_card *card = &hda_soc_card;
>          struct snd_soc_dai_link *dai_link;
>          u32 codec_count, codec_mask;
> @@ -148,7 +149,8 @@ static int skl_hda_fill_card_info(struct
> snd_soc_acpi_mach_params *mach_params)
>          card->num_dapm_routes = num_route;
> 
>          for_each_card_prelinks(card, i, dai_link)
> -               dai_link->platforms->name = mach_params->platform;
> +               for_each_link_platforms(dai_link, i, platform)
> +                       platform->name = mach_params->platform;

we already do this indirectly with:

skl_hda_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link 
*link)
{
	link->platforms->name = ctx->platform_name; <<<

I suspect the issue is that the plaforms part is not allocated. The 
8-byte out of bounds is not a string, it looks like a pointer stored in 
the wrong location.
Sridharan, Ranjani Jan. 22, 2020, 11:04 p.m. UTC | #7
On Wed, Jan 22, 2020 at 2:50 PM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

>
> >> Kai and Ranjani, do you think this impacts SOF as well? Or does our BE
> >> override somehow suppresses the problem?
> >>
> > Hi Pierre/Cezary,
> >
> > SOF does have the same problem too but I thought we're allowed to have
> dai
> > links without platform component? An alternative to adding the platform
> > component would be to do something like below.
> >
> > Thanks,
> > Ranjani
> > diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > index 11eaee9ae41f..dacf8014b870 100644
> > --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> > @@ -112,6 +112,7 @@ static char hda_soc_components[30];
> >
> >   static int skl_hda_fill_card_info(struct snd_soc_acpi_mach_params
> > *mach_params)
> >   {
> > +       struct snd_soc_dai_link_component *platform;
> >          struct snd_soc_card *card = &hda_soc_card;
> >          struct snd_soc_dai_link *dai_link;
> >          u32 codec_count, codec_mask;
> > @@ -148,7 +149,8 @@ static int skl_hda_fill_card_info(struct
> > snd_soc_acpi_mach_params *mach_params)
> >          card->num_dapm_routes = num_route;
> >
> >          for_each_card_prelinks(card, i, dai_link)
> > -               dai_link->platforms->name = mach_params->platform;
> > +               for_each_link_platforms(dai_link, i, platform)
> > +                       platform->name = mach_params->platform;
>
> we already do this indirectly with:
>
> skl_hda_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link
> *link)
> {
>         link->platforms->name = ctx->platform_name; <<<
>
> I suspect the issue is that the plaforms part is not allocated. The
> 8-byte out of bounds is not a string, it looks like a pointer stored in
> the wrong location.
>
skl_hda_fill_card_info() would be called before skl_hda_add_dai_link(). But
yes, it should be fixed here as well or as suggested in the patch, we
should set the platform component to prevent this error.

Thanks,
Ranjani
Kai Vehmanen Jan. 23, 2020, 8:31 a.m. UTC | #8
Hi,

On Wed, 22 Jan 2020, Pierre-Louis Bossart wrote:

> On 1/22/20 12:12 PM, Cezary Rojewski wrote:
> > Definitions for idisp snd_soc_dai_links within skl_hda_dsp_common are
> > missing platform component. Add it to address following bug reported by
> > KASAN:
[...]
> > [   10.538502] BUG: KASAN: global-out-of-bounds in
> > skl_hda_audio_probe+0x13a/0x2b0 [snd_soc_skl_hda_dsp]
> > [   10.538509] Write of size 8 at addr ffffffffc0606840 by task
> > systemd-udevd/299
> > (...)
> 
> You could probably skip the call trace, it doesn't really provide much
> information.
> 
> Kai and Ranjani, do you think this impacts SOF as well? Or does our BE
> override somehow suppresses the problem?

yes, this is a good catch Cezary! We actually initialize the platform 
correctly in other machine drivers, so this is a specific bug in the 
generic HDA mach driver.

What happens is that 'platforms' is initialized to an empty array:

static struct snd_soc_dai_link_component idisp1_cpus[] = { { .dai_name = "iDisp1 Pin", } }; 
static struct snd_soc_dai_link_component idisp1_codecs[] = { { .name = "ehdaudio0D2", .dai_name = "intel-hdmi-hifi1", } };
static struct snd_soc_dai_link_component idisp1_platforms[] = { }

... but then before card registration, mach driver code assumes there is 
at least one platform in the array:

»       for_each_card_prelinks(card, i, dai_link)                      
»       »       dai_link->platforms->name = mach_params->platform; 

... and this results in out-of-bound write.

Cezary's patch is aligned with other machine drivers and typical ASOC
macro usage so:

Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

I'll check if other machine drivers are impacted as well.

Br, Kai
Wasko, Michal Jan. 23, 2020, 3:10 p.m. UTC | #9
On 1/22/2020 8:52 PM, Pierre-Louis Bossart wrote:
>
>
> On 1/22/20 12:27 PM, Cezary Rojewski wrote:
>> For the last few days we have been playing with "vanilla" 5.5 kernel 
>> - one without ton of /skylake patches - to find out how could hda-dsp 
>> be enabled on skl/ kbl+ with the least amount of changes pulled from 
>> our branch possible.
>>
>> Turned out the addition of this single patch AND topology binary 
>> update got the job done.
>>
>> Now, how can we proceed with such solution. Can share the topology 
>> binary/ .conf if needed, so anyone interested can check it out.
>
> I am personally interested for tests but I doubt this option is usable 
> by anyone outside of Intel - additional issues with probe race 
> conditions with i915, e.g. on Linus' Dell XPS 9350, no DMIC support 
> and not selected anyways by Jaroslav's new logic, no UCM, and no plans 
> for the use of the HDMI common codec.

The Linux Skylake driver officially support audio over DSP on Intel cAVS 
1.5+ boards, that include Skylake HW target with hda-dsp configuration. 
The configuration is regularly tested by Intel Audio CI team.

As it was agreed with you Pierre the Skylake driver will be kept under 
maintenance and the proposed changes are about to keep hda-dsp 
configuration functional for anyone who would like to use it. Linus 
laptop issue is actually one of the good reasons why we would like to 
keep hda-dsp configuration functional

Your other statements Pierre are quite outdated:

     - Probe race conditions with i915 - resolved in HDA
     - DMIC is supported
     - UCM is not directly driver related and can be easily updated
     - Intel Audio CI was focused on common HD-A codec but the HDMI 
common codec is supported as well

> In case you didn't see it, the Skylake driver 'HDaudio codec' option 
> is suggested as one of the 'unsupported' features here:
> https://github.com/thesofproject/linux/pull/1742
>
> -Pierre

The suggestion to mark the Skylake driver 'HDaudio codec' option as 
'unsupported' is coming from you Pierre (patch from two daysago?) and I 
believe that you should consult such opinion with Intel Skylake driver 
maintainers.

Michal
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
Pierre-Louis Bossart Jan. 23, 2020, 6:22 p.m. UTC | #10
>>> For the last few days we have been playing with "vanilla" 5.5 kernel 
>>> - one without ton of /skylake patches - to find out how could hda-dsp 
>>> be enabled on skl/ kbl+ with the least amount of changes pulled from 
>>> our branch possible.
>>>
>>> Turned out the addition of this single patch AND topology binary 
>>> update got the job done.
>>>
>>> Now, how can we proceed with such solution. Can share the topology 
>>> binary/ .conf if needed, so anyone interested can check it out.
>>
>> I am personally interested for tests but I doubt this option is usable 
>> by anyone outside of Intel - additional issues with probe race 
>> conditions with i915, e.g. on Linus' Dell XPS 9350, no DMIC support 
>> and not selected anyways by Jaroslav's new logic, no UCM, and no plans 
>> for the use of the HDMI common codec.
> 
> The Linux Skylake driver officially support audio over DSP on Intel cAVS 
> 1.5+ boards, that include Skylake HW target with hda-dsp configuration. 
> The configuration is regularly tested by Intel Audio CI team.
> 
> As it was agreed with you Pierre the Skylake driver will be kept under 
> maintenance and the proposed changes are about to keep hda-dsp 
> configuration functional for anyone who would like to use it. Linus 
> laptop issue is actually one of the good reasons why we would like to 
> keep hda-dsp configuration functional

We have to agree on what 'maintained' means then.

I don't mind leaving the Skylake driver in the kernel and letting people 
who have access to Intel support use it. Cezary is listed as the 
maintainer as I suggested it, and this patch provides an necessary fix.

But does this mean this Hdaudio option is usable by distributions and 
Linux users who don't have access to Intel support?

I will assert that it's not, based on my own experience only 2 weeks 
ago. I tried to make audio work on a KBL NUC and had to comment stuff 
out due to an obsolete topology. see 
https://github.com/thesofproject/linux/pull/1667#issuecomment-572312157

You should also look at the help text for the option:

config SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC
	bool "HDAudio codec support"
	help
	  This option broke audio on Linus' Skylake laptop in December 2018
	  and the race conditions during the probe were not fixed since.
	  This option is DEPRECATED, all HDaudio codec support needs
	  to be handled by the SOF driver.
	  Distributions should not enable this option and there are no known
	  users of this capability.

No one objected to this wording back in October, but we still see this 
option selected in multiple distros, so the last suggestion is to move 
to an opt-in selection to guide distributions.

> Your other statements Pierre are quite outdated:
> 
>      - Probe race conditions with i915 - resolved in HDA

I checked last month and things still break on the Dell XPS. There are 
challenging race conditions that are not seen on Intel RVPs and NUCs, 
but broke Linus' laptop and a slew of others:

https://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143549.html

https://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143596.html

Unless you've verified SST support on those platforms, your claim of 
'resolved' is invalid.

>      - DMIC is supported

There is no topology provided with DMIC+HDaudio support. I asked for 
this more than 18 months ago and it was never made available, even to 
me, and SOF become the default solution for HDAudio+DMIC cases.

>      - UCM is not directly driver related and can be easily updated

"easily", but hasn't been done in 18 months, and it actually takes a lot 
of work to get things right. Especially with the SST driver and the 
mixers required on the platform side since nothing is connected by default.

>      - Intel Audio CI was focused on common HD-A codec but the HDMI 
> common codec is supported as well
> 
>> In case you didn't see it, the Skylake driver 'HDaudio codec' option 
>> is suggested as one of the 'unsupported' features here:
>> https://github.com/thesofproject/linux/pull/1742
>>
>> -Pierre
> 
> The suggestion to mark the Skylake driver 'HDaudio codec' option as 
> 'unsupported' is coming from you Pierre (patch from two daysago?) and I 
> believe that you should consult such opinion with Intel Skylake driver 
> maintainers.

You were in copy and did not comment, same for Cezary.

Maybe 'unsupported' is too strong a word, but it was Takashi's 
suggestion :-)
Wasko, Michal Jan. 27, 2020, 1:05 p.m. UTC | #11
On 1/23/2020 7:22 PM, Pierre-Louis Bossart wrote:
>
>
>>>> For the last few days we have been playing with "vanilla" 5.5 
>>>> kernel - one without ton of /skylake patches - to find out how 
>>>> could hda-dsp be enabled on skl/ kbl+ with the least amount of 
>>>> changes pulled from our branch possible.
>>>>
>>>> Turned out the addition of this single patch AND topology binary 
>>>> update got the job done.
>>>>
>>>> Now, how can we proceed with such solution. Can share the topology 
>>>> binary/ .conf if needed, so anyone interested can check it out.
>>>
>>> I am personally interested for tests but I doubt this option is 
>>> usable by anyone outside of Intel - additional issues with probe 
>>> race conditions with i915, e.g. on Linus' Dell XPS 9350, no DMIC 
>>> support and not selected anyways by Jaroslav's new logic, no UCM, 
>>> and no plans for the use of the HDMI common codec.
>>
>> The Linux Skylake driver officially support audio over DSP on Intel 
>> cAVS 1.5+ boards, that include Skylake HW target with hda-dsp 
>> configuration. The configuration is regularly tested by Intel Audio 
>> CI team.
>>
>> As it was agreed with you Pierre the Skylake driver will be kept 
>> under maintenance and the proposed changes are about to keep hda-dsp 
>> configuration functional for anyone who would like to use it. Linus 
>> laptop issue is actually one of the good reasons why we would like to 
>> keep hda-dsp configuration functional
>
> We have to agree on what 'maintained' means then.
>
> I don't mind leaving the Skylake driver in the kernel and letting 
> people who have access to Intel support use it. Cezary is listed as 
> the maintainer as I suggested it, and this patch provides an necessary 
> fix.
>
> But does this mean this Hdaudio option is usable by distributions and 
> Linux users who don't have access to Intel support?
>
> I will assert that it's not, based on my own experience only 2 weeks 
> ago. I tried to make audio work on a KBL NUC and had to comment stuff 
> out due to an obsolete topology. see 
> https://github.com/thesofproject/linux/pull/1667#issuecomment-572312157
That is exactly the reason why we would like to update the Skylake 
driver upstream code and it configuration files so that it will be 
usable by the community and not only keep it functional internally. As 
it was clarified by Cezary, we would like to make a minimum number of 
changes that are required.

Is there Pierre any non-technical reason why we should not fix the 
Skylake driver code on the upstream?
>
> You should also look at the help text for the option:
>
> config SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC
>     bool "HDAudio codec support"
>     help
>       This option broke audio on Linus' Skylake laptop in December 2018
>       and the race conditions during the probe were not fixed since.
>       This option is DEPRECATED, all HDaudio codec support needs
>       to be handled by the SOF driver.
>       Distributions should not enable this option and there are no known
>       users of this capability.
>
> No one objected to this wording back in October, but we still see this 
> option selected in multiple distros, so the last suggestion is to move 
> to an opt-in selection to guide distributions.
>
>> Your other statements Pierre are quite outdated:
>>
>>      - Probe race conditions with i915 - resolved in HDA
>
> I checked last month and things still break on the Dell XPS. There are 
> challenging race conditions that are not seen on Intel RVPs and NUCs, 
> but broke Linus' laptop and a slew of others:
>
> https://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143549.html 
>
>
> https://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143596.html 
>
>
> Unless you've verified SST support on those platforms, your claim of 
> 'resolved' is invalid.
>
>>      - DMIC is supported
>
> There is no topology provided with DMIC+HDaudio support. I asked for 
> this more than 18 months ago and it was never made available, even to 
> me, and SOF become the default solution for HDAudio+DMIC cases.
>
>>      - UCM is not directly driver related and can be easily updated
>
> "easily", but hasn't been done in 18 months, and it actually takes a 
> lot of work to get things right. Especially with the SST driver and 
> the mixers required on the platform side since nothing is connected by 
> default.
>
>>      - Intel Audio CI was focused on common HD-A codec but the HDMI 
>> common codec is supported as well
>>
>>> In case you didn't see it, the Skylake driver 'HDaudio codec' option 
>>> is suggested as one of the 'unsupported' features here:
>>> https://github.com/thesofproject/linux/pull/1742
>>>
>>> -Pierre
>>
>> The suggestion to mark the Skylake driver 'HDaudio codec' option as 
>> 'unsupported' is coming from you Pierre (patch from two daysago?) and 
>> I believe that you should consult such opinion with Intel Skylake 
>> driver maintainers.
>
> You were in copy and did not comment, same for Cezary.
>
> Maybe 'unsupported' is too strong a word, but it was Takashi's 
> suggestion :-)
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Pierre-Louis Bossart Jan. 27, 2020, 3:30 p.m. UTC | #12
>>> As it was agreed with you Pierre the Skylake driver will be kept 
>>> under maintenance and the proposed changes are about to keep hda-dsp 
>>> configuration functional for anyone who would like to use it. Linus 
>>> laptop issue is actually one of the good reasons why we would like to 
>>> keep hda-dsp configuration functional
>>
>> We have to agree on what 'maintained' means then.
>>
>> I don't mind leaving the Skylake driver in the kernel and letting 
>> people who have access to Intel support use it. Cezary is listed as 
>> the maintainer as I suggested it, and this patch provides an necessary 
>> fix.
>>
>> But does this mean this Hdaudio option is usable by distributions and 
>> Linux users who don't have access to Intel support?
>>
>> I will assert that it's not, based on my own experience only 2 weeks 
>> ago. I tried to make audio work on a KBL NUC and had to comment stuff 
>> out due to an obsolete topology. see 
>> https://github.com/thesofproject/linux/pull/1667#issuecomment-572312157
> That is exactly the reason why we would like to update the Skylake 
> driver upstream code and it configuration files so that it will be 
> usable by the community and not only keep it functional internally. As 
> it was clarified by Cezary, we would like to make a minimum number of 
> changes that are required.
> 
> Is there Pierre any non-technical reason why we should not fix the 
> Skylake driver code on the upstream?

My comment was only regarding support of HDaudio codecs w/ the Skylake 
driver. I personally gave up trying to support this option after 1.5 yrs 
of recurring issues. It will take a lot more than minimal patches I am 
afraid if you want this option to work across all known commercial 
devices, you will have to address race conditions such as the probe 
failing when DRM is built as a module, or on specific SKL/APL devices.

If you are signing-up to do this work I have no objections, and if in 
addition you add support for DMICs you'd solve existing issues with 
KBL/AmberLake for which users are left without a solution.

Just be aware of what you are signing up for, it's not a 'minimal' effort.
Wasko, Michal Jan. 28, 2020, 7:40 p.m. UTC | #13
On 1/27/2020 4:30 PM, Pierre-Louis Bossart wrote:
>
>>>> As it was agreed with you Pierre the Skylake driver will be kept 
>>>> under maintenance and the proposed changes are about to keep 
>>>> hda-dsp configuration functional for anyone who would like to use 
>>>> it. Linus laptop issue is actually one of the good reasons why we 
>>>> would like to keep hda-dsp configuration functional
>>>
>>> We have to agree on what 'maintained' means then.
>>>
>>> I don't mind leaving the Skylake driver in the kernel and letting 
>>> people who have access to Intel support use it. Cezary is listed as 
>>> the maintainer as I suggested it, and this patch provides an 
>>> necessary fix.
>>>
>>> But does this mean this Hdaudio option is usable by distributions 
>>> and Linux users who don't have access to Intel support?
>>>
>>> I will assert that it's not, based on my own experience only 2 weeks 
>>> ago. I tried to make audio work on a KBL NUC and had to comment 
>>> stuff out due to an obsolete topology. see 
>>> https://github.com/thesofproject/linux/pull/1667#issuecomment-572312157
>> That is exactly the reason why we would like to update the Skylake 
>> driver upstream code and it configuration files so that it will be 
>> usable by the community and not only keep it functional internally. 
>> As it was clarified by Cezary, we would like to make a minimum number 
>> of changes that are required.
>>
>> Is there Pierre any non-technical reason why we should not fix the 
>> Skylake driver code on the upstream?
>
> My comment was only regarding support of HDaudio codecs w/ the Skylake 
> driver. I personally gave up trying to support this option after 1.5 
> yrs of recurring issues. It will take a lot more than minimal patches 
> I am afraid if you want this option to work across all known 
> commercial devices, you will have to address race conditions such as 
> the probe failing when DRM is built as a module, or on specific 
> SKL/APL devices.
>
> If you are signing-up to do this work I have no objections, and if in 
> addition you add support for DMICs you'd solve existing issues with 
> KBL/AmberLake for which users are left without a solution.
>
> Just be aware of what you are signing up for, it's not a 'minimal' 
> effort.
>
The proposed patch-set is to restore the Skylake driver functionality 
for Skylake base targets. I called it ‘minimal’ because last time when 
we have tried to upstream the wide range of patch-sets with code 
refactors it was rejected because of ‘maintenance’ mark on the driver.

As we discussed on the call we will take a closer look on the HW boards 
that continue to reproduce the race condition issue. However the fix on 
that specific problem will be addressed as a separate patch-set.
Additionally we will work on providing reference topology configuration 
files that support DMICs.

I can’t commit any timeframe but as long as we will be maintainers and 
the changes will be welcome on the upstream we will work on improving 
the functionality of the Skylake driver.

Patch
diff mbox series

diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c
index eb419e1ec42b..78ff5f24c40e 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_common.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_common.c
@@ -41,16 +41,19 @@  int skl_hda_hdmi_add_pcm(struct snd_soc_card *card, int device)
 	return 0;
 }
 
-SND_SOC_DAILINK_DEFS(idisp1,
-	DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin")),
+SND_SOC_DAILINK_DEF(idisp1_cpu,
+	DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin")));
+SND_SOC_DAILINK_DEF(idisp1_codec,
 	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi1")));
 
-SND_SOC_DAILINK_DEFS(idisp2,
-	DAILINK_COMP_ARRAY(COMP_CPU("iDisp2 Pin")),
+SND_SOC_DAILINK_DEF(idisp2_cpu,
+	DAILINK_COMP_ARRAY(COMP_CPU("iDisp2 Pin")));
+SND_SOC_DAILINK_DEF(idisp2_codec,
 	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi2")));
 
-SND_SOC_DAILINK_DEFS(idisp3,
-	DAILINK_COMP_ARRAY(COMP_CPU("iDisp3 Pin")),
+SND_SOC_DAILINK_DEF(idisp3_cpu,
+	DAILINK_COMP_ARRAY(COMP_CPU("iDisp3 Pin")));
+SND_SOC_DAILINK_DEF(idisp3_codec,
 	DAILINK_COMP_ARRAY(COMP_CODEC("ehdaudio0D2", "intel-hdmi-hifi3")));
 
 SND_SOC_DAILINK_DEF(analog_cpu,
@@ -83,21 +86,21 @@  struct snd_soc_dai_link skl_hda_be_dai_links[HDA_DSP_MAX_BE_DAI_LINKS] = {
 		.id = 1,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
-		SND_SOC_DAILINK_REG(idisp1),
+		SND_SOC_DAILINK_REG(idisp1_cpu, idisp1_codec, platform),
 	},
 	{
 		.name = "iDisp2",
 		.id = 2,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
-		SND_SOC_DAILINK_REG(idisp2),
+		SND_SOC_DAILINK_REG(idisp2_cpu, idisp2_codec, platform),
 	},
 	{
 		.name = "iDisp3",
 		.id = 3,
 		.dpcm_playback = 1,
 		.no_pcm = 1,
-		SND_SOC_DAILINK_REG(idisp3),
+		SND_SOC_DAILINK_REG(idisp3_cpu, idisp3_codec, platform),
 	},
 	{
 		.name = "Analog Playback and Capture",