All of lore.kernel.org
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
@ 2020-01-22 18:12 Cezary Rojewski
  2020-01-22 18:27 ` Cezary Rojewski
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Cezary Rojewski @ 2020-01-22 18:12 UTC (permalink / raw)
  To: alsa-devel
  Cc: lgirdwood, Cezary Rojewski, broonie, tiwai, pierre-louis.bossart

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(-)

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",
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 18:12 [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug Cezary Rojewski
@ 2020-01-22 18:27 ` Cezary Rojewski
  2020-01-22 19:52   ` Pierre-Louis Bossart
  2020-01-22 19:55 ` Pierre-Louis Bossart
  2020-01-23 12:36 ` [alsa-devel] Applied "ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug" to the asoc tree Mark Brown
  2 siblings, 1 reply; 15+ messages in thread
From: Cezary Rojewski @ 2020-01-22 18:27 UTC (permalink / raw)
  To: broonie, tiwai, pierre-louis.bossart; +Cc: alsa-devel, lgirdwood

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 18:27 ` Cezary Rojewski
@ 2020-01-22 19:52   ` Pierre-Louis Bossart
  2020-01-23 15:10     ` Wasko, Michal
  0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-22 19:52 UTC (permalink / raw)
  To: Cezary Rojewski, broonie, tiwai; +Cc: alsa-devel, lgirdwood



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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 18:12 [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug Cezary Rojewski
  2020-01-22 18:27 ` Cezary Rojewski
@ 2020-01-22 19:55 ` Pierre-Louis Bossart
  2020-01-22 21:30   ` Sridharan, Ranjani
  2020-01-23  8:31   ` Kai Vehmanen
  2020-01-23 12:36 ` [alsa-devel] Applied "ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug" to the asoc tree Mark Brown
  2 siblings, 2 replies; 15+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-22 19:55 UTC (permalink / raw)
  To: Cezary Rojewski, alsa-devel
  Cc: broonie, Ranjani Sridharan, lgirdwood, Kai Vehmanen, tiwai



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",
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 19:55 ` Pierre-Louis Bossart
@ 2020-01-22 21:30   ` Sridharan, Ranjani
  2020-01-22 21:32     ` Mark Brown
  2020-01-22 21:54     ` Pierre-Louis Bossart
  2020-01-23  8:31   ` Kai Vehmanen
  1 sibling, 2 replies; 15+ messages in thread
From: Sridharan, Ranjani @ 2020-01-22 21:30 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Cezary Rojewski, Kai Vehmanen, Linux-ALSA, tiwai, Liam Girdwood,
	Ranjani Sridharan, Mark Brown

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;
 }
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 21:30   ` Sridharan, Ranjani
@ 2020-01-22 21:32     ` Mark Brown
  2020-01-22 21:54     ` Pierre-Louis Bossart
  1 sibling, 0 replies; 15+ messages in thread
From: Mark Brown @ 2020-01-22 21:32 UTC (permalink / raw)
  To: Sridharan, Ranjani
  Cc: Cezary Rojewski, Kai Vehmanen, tiwai, Liam Girdwood,
	Pierre-Louis Bossart, Ranjani Sridharan, Linux-ALSA


[-- Attachment #1.1: Type: text/plain, Size: 306 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 21:30   ` Sridharan, Ranjani
  2020-01-22 21:32     ` Mark Brown
@ 2020-01-22 21:54     ` Pierre-Louis Bossart
  2020-01-22 23:04       ` Sridharan, Ranjani
  1 sibling, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-22 21:54 UTC (permalink / raw)
  To: Sridharan, Ranjani
  Cc: Cezary Rojewski, Kai Vehmanen, Linux-ALSA, tiwai, Liam Girdwood,
	Ranjani Sridharan, Mark Brown


>> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 21:54     ` Pierre-Louis Bossart
@ 2020-01-22 23:04       ` Sridharan, Ranjani
  0 siblings, 0 replies; 15+ messages in thread
From: Sridharan, Ranjani @ 2020-01-22 23:04 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Cezary Rojewski, Kai Vehmanen, Linux-ALSA, tiwai, Liam Girdwood,
	Ranjani Sridharan, Mark Brown

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 19:55 ` Pierre-Louis Bossart
  2020-01-22 21:30   ` Sridharan, Ranjani
@ 2020-01-23  8:31   ` Kai Vehmanen
  1 sibling, 0 replies; 15+ messages in thread
