[alsa-devel] [PATCH] ALSA: hda - Fix runtime PM accounting

[Takashi Iwai]

At Thu, 30 Aug 2012 14:17:19 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Wednesday, August 29, 2012 10:59 PM
> > 
> > The patch below is the revised version.  I think it's a good cleanup at the
> > same time.  Basically it makes pm_notify callback simply turning up/down
> > the controller power without checking extra flags there.  The conditions are
> > checked in the caller side instead.
> > 
> > The pm_notify is called at codec creation and deletion times (but without
> > checking extra flags), thus the refcounts can be managed more easily.
> > 
> 
> Hi Takashi,
> 
> Usage of the new pm_notify() looks nice. The v3 patch works well on my platform. 
> 
> However, maybe we can add some check before notifying the codec power-down in snd_hda_codec_free().
> 
> static void snd_hda_codec_free(struct hda_codec *codec)
> {
> 	...
> 	restore_init_pincfgs(codec); 
> #ifdef CONFIG_SND_HDA_POWER_SAVE
> 	cancel_delayed_work(&codec->power_work);
> 	flush_workqueue(codec->bus->workq);
> #endif
> 	...
> 	if (codec->patch_ops.free)
> 		codec->patch_ops.free(codec);
> 	module_put(codec->owner);
> 	hda_call_pm_notify(codec->bus, false);
> 	...
> }
> 
> restore_init_pincfgs() will resume the codec to D0, and since the power_save timeout is at least 1second, so it's almost always safe to call hda_call_pm_notify(codec->bus, false) here.
> But an extremely low possibility is: the codec will be suspended again and call hda_call_pm_notify(,false) in hda_power_work() before the work is cancelled and queue is flushed.

Yeah, the check seems slipped from my previous post by some reason.
Maybe I cherry-picked a wrong branch.
It should be
	if (codec->power_on)
		hda_call_pm_notify(codec->bus, false);
and it should be called before module_put().

> And the flag 'codec->d3_stop_clk_ok' also cannot tell us whether a pm_notify(, false) has already been called or not, because codec->patch_ops.free() may call hda_set_power_state (D3) unexpectedly.
> So, I think that each codec still needs a flag 'power_count' and let the pm_notify modify the count:
> 
> static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec,  bool power_up)
> {
> 	struct azx *chip = bus->private_data;
> 
> 	if (power_up) {
> 		pm_runtime_get_sync(&chip->pci->dev);
> 		codec->power_count ++;
> 	}
> 	else {
> 		pm_runtime_put_sync(&chip->pci->dev);
> 		codec->power_count --;
> 	}
> }
> 
> And 
> static void snd_hda_codec_free(struct hda_codec *codec)
> {
> 	...
> +#ifdef CONFIG_SND_HDA_POWER_SAVE
> +	If(codec-> power_count)
> 		hda_call_pm_notify(codec->bus, false);
> +#endif
> 	...
> }

It's not needed.  In the case of snd_hda_codec_free(), the only
interest is whether pm_notify down was already called for this this
particular codec.  And this can be checked by codec->power_on flag.


Takashi