Re: [PATCH] ALSA: pcm: accept the OPEN state for snd_pcm_stop()

[Takashi Iwai <tiwai@suse.de>]

On Thu, 13 Jan 2022 12:31:30 +0100,
Jaroslav Kysela wrote:
> 
> It may be useful to completely invalidate streaming under some
> circumstances like the USB gadget detach. This case is a bit different
> than the complete disconnection. The applications should be notified
> that the PCM streaming is no longer available, but the recovery may be
> expected.
> 
> This patch adds support for SNDRV_PCM_STATE_OPEN passed
> to snd_pcm_stop() function. Only the hw_free ioctl is allowed to free
> the buffers in this state for a full recovery operation or the PCM file
> handle must be closed.
> 
> In the OPEN state, ioctls return EBADFD error (with the added hw_free
> exception above). The applications which are not aware about this new
> state transition (and recovery) will fail with an error. This operation
> is expected.
> 
> Link: https://lore.kernel.org/alsa-devel/4e17c99b-1d8a-8ebe-972c-9b1299952ff8@ivitera.com/
> Cc: Pavel Hofman <pavel.hofman@ivitera.com>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

I find the idea neat, but I'm afraid that it's a bit confusing from
the user POV as is.  Namely, the state is in OPEN, but you'd have to
perform hw_free manually at first for moving forward; how can user
know it?  Maybe PCM core should do hw_free by itself when hw_params is
called with hw_free_queued.

Also, do_hw_free() will skip the actual free because it's in OPEN
state, no?

In anyway, this doesn't look like a 5.17 material, so we still have
some time to stew more.


thanks,

Takashi

> ---
>  include/sound/pcm.h     |  1 +
>  sound/core/pcm.c        |  1 +
>  sound/core/pcm_native.c | 12 +++++++++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 33451f8ff755..4de1c2c91109 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -467,6 +467,7 @@ struct snd_pcm_substream {
>  	/* -- assigned files -- */
>  	int ref_count;
>  	atomic_t mmap_count;
> +	atomic_t queued_hw_free;
>  	unsigned int f_flags;
>  	void (*pcm_release)(struct snd_pcm_substream *);
>  	struct pid *pid;
> diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> index 6fd3677685d7..8dc7e99b2113 100644
> --- a/sound/core/pcm.c
> +++ b/sound/core/pcm.c
> @@ -694,6 +694,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
>  		snd_pcm_group_init(&substream->self_group);
>  		list_add_tail(&substream->link_list, &substream->self_group.substreams);
>  		atomic_set(&substream->mmap_count, 0);
> +		atomic_set(&substream->queued_hw_free, 0);
>  		prev = substream;
>  	}
>  	return 0;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 621883e71194..118ad3f26f4a 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -686,10 +686,13 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>  	snd_pcm_stream_lock_irq(substream);
>  	switch (runtime->status->state) {
>  	case SNDRV_PCM_STATE_OPEN:
> +		if (atomic_read(&substream->queued_hw_free))
> +			goto __badfd;
>  	case SNDRV_PCM_STATE_SETUP:
>  	case SNDRV_PCM_STATE_PREPARED:
>  		break;
>  	default:
> +__badfd:
>  		snd_pcm_stream_unlock_irq(substream);
>  		return -EBADFD;
>  	}
> @@ -829,6 +832,7 @@ static int do_hw_free(struct snd_pcm_substream *substream)
>  		result = substream->ops->hw_free(substream);
>  	if (substream->managed_buffer_alloc)
>  		snd_pcm_lib_free_pages(substream);
> +	atomic_set(&substream->queued_hw_free, 0);
>  	return result;
>  }
>  
> @@ -1454,6 +1458,8 @@ static void snd_pcm_post_stop(struct snd_pcm_substream *substream,
>  	}
>  	wake_up(&runtime->sleep);
>  	wake_up(&runtime->tsleep);
> +	if (state == SNDRV_PCM_STATE_OPEN)
> +		atomic_set(&substream->queued_hw_free, 1);
>  }
>  
>  static const struct action_ops snd_pcm_action_stop = {
> @@ -1469,6 +1475,9 @@ static const struct action_ops snd_pcm_action_stop = {
>   *
>   * The state of each stream is then changed to the given state unconditionally.
>   *
> + * If the requested state is OPEN, the stream is invalidated and
> + * the application must call hw_free to recover the operation.
> + *
>   * Return: Zero if successful, or a negative error code.
>   */
>  int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t state)
> @@ -2637,7 +2646,8 @@ void snd_pcm_release_substream(struct snd_pcm_substream *substream)
>  
>  	snd_pcm_drop(substream);
>  	if (substream->hw_opened) {
> -		if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN)
> +		if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN ||
> +		    atomic_read(&substream->queued_hw_free))
>  			do_hw_free(substream);
>  		substream->ops->close(substream);
>  		substream->hw_opened = 0;
> -- 
> 2.33.1
>