From: Kai Vehmanen @ 2020-01-23  8:31 UTC (permalink / raw)
  To: Cezary Rojewski, Pierre-Louis Bossart
  Cc: alsa-devel, broonie, Ranjani Sridharan, lgirdwood, tiwai

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] Applied "ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug" to the asoc tree
  2020-01-22 18:12 [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug Cezary Rojewski
  2020-01-22 18:27 ` Cezary Rojewski
  2020-01-22 19:55 ` Pierre-Louis Bossart
@ 2020-01-23 12:36 ` Mark Brown
  2 siblings, 0 replies; 15+ messages in thread
From: Mark Brown @ 2020-01-23 12:36 UTC (permalink / raw)
  To: Cezary Rojewski
  Cc: alsa-devel, Mark Brown, tiwai, lgirdwood, pierre-louis.bossart

The patch

   ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 15adb20f64c302b31e10ad50f22bb224052ce1df Mon Sep 17 00:00:00 2001
From: Cezary Rojewski <cezary.rojewski@intel.com>
Date: Wed, 22 Jan 2020 19:12:54 +0100
Subject: [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug

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>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Link: https://lore.kernel.org/r/20200122181254.22801-1-cezary.rojewski@intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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",
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-22 19:52   ` Pierre-Louis Bossart
@ 2020-01-23 15:10     ` Wasko, Michal
  2020-01-23 18:22       ` Pierre-Louis Bossart
  0 siblings, 1 reply; 15+ messages in thread
From: Wasko, Michal @ 2020-01-23 15:10 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Cezary Rojewski, broonie, tiwai
  Cc: alsa-devel, lgirdwood


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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-23 15:10     ` Wasko, Michal
@ 2020-01-23 18:22       ` Pierre-Louis Bossart
  2020-01-27 13:05         ` Wasko, Michal
  0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-23 18:22 UTC (permalink / raw)
  To: Wasko, Michal, Cezary Rojewski, broonie, tiwai; +Cc: alsa-devel, lgirdwood



>>> 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 :-)
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-23 18:22       ` Pierre-Louis Bossart
@ 2020-01-27 13:05         ` Wasko, Michal
  2020-01-27 15:30           ` Pierre-Louis Bossart
  0 siblings, 1 reply; 15+ messages in thread
From: Wasko, Michal @ 2020-01-27 13:05 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Cezary Rojewski, broonie, tiwai
  Cc: alsa-devel, lgirdwood


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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-27 13:05         ` Wasko, Michal
@ 2020-01-27 15:30           ` Pierre-Louis Bossart
  2020-01-28 19:40             ` Wasko, Michal
  0 siblings, 1 reply; 15+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-27 15:30 UTC (permalink / raw)
  To: Wasko, Michal, Cezary Rojewski, broonie, tiwai; +Cc: alsa-devel, lgirdwood


>>> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug
  2020-01-27 15:30           ` Pierre-Louis Bossart
@ 2020-01-28 19:40             ` Wasko, Michal
  0 siblings, 0 replies; 15+ messages in thread
From: Wasko, Michal @ 2020-01-28 19:40 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Cezary Rojewski, broonie, tiwai
  Cc: alsa-devel, lgirdwood


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.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

end of thread, other threads:[~2020-01-28 20:06 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22 18:12 [alsa-devel] [PATCH] ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug Cezary Rojewski
2020-01-22 18:27 ` Cezary Rojewski
2020-01-22 19:52   ` Pierre-Louis Bossart
2020-01-23 15:10     ` Wasko, Michal
2020-01-23 18:22       ` Pierre-Louis Bossart
2020-01-27 13:05         ` Wasko, Michal
2020-01-27 15:30           ` Pierre-Louis Bossart
2020-01-28 19:40             ` Wasko, Michal
2020-01-22 19:55 ` Pierre-Louis Bossart
2020-01-22 21:30   ` Sridharan, Ranjani
2020-01-22 21:32     ` Mark Brown
2020-01-22 21:54     ` Pierre-Louis Bossart
2020-01-22 23:04       ` Sridharan, Ranjani
2020-01-23  8:31   ` Kai Vehmanen
2020-01-23 12:36 ` [alsa-devel] Applied "ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug" to the asoc tree Mark Brown

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.