From: Takashi Iwai <tiwai@suse.de> To: Mohan Kumar D <mkumard@nvidia.com> Cc: tiwai@suse.com, kai.vehmanen@linux.intel.com, perex@perex.cz, ville.syrjala@linux.intel.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, thierry.reding@gmail.com, jonathanh@nvidia.com Subject: Re: [PATCH] ALSA: hda: Avoid unsol event during RPM suspending Date: Mon, 28 Mar 2022 12:57:31 +0200 [thread overview] Message-ID: <s5ha6dal4ys.wl-tiwai@suse.de> (raw) In-Reply-To: <7f7934e6-137c-4d8d-049b-0ed5e57cf00b@nvidia.com> On Mon, 28 Mar 2022 12:19:03 +0200, Mohan Kumar D wrote: > > > On 3/28/2022 3:12 PM, Takashi Iwai wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, 28 Mar 2022 11:14:11 +0200, > > Mohan Kumar wrote: > >> There is a corner case with unsol event handling during codec runtime > >> suspending state. When the codec runtime suspend call initiated, the > >> codec->in_pm atomic variable would be 0, currently the codec runtime > >> suspend function calls snd_hdac_enter_pm() which will just increments > >> the codec->in_pm atomic variable. Consider unsol event happened just > >> after this step and before snd_hdac_leave_pm() in the codec runtime > >> suspend function. The snd_hdac_power_up_pm() in the unsol event > >> flow in hdmi_present_sense_via_verbs() function would just increment > >> the codec->in_pm atomic variable without calling pm_runtime_get_sync > >> function. > >> > >> As codec runtime suspend flow is already in progress and in parallel > >> unsol event is also accessing the codec verbs, as soon as codec > >> suspend flow completes and clocks are switched off before completing > >> the unsol event handling as both functions doesn't wait for each other. > >> This will result in below errors > >> > >> [ 589.428020] tegra-hda 3510000.hda: azx_get_response timeout, switching > >> to polling mode: last cmd=0x505f2f57 > >> [ 589.428344] tegra-hda 3510000.hda: spurious response 0x80000074:0x5, > >> last cmd=0x505f2f57 > >> [ 589.428547] tegra-hda 3510000.hda: spurious response 0x80000065:0x5, > >> last cmd=0x505f2f57 > >> > >> To avoid this, the unsol event flow should not perform any codec verb > >> related operations during RPM_SUSPENDING state. > >> > >> Signed-off-by: Mohan Kumar <mkumard@nvidia.com> > > Thanks, that's a hairy problem... > > > > The logic sounds good, but can we check the PM state before calling > > snd_hda_power_up_pm()? > > If am not wrong, PM apis exposed either provide RPM_ACTIVE or > RPM_SUSPENDED status. Don't see anything which provides info on > RPM_SUSPENDING. We might need to exactly know this state to fix this > issue. Well, maybe my question wasn't clear. What I meant was that your change below > ret = snd_hda_power_up_pm(codec); > - if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) > + if ((ret < 0 && pm_runtime_suspended(dev)) || > + (dev->power.runtime_status == RPM_SUSPENDING)) > goto out; can be rather like: > + if (dev->power.runtime_status == RPM_SUSPENDING) > + return; > ret = snd_hda_power_up_pm(codec); > if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) so that it skips unneeded power up/down calls. Basically the state is set at drivers/base/power/runtime.c rpm_suspend() just before calling the device's runtime_suspend callback. So the state is supposed to be same before and after snd_hda_power_up_pm() in that case. thanks, Takashi
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Mohan Kumar D <mkumard@nvidia.com> Cc: jonathanh@nvidia.com, alsa-devel@alsa-project.org, kai.vehmanen@linux.intel.com, linux-kernel@vger.kernel.org, tiwai@suse.com, thierry.reding@gmail.com, ville.syrjala@linux.intel.com Subject: Re: [PATCH] ALSA: hda: Avoid unsol event during RPM suspending Date: Mon, 28 Mar 2022 12:57:31 +0200 [thread overview] Message-ID: <s5ha6dal4ys.wl-tiwai@suse.de> (raw) In-Reply-To: <7f7934e6-137c-4d8d-049b-0ed5e57cf00b@nvidia.com> On Mon, 28 Mar 2022 12:19:03 +0200, Mohan Kumar D wrote: > > > On 3/28/2022 3:12 PM, Takashi Iwai wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, 28 Mar 2022 11:14:11 +0200, > > Mohan Kumar wrote: > >> There is a corner case with unsol event handling during codec runtime > >> suspending state. When the codec runtime suspend call initiated, the > >> codec->in_pm atomic variable would be 0, currently the codec runtime > >> suspend function calls snd_hdac_enter_pm() which will just increments > >> the codec->in_pm atomic variable. Consider unsol event happened just > >> after this step and before snd_hdac_leave_pm() in the codec runtime > >> suspend function. The snd_hdac_power_up_pm() in the unsol event > >> flow in hdmi_present_sense_via_verbs() function would just increment > >> the codec->in_pm atomic variable without calling pm_runtime_get_sync > >> function. > >> > >> As codec runtime suspend flow is already in progress and in parallel > >> unsol event is also accessing the codec verbs, as soon as codec > >> suspend flow completes and clocks are switched off before completing > >> the unsol event handling as both functions doesn't wait for each other. > >> This will result in below errors > >> > >> [ 589.428020] tegra-hda 3510000.hda: azx_get_response timeout, switching > >> to polling mode: last cmd=0x505f2f57 > >> [ 589.428344] tegra-hda 3510000.hda: spurious response 0x80000074:0x5, > >> last cmd=0x505f2f57 > >> [ 589.428547] tegra-hda 3510000.hda: spurious response 0x80000065:0x5, > >> last cmd=0x505f2f57 > >> > >> To avoid this, the unsol event flow should not perform any codec verb > >> related operations during RPM_SUSPENDING state. > >> > >> Signed-off-by: Mohan Kumar <mkumard@nvidia.com> > > Thanks, that's a hairy problem... > > > > The logic sounds good, but can we check the PM state before calling > > snd_hda_power_up_pm()? > > If am not wrong, PM apis exposed either provide RPM_ACTIVE or > RPM_SUSPENDED status. Don't see anything which provides info on > RPM_SUSPENDING. We might need to exactly know this state to fix this > issue. Well, maybe my question wasn't clear. What I meant was that your change below > ret = snd_hda_power_up_pm(codec); > - if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) > + if ((ret < 0 && pm_runtime_suspended(dev)) || > + (dev->power.runtime_status == RPM_SUSPENDING)) > goto out; can be rather like: > + if (dev->power.runtime_status == RPM_SUSPENDING) > + return; > ret = snd_hda_power_up_pm(codec); > if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) so that it skips unneeded power up/down calls. Basically the state is set at drivers/base/power/runtime.c rpm_suspend() just before calling the device's runtime_suspend callback. So the state is supposed to be same before and after snd_hda_power_up_pm() in that case. thanks, Takashi
next prev parent reply other threads:[~2022-03-28 10:57 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-28 9:14 [PATCH] ALSA: hda: Avoid unsol event during RPM suspending Mohan Kumar 2022-03-28 9:14 ` Mohan Kumar 2022-03-28 9:42 ` Takashi Iwai 2022-03-28 9:42 ` Takashi Iwai 2022-03-28 10:19 ` Mohan Kumar D 2022-03-28 10:19 ` Mohan Kumar D 2022-03-28 10:57 ` Takashi Iwai [this message] 2022-03-28 10:57 ` Takashi Iwai 2022-03-28 13:51 ` Mohan Kumar D 2022-03-28 13:51 ` Mohan Kumar D 2022-03-28 16:15 ` Takashi Iwai 2022-03-28 16:15 ` Takashi Iwai 2022-03-28 17:03 ` Mohan Kumar D 2022-03-28 17:03 ` Mohan Kumar D 2022-03-28 13:07 ` kernel test robot 2022-03-28 13:07 ` kernel test robot 2022-03-28 16:09 ` kernel test robot 2022-03-28 16:09 ` kernel test robot 2022-03-29 15:59 Mohan Kumar 2022-03-29 15:59 ` Mohan Kumar 2022-03-30 8:21 ` Takashi Iwai 2022-03-30 8:21 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5ha6dal4ys.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=jonathanh@nvidia.com \ --cc=kai.vehmanen@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mkumard@nvidia.com \ --cc=perex@perex.cz \ --cc=thierry.reding@gmail.com \ --cc=tiwai@suse.com \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.