Re: [PATCH] ALSA: dummy: Defer hrtimer data freeing until runtime detach

[Cen Zhang]

Hi Takashi,

>
> Hmm, it's not clear how this happens.  It looks like a typical
> obscuration by AI reports...
>
> Basically dummy_hrtimer_sync_stop should handled the hrtimer cancel,
> but maybe the problem is rather its lack at releasing?  If so, the
> patch below should cover instead.
>
>
> thanks,
>
> Takashi

Thank you for looking at this. My commit message did not explain the
scenario clearly. Sorry for the confusion.

The observed failure involves a PREPARE ioctl on a surviving linked
substream. The test opens two dummy capture substreams, A and B, with
the hrtimer backend selected, configures both with HW_PARAMS, and links
them with LINK. It then performs the final close of A while issuing
PREPARE on B. It does not issue START.

On the current for-next source, the close path is:

  snd_pcm_release_substream(A)
    snd_pcm_drop(A)
    do_hw_free(A)
      snd_pcm_sync_stop(A, true)
    dummy_pcm_close(A)
      dummy_hrtimer_free(A)
        kfree(dpcm)
    pcm_release_private(A)
      snd_pcm_unlink(A)
    snd_pcm_detach_substream(A)

So the close path already synchronizes the timer before freeing dpcm.
The gap is between driver close and unlink. snd_pcm_drop() leaves the
linked group in SETUP. The release path calls do_hw_free() directly;
unlike the HW_FREE ioctl wrapper, that helper does not change the
runtime state to OPEN. A therefore remains linked and in a state that
snd_pcm_pre_prepare() accepts until the later unlink.

The relevant ordering is:

  Target A final close              PREPARE on linked peer B
  --------------------              ------------------------
  snd_pcm_drop(): group -> SETUP
  do_hw_free(): synchronize timer
  dummy_pcm_close(): free dpcm
                                    Take link read lock
                                    Walk group, including A
                                    Accept A's SETUP state
                                    dummy_pcm_prepare(A)
                                      Read freed timer_ops
  pcm_release_private()
    snd_pcm_unlink(): link write lock

PREPARE holds the link read lock and the participating runtime buffer
mutexes. Driver close does not hold those locks across the free. Timer
cancellation waits for timer callbacks, but does not wait for this
separate PREPARE action. The later unlink is what prevents another
group action from visiting A.

>
> -- 8< --
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2301,6 +2301,8 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream)
>         result = snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
>         /* runtime->control->appl_ptr = runtime->status->hw_ptr; */
>
> +       snd_pcm_sync_stop(substream, true);
> +
>         return result;
>  }
>

Synchronizing a pending hrtimer callback before release is sensible.
For the linked PREPARE ordering above, though, I think an additional
lifetime step may still be needed. PREPARE on B can start after the
extra sync_stop() has completed and after A's close has freed dpcm,
while A is still linked. It can then reach A's prepare callback as
shown above. Waiting for timer callbacks alone does not exclude that
later group action.

Best regards,
Cen Zhang