| Message ID | 20200722173524.30161-1-mateusz.gorski@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5610921a4435ef45c33702073e64f835f3dca7f1 |
| Headers | show |
| Series | ASoC: Intel: skl_hda_dsp_generic: Fix NULLptr dereference in autosuspend delay | expand |
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); >
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.
>>> 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.
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
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
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
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
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);
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(-)