[PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Tzung-Bi Shih]

runtime_usage of sound card has been observed to grow without bound.
For example:
$ cat /sys/devices/platform/sound/power/runtime_usage
46
$ sox -n -t s16 -r 48000 -c 2 - synth 1 sine 440 vol 0.1 | \
  aplay -q -D hw:0,0 -f S16_LE -r 48000 -c 2
$ cat /sys/devices/platform/sound/power/runtime_usage
52

Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be
updated") stops to force update bias_level on card.  If card doesn't
provide set_bias_level callback, the snd_soc_dapm_set_bias_level()
is equivalent to NOP for card device.

As a result, dapm_pre_sequence_async() doesn't change the bias_level of
card device correctly.  Thus, pm_runtime_get_sync() would be called in
dapm_pre_sequence_async() without symmetric pm_runtime_put() in
dapm_post_sequence_async().

Don't call pm_runtime_* on card device.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 sound/soc/soc-dapm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi,
I'm not very sure if the patch makes sense.

I was considering:
(1) update the bias_level of card device correctly
(2) skip pm_runtime_* for card device

Per commit message on 4e872a46823c, I believe there is some context
I'm not aware of it.  Thus, the patch adopts (2).

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 5076299abf37..3273161e2787 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1798,7 +1798,7 @@ static void dapm_pre_sequence_async(void *data, async_cookie_t cookie)
 	/* If we're off and we're not supposed to go into STANDBY */
 	if (d->bias_level == SND_SOC_BIAS_OFF &&
 	    d->target_bias_level != SND_SOC_BIAS_OFF) {
-		if (d->dev)
+		if (d->dev && cookie)
 			pm_runtime_get_sync(d->dev);
 
 		ret = snd_soc_dapm_set_bias_level(d, SND_SOC_BIAS_STANDBY);
@@ -1845,7 +1845,7 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie)
 			dev_err(d->dev, "ASoC: Failed to turn off bias: %d\n",
 				ret);
 
-		if (d->dev)
+		if (d->dev && cookie)
 			pm_runtime_put(d->dev);
 	}
 
-- 
2.28.0.rc0.142.g3c755180ce-goog

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Fri, Jul 24, 2020 at 03:07:31PM +0800, Tzung-Bi Shih wrote:

> Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be
> updated") stops to force update bias_level on card.  If card doesn't
> provide set_bias_level callback, the snd_soc_dapm_set_bias_level()
> is equivalent to NOP for card device.

> As a result, dapm_pre_sequence_async() doesn't change the bias_level of
> card device correctly.  Thus, pm_runtime_get_sync() would be called in
> dapm_pre_sequence_async() without symmetric pm_runtime_put() in
> dapm_post_sequence_async().

> Don't call pm_runtime_* on card device.

Why is this a good fix, as opposed to only skipping the set_bias_level()
bit?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Tzung-Bi Shih]

On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Jul 24, 2020 at 03:07:31PM +0800, Tzung-Bi Shih wrote:
>
> > Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be
> > updated") stops to force update bias_level on card.  If card doesn't
> > provide set_bias_level callback, the snd_soc_dapm_set_bias_level()
> > is equivalent to NOP for card device.
>
> > As a result, dapm_pre_sequence_async() doesn't change the bias_level of
> > card device correctly.  Thus, pm_runtime_get_sync() would be called in
> > dapm_pre_sequence_async() without symmetric pm_runtime_put() in
> > dapm_post_sequence_async().
>
> > Don't call pm_runtime_* on card device.
>
> Why is this a good fix, as opposed to only skipping the set_bias_level()
> bit?

Did you mean: skip to call snd_soc_dapm_set_bias_level() in
dapm_pre_sequence_async() and dapm_post_sequence_async()?

If so, skipping snd_soc_dapm_set_bias_level() won't fix the issue.
The runtime_usage increases in pm_runtime_get() and decreases in
pm_runtime_put().

Besides, snd_soc_dapm_set_bias_level() calls card->set_bias_level()
and card->set_bias_level_post() if any.  Skip to call
snd_soc_dapm_set_bias_level() couldn't be a good idea.  It may change
some existing code behavior.

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Fri, Jul 24, 2020 at 08:26:13PM +0800, Tzung-Bi Shih wrote:
> On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote:

> > Why is this a good fix, as opposed to only skipping the set_bias_level()
> > bit?

> Did you mean: skip to call snd_soc_dapm_set_bias_level() in
> dapm_pre_sequence_async() and dapm_post_sequence_async()?

No, I mean why not just add the missing puts which are currently being
skipped due to being caught up with the bias level changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Tzung-Bi Shih]

On Fri, Jul 24, 2020 at 9:02 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jul 24, 2020 at 08:26:13PM +0800, Tzung-Bi Shih wrote:
> > On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > Why is this a good fix, as opposed to only skipping the set_bias_level()
> > > bit?
>
> > Did you mean: skip to call snd_soc_dapm_set_bias_level() in
> > dapm_pre_sequence_async() and dapm_post_sequence_async()?
>
> No, I mean why not just add the missing puts which are currently being
> skipped due to being caught up with the bias level changes.

The challenge I'm facing: (&card->dapm)->bias_level is always
SND_SOC_BIAS_OFF.  Commit 4e872a46823c stops to update it.  It has
nowhere to add the missing puts().

Ideally, if the bias_level goes away SND_SOC_BIAS_OFF in
dapm_pre_sequence_async(), it calls pm_runtime_get().  If the
bias_level goes into SND_SOC_BIAS_OFF in dapm_post_sequence_async(),
it calls pm_runtime_put().

I tried to revert commit 4e872a46823c but it seems to screw the card state up.

Would you have any suggestions?

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Mark Brown]

On Fri, 24 Jul 2020 15:07:31 +0800, Tzung-Bi Shih wrote:
> runtime_usage of sound card has been observed to grow without bound.
> For example:
> $ cat /sys/devices/platform/sound/power/runtime_usage
> 46
> $ sox -n -t s16 -r 48000 -c 2 - synth 1 sine 440 vol 0.1 | \
>   aplay -q -D hw:0,0 -f S16_LE -r 48000 -c 2
> $ cat /sys/devices/platform/sound/power/runtime_usage
> 52
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: dapm: don't call pm_runtime_* on card device
      commit: 3aecfc72d7ad73330e7a6ebd0005738a8fd417ab

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] ASoC: dapm: don't call pm_runtime_* on card device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Fri, Jul 24, 2020 at 10:41:41PM +0800, Tzung-Bi Shih wrote:

> The challenge I'm facing: (&card->dapm)->bias_level is always
> SND_SOC_BIAS_OFF.  Commit 4e872a46823c stops to update it.  It has
> nowhere to add the missing puts().

> Ideally, if the bias_level goes away SND_SOC_BIAS_OFF in
> dapm_pre_sequence_async(), it calls pm_runtime_get().  If the
> bias_level goes into SND_SOC_BIAS_OFF in dapm_post_sequence_async(),
> it calls pm_runtime_put().

Right, I see the problem now.  This is probably fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]