[PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Mateusz Gorski]

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

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Pierre-Louis Bossart]

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

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Cezary Rojewski]

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.

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Pierre-Louis Bossart]

>>> 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.

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Kai Vehmanen]

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

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Gorski, Mateusz]

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

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Kai Vehmanen]

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

Re: [PATCH] ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay

[Mark Brown]

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