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

[Lin, Mengdong]

> -----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.
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
	...
}

Thanks
Mengdong