| Message ID | 20200122181254.22801-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 15adb20f64c302b31e10ad50f22bb224052ce1df |
| Headers | show |
| Series | ASoC: Intel: skl_hda_dsp_common: Fix global-out-of-bounds bug | expand |
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
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
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", >
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; }
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.
>> 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.
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
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
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 >
>>> 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 :-)
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
>>> 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.
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.
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",
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(-)