Re: [PATCH V1 1/4] ALSA: hda: fix controller cannot suspend when codec using jackpoll

[Takashi Iwai <tiwai@suse.de>]

On Thu, 19 Jun 2025 04:08:41 +0200,
joakim.zhang@cixtech.com wrote:
> 
> From: Joakim Zhang <joakim.zhang@cixtech.com>
> 
> For current design, sync cancel jackpoll delayed work in
> hda_codec_runtime_suspend(), sync means it will cancel pending
> works, so jackpoll work would be called.
>   hda_jackpoll_work()-->
>       snd_hda_power_up_pm()-->
>           if (!atomic_inc_not_zero(&codec->in_pm))
> 		return snd_hdac_power_up(codec);-->
> 		    pm_runtime_get_sync()
> 
> The point where cancel_delayed_work_sync() called not in_pm,
> though it actually is. It will runtime resume codec device again,
> the power usage_count increase 1, but it should be 0 when codec
> is suspending, finally codec is in resume state, not suspended.
> 
> HDA controller as the parent of codec device, also cannot enter
> suspend, this patch change to sync cancel jackpoll delayed work when
> codec in_pm is set, so it will not resume codec again.
> 
> Signed-off-by: Joakim Zhang <joakim.zhang@cixtech.com>
> ---
>  sound/pci/hda/hda_codec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index c018beeecd3d..12caa5b2e678 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2912,7 +2912,9 @@ static int hda_codec_runtime_suspend(struct device *dev)
>  	if (!codec->card)
>  		return 0;
>  
> +	snd_hdac_enter_pm(&codec->core);
>  	cancel_delayed_work_sync(&codec->jackpoll_work);
> +	snd_hdac_leave_pm(&codec->core);

That's too awkward...

I believe that, if the hda-codec doesn't set jackpoll_in_suspend flag,
we should simply disable the runtime PM.  Such a device is supposed to
be polling frequently (e.g. VIA codec) that conflicts with the runtime
PM.

For that, we can simply define the runtime_idle callback.

Another point is that hda_jackpoll_work() could simply use
snd_hda_power_up() and *_down() instead of snd_hda_power_up_pm() and
co.  We used *_pm variant because the function was called directly in
the init or the resume paths.  But we can call this only from the
work, then we can switch to the safer *_up/down() calls.  Then this
assures the runtime resume completion in the work without much races.

With those transitions, basically the cancel_delayed_work_sync() call
at hda_codec_runtime_suspend() can be dropped.  Also, the conditional
schedule_delayed_work() there, too; if we don't cancel, we don't need
reschedule, either.

So the (totally untested) PoC is something like below.
It contains also the fix corresponding to your patch 3, too.


thanks,

Takashi

---
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index c018beeecd3d..5508381a1833 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -639,24 +639,16 @@ static void hda_jackpoll_work(struct work_struct *work)
 	struct hda_codec *codec =
 		container_of(work, struct hda_codec, jackpoll_work.work);
 
-	/* for non-polling trigger: we need nothing if already powered on */
-	if (!codec->jackpoll_interval && snd_hdac_is_power_on(&codec->core))
-		return;
-
-	/* the power-up/down sequence triggers the runtime resume */
-	snd_hda_power_up_pm(codec);
-	/* update jacks manually if polling is required, too */
-	if (codec->jackpoll_interval) {
-		snd_hda_jack_set_dirty_all(codec);
-		snd_hda_jack_poll_all(codec);
-	}
-	snd_hda_power_down_pm(codec);
-
 	if (!codec->jackpoll_interval)
 		return;
 
-	schedule_delayed_work(&codec->jackpoll_work,
-			      codec->jackpoll_interval);
+	/* the power-up/down sequence triggers the runtime resume */
+	snd_hda_power_up(codec);
+	/* update jacks manually if polling is required, too */
+	snd_hda_jack_set_dirty_all(codec);
+	snd_hda_jack_poll_all(codec);
+	schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval);
+	snd_hda_power_down(codec);
 }
 
 /* release all pincfg lists */
