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

[Lin, Mengdong]

> Now I introduced codec->epss flag that is initialized once in
> snd_hda_codec_new(), and let the codec driver overrides this flag.
> One bonus is that you can avoid the unnecessary codec parameter read at
> each power change.

Yes, this is better. And the patch works well for me - [PATCH 1/2] ALSA: hda - Avoid unnecessary parameter read for EPSS.

> In anyway, there are a few issues:
> - The refcounting gets broken after module unload.  It's because we
>   leave the codec's pm_runtime usages after removal.

Thanks for pointing out this! But I think the following codec can introduce risk of race among multiple codecs to modify ' chip->power_count',
because only one codec's power on/off is serialized.
static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec)
{
	 ...
	if (codec->power_on) {
		pm_runtime_get_sync(&chip->pci->dev);
		chip->power_count++;
	} else {
		pm_runtime_put_sync(&chip->pci->dev);
		chip->power_count--;
	}
}

How about calling pm_runtime_put_noidle() when freeing a codec in D0? 
Maybe we can add a bus notify function like this:
Static void azx_codec_free_notify(struct hda_codec *codec)
{
	If(codec->power_on)
		pm_runtime_put_noidle(&codec->bus->pci->dev);
}
And snd_hda_codec_free() can trigger this notify.

> - The D3-stop-clock check isn't done properly for the codecs providing
>   own set_power_state ops.

I cannot test the branch 'if (codec->patch_ops.set_power_state)' in hda_set_power_state() for lack of such a codec. 
But the logic seems right and the other branch works well for me :-)

> - Ditto for the repeated power-set sequence, the error check, etc.
In hda_sync_power_state(), 
	if ((state & 0xff) == power_state)   ... should be if ((state & 0xf0)>> 4 == power_state), because PS-Act is bits 7:4
		break;

And would you please explain why changing to D3 need not wait? One of my codec does have a delay on change from D0 to D3.

Thanks
Mengdong