Re: [PATCH] ASoC: samsung: i2s: Fix runtime PM usage count leak on failed trigger

[Xiangwu Lai]

Hi Mark,

Thanks for the review.

> This mixes scoped_guard and goto, that is prone to breakage.

Understood.  I reached for the goto because pm_runtime_put() may sleep
and therefore cannot run inside the spinlock guard, but jumping out of
the scoped_guard() body is not the way to get there.  If anything from
this patch survives review, the locked section needs to be factored
out into a helper so that the error handling lives outside the guard
scope.

> If the subsream is not linked then the core will unwind th start which
> does actually do the trigger() the commit message said won't happen...

You are right, and this is the bigger problem with the patch.  I went
back through the trigger paths and the commit message claim that
neither core issues a compensating TRIGGER_STOP is wrong for the
non-linked case, which is the normal case:

  - snd_pcm_action_single() calls ops->undo_action() when do_action()
    fails, and snd_pcm_undo_start() then sends TRIGGER_STOP to the
    substream whose trigger failed, since runtime->trigger_master is
    set by snd_pcm_pre_start().

  - The ASoC level rollback does skip the failed DAI:
    snd_soc_pcm_dai_trigger() with rollback == 1 only stops DAIs that
    carry a trigger mark, and the failed one never got marked.  But
    the PCM core undo then calls soc_pcm_trigger() with rollback == 0,
    which runs the DAI trigger unconditionally.

So on current mainline a failed START on a non-linked substream is
followed by TRIGGER_STOP, and the pm_runtime_put() in the STOP path
already balances the get_sync().  This has been the behavior at least
since v4.19.  With the patch applied the driver would put as well, so
a failed start on the common paths would call pm_runtime_put() twice
against a single get_sync() and push the usage count below where it
started.  In other words, the patch trades a non-existent leak for a
real underflow.

The compensating TRIGGER_STOP is only missing when the substream is
part of a linked group: snd_pcm_action_group() only undoes the group
members that were started before the failing one, and the failing
substream itself never sees the STOP.  That is the only case where the
usage count really does leak.

Since the two action paths disagree on whether the failed stream gets
undone, this looks like something to sort out in the PCM core rather
than in individual DAI drivers.  Making the group undo cover the
failed substream as well would fix the linked case for every driver at
once and would spare drivers from having to guess whether the core
will unwind for them.

Unless you disagree I will drop this patch.  If you think the linked
case is worth chasing I can prepare an RFC for sound/core/pcm_native.c
(Cc Takashi) instead, so let me know which way you prefer.

Thanks,
Xiangwu Lai