@@ -2895,12 +2887,12 @@ static void hda_call_codec_resume(struct hda_codec *codec)
 		snd_hda_regmap_sync(codec);
 	}
 
-	if (codec->jackpoll_interval)
-		hda_jackpoll_work(&codec->jackpoll_work.work);
-	else
-		snd_hda_jack_report_sync(codec);
+	snd_hda_jack_report_sync(codec);
 	codec->core.dev.power.power_state = PMSG_ON;
 	snd_hdac_leave_pm(&codec->core);
+	if (codec->jackpoll_interval)
+		schedule_delayed_work(&codec->jackpoll_work,
+				      codec->jackpoll_interval);
 }
 
 static int hda_codec_runtime_suspend(struct device *dev)
@@ -2912,8 +2904,6 @@ static int hda_codec_runtime_suspend(struct device *dev)
 	if (!codec->card)
 		return 0;
 
-	cancel_delayed_work_sync(&codec->jackpoll_work);
-
 	state = hda_call_codec_suspend(codec);
 	if (codec->link_down_at_suspend ||
 	    (codec_has_clkstop(codec) && codec_has_epss(codec) &&
@@ -2921,10 +2911,6 @@ static int hda_codec_runtime_suspend(struct device *dev)
 		snd_hdac_codec_link_down(&codec->core);
 	snd_hda_codec_display_power(codec, false);
 
-	if (codec->bus->jackpoll_in_suspend &&
-		(dev->power.power_state.event != PM_EVENT_SUSPEND))
-		schedule_delayed_work(&codec->jackpoll_work,
-					codec->jackpoll_interval);
 	return 0;
 }
 
@@ -2943,6 +2929,15 @@ static int hda_codec_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int hda_codec_runtime_idle(struct device *dev)
+{
+	struct hda_codec *codec = dev_to_hda_codec(dev);
+
+	if (codec->jackpoll_interval && !codec->bus->jackpoll_in_suspend)
+		return -EBUSY;
+	return 0;
+}
+
 static int hda_codec_pm_prepare(struct device *dev)
 {
 	struct hda_codec *codec = dev_to_hda_codec(dev);
@@ -3008,7 +3003,8 @@ const struct dev_pm_ops hda_codec_driver_pm = {
 	.thaw = pm_sleep_ptr(hda_codec_pm_thaw),
 	.poweroff = pm_sleep_ptr(hda_codec_pm_suspend),
 	.restore = pm_sleep_ptr(hda_codec_pm_restore),
-	RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume, NULL)
+	RUNTIME_PM_OPS(hda_codec_runtime_suspend, hda_codec_runtime_resume,
+		       hda_codec_runtime_idle)
 };
 
 /* suspend the codec at shutdown; called from driver's shutdown callback */
@@ -3020,6 +3016,7 @@ void snd_hda_codec_shutdown(struct hda_codec *codec)
 	if (!codec->core.registered)
 		return;
 
+	codec->jackpoll_interval = 0; /* don't poll any longer */
 	cancel_delayed_work_sync(&codec->jackpoll_work);
 	list_for_each_entry(cpcm, &codec->pcm_list_head, list)
 		snd_pcm_suspend_all(cpcm->pcm);
@@ -3086,10 +3083,11 @@ int snd_hda_codec_build_controls(struct hda_codec *codec)
 	if (err < 0)
 		return err;
 
+	snd_hda_jack_report_sync(codec); /* call at the last init point */
 	if (codec->jackpoll_interval)
-		hda_jackpoll_work(&codec->jackpoll_work.work);
-	else
-		snd_hda_jack_report_sync(codec); /* call at the last init point */
+		schedule_delayed_work(&codec->jackpoll_work,
+				      codec->jackpoll_interval);
+
 	sync_power_up_states(codec);
 	return 0;
 }