On 3/28/2022 4:27 PM, Takashi Iwai wrote:
External email: Use caution opening links or attachments
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@xxxxxxxxxx>
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!, Make sense, will push the updated patch after testing with
latest suggestion.
thanks,
Takashi
