* [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
@ 2020-07-22 17:35 Mateusz Gorski
2020-07-22 18:24 ` Pierre-Louis Bossart
2020-08-19 12:15 ` Mark Brown
0 siblings, 2 replies; 8+ messages in thread
From: Mateusz Gorski @ 2020-07-22 17:35 UTC (permalink / raw)
To: alsa-devel
Cc: Mateusz Gorski, cezary.rojewski, broonie, tiwai, pierre-louis.bossart
Different modules for HDMI codec are used depending on the
"hda_codec_use_common_hdmi" option being enabled or not. Driver private
context for both of them is different.
This leads to null-pointer dereference error when driver tries to set
autosuspend delay for HDMI codec while the option is off (hdac_hdmi
module is used for HDMI).
Change the string in conditional statement to "ehdaudio0D0" to ensure
that only the HDAudio codec is handled by this function.
Fixes: 5bf73b1b1dec ("ASoC: intel/skl/hda - fix oops on systems without i915 audio codec")
Signed-off-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
---
sound/soc/intel/boards/skl_hda_dsp_generic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
index ca4900036ead..bc50eda297ab 100644
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
+++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
@@ -181,7 +181,7 @@ static void skl_set_hda_codec_autosuspend_delay(struct snd_soc_card *card)
struct snd_soc_dai *dai;
for_each_card_rtds(card, rtd) {
- if (!strstr(rtd->dai_link->codecs->name, "ehdaudio"))
+ if (!strstr(rtd->dai_link->codecs->name, "ehdaudio0D0"))
continue;
dai = asoc_rtd_to_codec(rtd, 0);
hda_pvt = snd_soc_component_get_drvdata(dai->component);
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-07-22 17:35 [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay Mateusz Gorski
@ 2020-07-22 18:24 ` Pierre-Louis Bossart
2020-07-22 18:59 ` Cezary Rojewski
2020-08-19 12:15 ` Mark Brown
1 sibling, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-07-22 18:24 UTC (permalink / raw)
To: Mateusz Gorski, alsa-devel; +Cc: cezary.rojewski, broonie, tiwai
On 7/22/20 12:35 PM, Mateusz Gorski wrote:
> Different modules for HDMI codec are used depending on the
> "hda_codec_use_common_hdmi" option being enabled or not. Driver private
> context for both of them is different.
> This leads to null-pointer dereference error when driver tries to set
> autosuspend delay for HDMI codec while the option is off (hdac_hdmi
> module is used for HDMI).
>
> Change the string in conditional statement to "ehdaudio0D0" to ensure
> that only the HDAudio codec is handled by this function.
I am not sure this is correct.
I may be wrong, but my understanding is the following:
Before 5bf73b1b1dec, the driver would use the first dailink of the card,
and in the case of devices without an HDaudio codec (e.g. Up2 board) it
would set the auto suspend delay using that first dailink. See the code
in skl_hda_fill_card_info(), it reorders the dailinks when HDaudio
codecs are not present so if you test for 'edhaudio00' you no longer
allow for this HDMI-only case to be handled with autosuspend.
Kai would need to review this, so this will have to wait I am afraid.
>
> Fixes: 5bf73b1b1dec ("ASoC: intel/skl/hda - fix oops on systems without i915 audio codec")
> Signed-off-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
> ---
> sound/soc/intel/boards/skl_hda_dsp_generic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> index ca4900036ead..bc50eda297ab 100644
> --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c
> +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c
> @@ -181,7 +181,7 @@ static void skl_set_hda_codec_autosuspend_delay(struct snd_soc_card *card)
> struct snd_soc_dai *dai;
>
> for_each_card_rtds(card, rtd) {
> - if (!strstr(rtd->dai_link->codecs->name, "ehdaudio"))
> + if (!strstr(rtd->dai_link->codecs->name, "ehdaudio0D0"))
> continue;
> dai = asoc_rtd_to_codec(rtd, 0);
> hda_pvt = snd_soc_component_get_drvdata(dai->component);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-07-22 18:24 ` Pierre-Louis Bossart
@ 2020-07-22 18:59 ` Cezary Rojewski
2020-07-22 19:04 ` Pierre-Louis Bossart
0 siblings, 1 reply; 8+ messages in thread
From: Cezary Rojewski @ 2020-07-22 18:59 UTC (permalink / raw)
To: Pierre-Louis Bossart; +Cc: alsa-devel, broonie, tiwai, Mateusz Gorski
On 2020-07-22 8:24 PM, Pierre-Louis Bossart wrote:
>
>
> On 7/22/20 12:35 PM, Mateusz Gorski wrote:
>> Different modules for HDMI codec are used depending on the
>> "hda_codec_use_common_hdmi" option being enabled or not. Driver private
>> context for both of them is different.
>> This leads to null-pointer dereference error when driver tries to set
>> autosuspend delay for HDMI codec while the option is off (hdac_hdmi
>> module is used for HDMI).
>>
>> Change the string in conditional statement to "ehdaudio0D0" to ensure
>> that only the HDAudio codec is handled by this function.
>
> I am not sure this is correct.
>
> I may be wrong, but my understanding is the following:
>
> Before 5bf73b1b1dec, the driver would use the first dailink of the card,
> and in the case of devices without an HDaudio codec (e.g. Up2 board) it
> would set the auto suspend delay using that first dailink. See the code
> in skl_hda_fill_card_info(), it reorders the dailinks when HDaudio
> codecs are not present so if you test for 'edhaudio00' you no longer
> allow for this HDMI-only case to be handled with autosuspend.
>
> Kai would need to review this, so this will have to wait I am afraid.
>
So, for_each_card_rtds needs to be context aware (hdmi type). Right now,
introduced _autosuspend_delay is causing kernel panics.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-07-22 18:59 ` Cezary Rojewski
@ 2020-07-22 19:04 ` Pierre-Louis Bossart
2020-08-12 13:48 ` Kai Vehmanen
0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-07-22 19:04 UTC (permalink / raw)
To: Cezary Rojewski; +Cc: alsa-devel, broonie, tiwai, Mateusz Gorski
>>> Change the string in conditional statement to "ehdaudio0D0" to ensure
>>> that only the HDAudio codec is handled by this function.
>>
>> I am not sure this is correct.
>>
>> I may be wrong, but my understanding is the following:
>>
>> Before 5bf73b1b1dec, the driver would use the first dailink of the
>> card, and in the case of devices without an HDaudio codec (e.g. Up2
>> board) it would set the auto suspend delay using that first dailink.
>> See the code in skl_hda_fill_card_info(), it reorders the dailinks
>> when HDaudio codecs are not present so if you test for 'edhaudio00'
>> you no longer allow for this HDMI-only case to be handled with
>> autosuspend.
>>
>> Kai would need to review this, so this will have to wait I am afraid.
>>
>
> So, for_each_card_rtds needs to be context aware (hdmi type). Right now,
> introduced _autosuspend_delay is causing kernel panics.
The code before 5bf73b1b1dec would use an HDMI dailink when HDaudio
codecs are not present, so I don't really see the point on being context
aware. Either this never worked or there's a side effect. In both cases,
I would kindly ask that this does not get merged before Kai is back.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-07-22 19:04 ` Pierre-Louis Bossart
@ 2020-08-12 13:48 ` Kai Vehmanen
2020-08-17 14:32 ` Gorski, Mateusz
0 siblings, 1 reply; 8+ messages in thread
From: Kai Vehmanen @ 2020-08-12 13:48 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Mateusz Gorski, Cezary Rojewski, broonie, alsa-devel, tiwai
Hi,
On Wed, 22 Jul 2020, Pierre-Louis Bossart wrote:
> > > > Change the string in conditional statement to "ehdaudio0D0" to ensure
> > > > that only the HDAudio codec is handled by this function.
> > >
> > > I am not sure this is correct.
> > >
> > > I may be wrong, but my understanding is the following:
> > >
> > > Before 5bf73b1b1dec, the driver would use the first dailink of the card,
> > > and in the case of devices without an HDaudio codec (e.g. Up2 board) it
> > > would set the auto suspend delay using that first dailink. See the code in
> > > skl_hda_fill_card_info(), it reorders the dailinks when HDaudio codecs are
> > > not present so if you test for 'edhaudio00' you no longer allow for this
> > > HDMI-only case to be handled with autosuspend.
> > >
> > > Kai would need to review this, so this will have to wait I am afraid.
> > >
> >
> > So, for_each_card_rtds needs to be context aware (hdmi type). Right now,
> > introduced _autosuspend_delay is causing kernel panics.
>
> The code before 5bf73b1b1dec would use an HDMI dailink when HDaudio codecs are
> not present, so I don't really see the point on being context aware. Either
> this never worked or there's a side effect. In both cases, I would kindly ask
> that this does not get merged before Kai is back.
the patch from Mateusz might be most pragmatic way to solve this.
The original problem was not setting autosuspend for external HDA codecs
which cause jack detection issues with some codecs. So we added the call
to set autosuspend timeout for all HDA codecs (patch "ASoC: intel/skl/hda
- set autosuspend timeout for hda codecs"). This is not strictly needed
for HDMI, but as it (seemed) cleaner to just call autosuspend for all HDA
codecs, the patch did that. Later we have hit issues with special cases
for HDMI, first with the case of disabled HDMI codec (my patch "ASoC:
intel/skl/hda - fix oops on systems without i915 audio codec") and now
issues with systems using hdac_hdmi.
So what we really want to do is to confirm the codec driver is hdac_hda
(and not hdac_hdmi or any other drivers), and if yes, then call the
autosuspend function. I did spend some time trying to find a clean(er) way
to do this, but codec name seemed the best option. I'll test the hdmi-only
case, but I believe Mateusz patch will work in that case as well.
Br, Kai
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-08-12 13:48 ` Kai Vehmanen
@ 2020-08-17 14:32 ` Gorski, Mateusz
2020-08-17 16:35 ` Kai Vehmanen
0 siblings, 1 reply; 8+ messages in thread
From: Gorski, Mateusz @ 2020-08-17 14:32 UTC (permalink / raw)
To: Kai Vehmanen, Pierre-Louis Bossart
Cc: Cezary Rojewski, broonie, alsa-devel, tiwai
W dniu 8/12/2020 o 3:48 PM, Kai Vehmanen pisze:
> Hi,
>
> On Wed, 22 Jul 2020, Pierre-Louis Bossart wrote:
>
>>>>> Change the string in conditional statement to "ehdaudio0D0" to ensure
>>>>> that only the HDAudio codec is handled by this function.
>>>> I am not sure this is correct.
>>>>
>>>> I may be wrong, but my understanding is the following:
>>>>
>>>> Before 5bf73b1b1dec, the driver would use the first dailink of the card,
>>>> and in the case of devices without an HDaudio codec (e.g. Up2 board) it
>>>> would set the auto suspend delay using that first dailink. See the code in
>>>> skl_hda_fill_card_info(), it reorders the dailinks when HDaudio codecs are
>>>> not present so if you test for 'edhaudio00' you no longer allow for this
>>>> HDMI-only case to be handled with autosuspend.
>>>>
>>>> Kai would need to review this, so this will have to wait I am afraid.
>>>>
>>> So, for_each_card_rtds needs to be context aware (hdmi type). Right now,
>>> introduced _autosuspend_delay is causing kernel panics.
>> The code before 5bf73b1b1dec would use an HDMI dailink when HDaudio codecs are
>> not present, so I don't really see the point on being context aware. Either
>> this never worked or there's a side effect. In both cases, I would kindly ask
>> that this does not get merged before Kai is back.
> the patch from Mateusz might be most pragmatic way to solve this.
>
> The original problem was not setting autosuspend for external HDA codecs
> which cause jack detection issues with some codecs. So we added the call
> to set autosuspend timeout for all HDA codecs (patch "ASoC: intel/skl/hda
> - set autosuspend timeout for hda codecs"). This is not strictly needed
> for HDMI, but as it (seemed) cleaner to just call autosuspend for all HDA
> codecs, the patch did that. Later we have hit issues with special cases
> for HDMI, first with the case of disabled HDMI codec (my patch "ASoC:
> intel/skl/hda - fix oops on systems without i915 audio codec") and now
> issues with systems using hdac_hdmi.
>
> So what we really want to do is to confirm the codec driver is hdac_hda
> (and not hdac_hdmi or any other drivers), and if yes, then call the
> autosuspend function. I did spend some time trying to find a clean(er) way
> to do this, but codec name seemed the best option. I'll test the hdmi-only
> case, but I believe Mateusz patch will work in that case as well.
>
> Br, Kai
Thanks for the review,
is there anything else I can do with this patch?
Also, Mark, please let me know if I should resend this change.
Thanks,
Mateusz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-08-17 14:32 ` Gorski, Mateusz
@ 2020-08-17 16:35 ` Kai Vehmanen
0 siblings, 0 replies; 8+ messages in thread
From: Kai Vehmanen @ 2020-08-17 16:35 UTC (permalink / raw)
To: Gorski, Mateusz
Cc: alsa-devel, Kai Vehmanen, tiwai, Cezary Rojewski,
Pierre-Louis Bossart, broonie
Hi,
On Mon, 17 Aug 2020, Gorski, Mateusz wrote:
> W dniu 8/12/2020 o 3:48 PM, Kai Vehmanen pisze:
> > So what we really want to do is to confirm the codec driver is hdac_hda
> > (and not hdac_hdmi or any other drivers), and if yes, then call the
> > autosuspend function. I did spend some time trying to find a clean(er) way
> > to do this, but codec name seemed the best option. I'll test the hdmi-only
> > case, but I believe Mateusz patch will work in that case as well.
>
> is there anything else I can do with this patch?
I today tested the patch on systems with only HDMI and it does the right
thing as expected. I also took another look at ways to more cleanly
separate between hdac-hdmi and hdac-hda here, but so far nothing cleaner.
As this is fixing a clear regression, I'll give my:
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Br, Kai
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
2020-07-22 17:35 [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay Mateusz Gorski
2020-07-22 18:24 ` Pierre-Louis Bossart
@ 2020-08-19 12:15 ` Mark Brown
1 sibling, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-08-19 12:15 UTC (permalink / raw)
To: alsa-devel, Mateusz Gorski; +Cc: cezary.rojewski, pierre-louis.bossart, tiwai
On Wed, 22 Jul 2020 19:35:24 +0200, Mateusz Gorski wrote:
> Different modules for HDMI codec are used depending on the
> "hda_codec_use_common_hdmi" option being enabled or not. Driver private
> context for both of them is different.
> This leads to null-pointer dereference error when driver tries to set
> autosuspend delay for HDMI codec while the option is off (hdac_hdmi
> module is used for HDMI).
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay
commit: 5610921a4435ef45c33702073e64f835f3dca7f1
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-19 12:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 17:35 [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay Mateusz Gorski
2020-07-22 18:24 ` Pierre-Louis Bossart
2020-07-22 18:59 ` Cezary Rojewski
2020-07-22 19:04 ` Pierre-Louis Bossart
2020-08-12 13:48 ` Kai Vehmanen
2020-08-17 14:32 ` Gorski, Mateusz
2020-08-17 16:35 ` Kai Vehmanen
2020-08-19 12:15 ` 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.