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@xxxxxxxxxxx/ > Cc: Pavel Hofman <pavel.hofman@xxxxxxxxxxx> > Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx> 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 >
