diff mbox series

ALSA: pcm: accept the OPEN state for snd_pcm_stop()

Message ID 20220113113130.1961332-1-perex@perex.cz (mailing list archive)
State New, archived
Headers show
Series ALSA: pcm: accept the OPEN state for snd_pcm_stop() | expand

Commit Message

Jaroslav Kysela Jan. 13, 2022, 11:31 a.m. UTC
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>
---
 include/sound/pcm.h     |  1 +
 sound/core/pcm.c        |  1 +
 sound/core/pcm_native.c | 12 +++++++++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

Comments

Takashi Iwai Jan. 13, 2022, 12:56 p.m. UTC | #1
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
>
Jaroslav Kysela Jan. 13, 2022, 1:32 p.m. UTC | #2
On 13. 01. 22 13:56, Takashi Iwai wrote:
> 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?

Thanks for the feedback.

The state ioctls are also not blocked, so the PCM state can be checked when 
EBADFD is returned for the updated applications. But as noted in the comment, 
it's expected that current applications will fail (like for the disconnect).

The OPEN state can be set only when hw_params fails in the current code. So 
the applications can distinguish the hw_params error / invalidate stream 
cases. We may also add this info (flag) to the PCM status structure.

This extension can be also implemented using a new PCM state, but in the 
regard of our discussion a few months ago, it's probably not a way.

> Maybe PCM core should do hw_free by itself when hw_params is
> called with hw_free_queued.

Yes, it's a possible optimization, too. I had same idea when I post the patch. 
But the mmap is an issue here - applications must do unmap before hw_params, 
so I'm not convinced to add this auto-free to hw_params, because hw_free has 
the straight semantics (munmap before). It would be probably clever to keep 
those steps separated.

Also ideally, there may be a check in hw_params, if parameters (buffers) are 
changed, but the implementation is not so easy. Maybe we can allow OPEN -> 
PREPARE transition for this case, so the applications may just restart the 
streaming in the most light way.

This extension can be also implemented using a new PCM state, but in the 
regard of our discussion a few months ago, it's probably not a way.

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

Yes, I forgot to add it. I'll sent v2 when we settle those little details.

					Jaroslav
Takashi Iwai Jan. 13, 2022, 1:45 p.m. UTC | #3
On Thu, 13 Jan 2022 14:32:21 +0100,
Jaroslav Kysela wrote:
> 
> On 13. 01. 22 13:56, Takashi Iwai wrote:
> > 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?
> 
> Thanks for the feedback.
> 
> The state ioctls are also not blocked, so the PCM state can be checked
> when EBADFD is returned for the updated applications. But as noted in
> the comment, it's expected that current applications will fail (like
> for the disconnect).

OK.  So this must be for a specific new use case...

> The OPEN state can be set only when hw_params fails in the current
> code. So the applications can distinguish the hw_params error /
> invalidate stream cases. We may also add this info (flag) to the PCM
> status structure.
> 
> This extension can be also implemented using a new PCM state, but in
> the regard of our discussion a few months ago, it's probably not a
> way.

Right, that'll become a compatibility headache.

> > Maybe PCM core should do hw_free by itself when hw_params is
> > called with hw_free_queued.
> 
> Yes, it's a possible optimization, too. I had same idea when I post
> the patch. But the mmap is an issue here - applications must do unmap
> before hw_params, so I'm not convinced to add this auto-free to
> hw_params, because hw_free has the straight semantics (munmap
> before). It would be probably clever to keep those steps separated.

Hm, right, mmap is messy.

> Also ideally, there may be a check in hw_params, if parameters
> (buffers) are changed, but the implementation is not so easy. Maybe we
> can allow OPEN -> 
> PREPARE transition for this case, so the applications may just restart
> the streaming in the most light way.

Hmm.  Reading more about those restrictions and requirements, I feel
that this might be better implemented in the gadget driver side
locally at first.  Basically we can handle similarly: add a new local
flag, set it at the stream stop, and return an error at prepare until
hw_params gets reconfigured.  This might be even smaller changes?


thanks,

Takashi
Jaroslav Kysela Jan. 13, 2022, 3:08 p.m. UTC | #4
On 13. 01. 22 14:45, Takashi Iwai wrote:

>> Also ideally, there may be a check in hw_params, if parameters
>> (buffers) are changed, but the implementation is not so easy. Maybe we
>> can allow OPEN ->
>> PREPARE transition for this case, so the applications may just restart
>> the streaming in the most light way.
> 
> Hmm.  Reading more about those restrictions and requirements, I feel
> that this might be better implemented in the gadget driver side
> locally at first.  Basically we can handle similarly: add a new local
> flag, set it at the stream stop, and return an error at prepare until
> hw_params gets reconfigured.  This might be even smaller changes?

Pavel reported that stop to SETUP is not enough for sox, but it's true that 
the driver may fail in the prepare() callback until the standard stream 
operation is not recovered. I think that sox is trying to recover and it 
succeeds - then the I/O timeout occurs.

Ref: 
https://lore.kernel.org/alsa-devel/9635d70f-dc12-f9ed-29f5-ce34a1d4b112@ivitera.com/

The gadget driver (drivers/usb/gadget/function/u_audio.c) has empty prepare 
callback at the moment.

Pavel, could you try to add the no-stream flag management to the gadget driver 
and return an error in the prepare callback in this case?

					Jaroslav
kernel test robot Jan. 13, 2022, 5:10 p.m. UTC | #5
Hi Jaroslav,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tiwai-sound/for-next]
[also build test WARNING on v5.16 next-20220113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-pcm-accept-the-OPEN-state-for-snd_pcm_stop/20220113-193304
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: hexagon-randconfig-r013-20220113 (https://download.01.org/0day-ci/archive/20220114/202201140042.ARpH6f9U-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d1021978b8e7e35dcc30201ca1731d64b5a602a8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/18b89b55d815ee4e4f78fa96507d2ad7a03c9c8c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jaroslav-Kysela/ALSA-pcm-accept-the-OPEN-state-for-snd_pcm_stop/20220113-193304
        git checkout 18b89b55d815ee4e4f78fa96507d2ad7a03c9c8c
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash sound/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/core/pcm_native.c:691:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
           case SNDRV_PCM_STATE_SETUP:
           ^
   sound/core/pcm_native.c:691:2: note: insert 'break;' to avoid fall-through
           case SNDRV_PCM_STATE_SETUP:
           ^
           break; 
   1 warning generated.


vim +691 sound/core/pcm_native.c

60f96aaecb19ca2 Takashi Sakamoto  2017-06-09  674  
877211f5e1b1196 Takashi Iwai      2005-11-17  675  static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
877211f5e1b1196 Takashi Iwai      2005-11-17  676  			     struct snd_pcm_hw_params *params)
^1da177e4c3f415 Linus Torvalds    2005-04-16  677  {
877211f5e1b1196 Takashi Iwai      2005-11-17  678  	struct snd_pcm_runtime *runtime;
9442e691e4aec85 Takashi Iwai      2006-09-30  679  	int err, usecs;
^1da177e4c3f415 Linus Torvalds    2005-04-16  680  	unsigned int bits;
^1da177e4c3f415 Linus Torvalds    2005-04-16  681  	snd_pcm_uframes_t frames;
^1da177e4c3f415 Linus Torvalds    2005-04-16  682  
7eaa943c8ed8e91 Takashi Iwai      2008-08-08  683  	if (PCM_RUNTIME_CHECK(substream))
7eaa943c8ed8e91 Takashi Iwai      2008-08-08  684  		return -ENXIO;
^1da177e4c3f415 Linus Torvalds    2005-04-16  685  	runtime = substream->runtime;
^1da177e4c3f415 Linus Torvalds    2005-04-16  686  	snd_pcm_stream_lock_irq(substream);
^1da177e4c3f415 Linus Torvalds    2005-04-16  687  	switch (runtime->status->state) {
^1da177e4c3f415 Linus Torvalds    2005-04-16  688  	case SNDRV_PCM_STATE_OPEN:
18b89b55d815ee4 Jaroslav Kysela   2022-01-13  689  		if (atomic_read(&substream->queued_hw_free))
18b89b55d815ee4 Jaroslav Kysela   2022-01-13  690  			goto __badfd;
^1da177e4c3f415 Linus Torvalds    2005-04-16 @691  	case SNDRV_PCM_STATE_SETUP:
^1da177e4c3f415 Linus Torvalds    2005-04-16  692  	case SNDRV_PCM_STATE_PREPARED:
^1da177e4c3f415 Linus Torvalds    2005-04-16  693  		break;
^1da177e4c3f415 Linus Torvalds    2005-04-16  694  	default:
18b89b55d815ee4 Jaroslav Kysela   2022-01-13  695  __badfd:
^1da177e4c3f415 Linus Torvalds    2005-04-16  696  		snd_pcm_stream_unlock_irq(substream);
^1da177e4c3f415 Linus Torvalds    2005-04-16  697  		return -EBADFD;
^1da177e4c3f415 Linus Torvalds    2005-04-16  698  	}
^1da177e4c3f415 Linus Torvalds    2005-04-16  699  	snd_pcm_stream_unlock_irq(substream);
8eeaa2f9e06dcfb Takashi Iwai      2014-02-10  700  #if IS_ENABLED(CONFIG_SND_PCM_OSS)
^1da177e4c3f415 Linus Torvalds    2005-04-16  701  	if (!substream->oss.oss)
^1da177e4c3f415 Linus Torvalds    2005-04-16  702  #endif
9c323fcbc51493f Takashi Iwai      2006-04-28  703  		if (atomic_read(&substream->mmap_count))
^1da177e4c3f415 Linus Torvalds    2005-04-16  704  			return -EBADFD;
^1da177e4c3f415 Linus Torvalds    2005-04-16  705  
29bb274e9497466 Takashi Iwai      2021-02-06  706  	snd_pcm_sync_stop(substream, true);
1e850beea2781d3 Takashi Iwai      2019-11-17  707  
^1da177e4c3f415 Linus Torvalds    2005-04-16  708  	params->rmask = ~0U;
^1da177e4c3f415 Linus Torvalds    2005-04-16  709  	err = snd_pcm_hw_refine(substream, params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  710  	if (err < 0)
^1da177e4c3f415 Linus Torvalds    2005-04-16  711  		goto _error;
^1da177e4c3f415 Linus Torvalds    2005-04-16  712  
^1da177e4c3f415 Linus Torvalds    2005-04-16  713  	err = snd_pcm_hw_params_choose(substream, params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  714  	if (err < 0)
^1da177e4c3f415 Linus Torvalds    2005-04-16  715  		goto _error;
^1da177e4c3f415 Linus Torvalds    2005-04-16  716  
f9a076bff053100 Takashi Sakamoto  2017-06-09  717  	err = fixup_unreferenced_params(substream, params);
f9a076bff053100 Takashi Sakamoto  2017-06-09  718  	if (err < 0)
f9a076bff053100 Takashi Sakamoto  2017-06-09  719  		goto _error;
f9a076bff053100 Takashi Sakamoto  2017-06-09  720  
0dba808eae2627f Takashi Iwai      2019-11-17  721  	if (substream->managed_buffer_alloc) {
0dba808eae2627f Takashi Iwai      2019-11-17  722  		err = snd_pcm_lib_malloc_pages(substream,
0dba808eae2627f Takashi Iwai      2019-11-17  723  					       params_buffer_bytes(params));
0dba808eae2627f Takashi Iwai      2019-11-17  724  		if (err < 0)
0dba808eae2627f Takashi Iwai      2019-11-17  725  			goto _error;
0dba808eae2627f Takashi Iwai      2019-11-17  726  		runtime->buffer_changed = err > 0;
0dba808eae2627f Takashi Iwai      2019-11-17  727  	}
0dba808eae2627f Takashi Iwai      2019-11-17  728  
^1da177e4c3f415 Linus Torvalds    2005-04-16  729  	if (substream->ops->hw_params != NULL) {
^1da177e4c3f415 Linus Torvalds    2005-04-16  730  		err = substream->ops->hw_params(substream, params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  731  		if (err < 0)
^1da177e4c3f415 Linus Torvalds    2005-04-16  732  			goto _error;
^1da177e4c3f415 Linus Torvalds    2005-04-16  733  	}
^1da177e4c3f415 Linus Torvalds    2005-04-16  734  
^1da177e4c3f415 Linus Torvalds    2005-04-16  735  	runtime->access = params_access(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  736  	runtime->format = params_format(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  737  	runtime->subformat = params_subformat(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  738  	runtime->channels = params_channels(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  739  	runtime->rate = params_rate(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  740  	runtime->period_size = params_period_size(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  741  	runtime->periods = params_periods(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  742  	runtime->buffer_size = params_buffer_size(params);
^1da177e4c3f415 Linus Torvalds    2005-04-16  743  	runtime->info = params->info;
^1da177e4c3f415 Linus Torvalds    2005-04-16  744  	runtime->rate_num = params->rate_num;
^1da177e4c3f415 Linus Torvalds    2005-04-16  745  	runtime->rate_den = params->rate_den;
ab69a4904b5dd4d Clemens Ladisch   2010-11-15  746  	runtime->no_period_wakeup =
ab69a4904b5dd4d Clemens Ladisch   2010-11-15  747  			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
ab69a4904b5dd4d Clemens Ladisch   2010-11-15  748  			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
^1da177e4c3f415 Linus Torvalds    2005-04-16  749  
^1da177e4c3f415 Linus Torvalds    2005-04-16  750  	bits = snd_pcm_format_physical_width(runtime->format);
^1da177e4c3f415 Linus Torvalds    2005-04-16  751  	runtime->sample_bits = bits;
^1da177e4c3f415 Linus Torvalds    2005-04-16  752  	bits *= runtime->channels;
^1da177e4c3f415 Linus Torvalds    2005-04-16  753  	runtime->frame_bits = bits;
^1da177e4c3f415 Linus Torvalds    2005-04-16  754  	frames = 1;
^1da177e4c3f415 Linus Torvalds    2005-04-16  755  	while (bits % 8 != 0) {
^1da177e4c3f415 Linus Torvalds    2005-04-16  756  		bits *= 2;
^1da177e4c3f415 Linus Torvalds    2005-04-16  757  		frames *= 2;
^1da177e4c3f415 Linus Torvalds    2005-04-16  758  	}
^1da177e4c3f415 Linus Torvalds    2005-04-16  759  	runtime->byte_align = bits / 8;
^1da177e4c3f415 Linus Torvalds    2005-04-16  760  	runtime->min_align = frames;
^1da177e4c3f415 Linus Torvalds    2005-04-16  761  
^1da177e4c3f415 Linus Torvalds    2005-04-16  762  	/* Default sw params */
^1da177e4c3f415 Linus Torvalds    2005-04-16  763  	runtime->tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
^1da177e4c3f415 Linus Torvalds    2005-04-16  764  	runtime->period_step = 1;
^1da177e4c3f415 Linus Torvalds    2005-04-16  765  	runtime->control->avail_min = runtime->period_size;
^1da177e4c3f415 Linus Torvalds    2005-04-16  766  	runtime->start_threshold = 1;
^1da177e4c3f415 Linus Torvalds    2005-04-16  767  	runtime->stop_threshold = runtime->buffer_size;
^1da177e4c3f415 Linus Torvalds    2005-04-16  768  	runtime->silence_threshold = 0;
^1da177e4c3f415 Linus Torvalds    2005-04-16  769  	runtime->silence_size = 0;
ead4046b2fdfd69 Clemens Ladisch   2010-05-21  770  	runtime->boundary = runtime->buffer_size;
ead4046b2fdfd69 Clemens Ladisch   2010-05-21  771  	while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
ead4046b2fdfd69 Clemens Ladisch   2010-05-21  772  		runtime->boundary *= 2;
^1da177e4c3f415 Linus Torvalds    2005-04-16  773  
add9d56d7b37815 Takashi Iwai      2019-12-11  774  	/* clear the buffer for avoiding possible kernel info leaks */
618de0f4ef11acd Takashi Iwai      2020-12-18  775  	if (runtime->dma_area && !substream->ops->copy_user) {
618de0f4ef11acd Takashi Iwai      2020-12-18  776  		size_t size = runtime->dma_bytes;
618de0f4ef11acd Takashi Iwai      2020-12-18  777  
618de0f4ef11acd Takashi Iwai      2020-12-18  778  		if (runtime->info & SNDRV_PCM_INFO_MMAP)
618de0f4ef11acd Takashi Iwai      2020-12-18  779  			size = PAGE_ALIGN(size);
618de0f4ef11acd Takashi Iwai      2020-12-18  780  		memset(runtime->dma_area, 0, size);
618de0f4ef11acd Takashi Iwai      2020-12-18  781  	}
add9d56d7b37815 Takashi Iwai      2019-12-11  782  
^1da177e4c3f415 Linus Torvalds    2005-04-16  783  	snd_pcm_timer_resolution_change(substream);
9b0573c07f278e9 Takashi Iwai      2012-10-12  784  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
9442e691e4aec85 Takashi Iwai      2006-09-30  785  
5371a79be97caac Rafael J. Wysocki 2020-02-12  786  	if (cpu_latency_qos_request_active(&substream->latency_pm_qos_req))
5371a79be97caac Rafael J. Wysocki 2020-02-12  787  		cpu_latency_qos_remove_request(&substream->latency_pm_qos_req);
137c171cf7ecf62 Takashi Iwai      2021-06-08  788  	usecs = period_to_usecs(runtime);
137c171cf7ecf62 Takashi Iwai      2021-06-08  789  	if (usecs >= 0)
5371a79be97caac Rafael J. Wysocki 2020-02-12  790  		cpu_latency_qos_add_request(&substream->latency_pm_qos_req,
5371a79be97caac Rafael J. Wysocki 2020-02-12  791  					    usecs);
^1da177e4c3f415 Linus Torvalds    2005-04-16  792  	return 0;
^1da177e4c3f415 Linus Torvalds    2005-04-16  793   _error:
25985edcedea639 Lucas De Marchi   2011-03-30  794  	/* hardware might be unusable from this time,
^1da177e4c3f415 Linus Torvalds    2005-04-16  795  	   so we force application to retry to set
^1da177e4c3f415 Linus Torvalds    2005-04-16  796  	   the correct hardware parameter settings */
9b0573c07f278e9 Takashi Iwai      2012-10-12  797  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);
^1da177e4c3f415 Linus Torvalds    2005-04-16  798  	if (substream->ops->hw_free != NULL)
^1da177e4c3f415 Linus Torvalds    2005-04-16  799  		substream->ops->hw_free(substream);
0dba808eae2627f Takashi Iwai      2019-11-17  800  	if (substream->managed_buffer_alloc)
0dba808eae2627f Takashi Iwai      2019-11-17  801  		snd_pcm_lib_free_pages(substream);
^1da177e4c3f415 Linus Torvalds    2005-04-16  802  	return err;
^1da177e4c3f415 Linus Torvalds    2005-04-16  803  }
^1da177e4c3f415 Linus Torvalds    2005-04-16  804  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Nathan Chancellor Jan. 13, 2022, 5:18 p.m. UTC | #6
On Fri, Jan 14, 2022 at 01:10:23AM +0800, kernel test robot wrote:
> Hi Jaroslav,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on tiwai-sound/for-next]
> [also build test WARNING on v5.16 next-20220113]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-pcm-accept-the-OPEN-state-for-snd_pcm_stop/20220113-193304
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
> config: hexagon-randconfig-r013-20220113 (https://download.01.org/0day-ci/archive/20220114/202201140042.ARpH6f9U-lkp@intel.com/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d1021978b8e7e35dcc30201ca1731d64b5a602a8)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/18b89b55d815ee4e4f78fa96507d2ad7a03c9c8c
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Jaroslav-Kysela/ALSA-pcm-accept-the-OPEN-state-for-snd_pcm_stop/20220113-193304
>         git checkout 18b89b55d815ee4e4f78fa96507d2ad7a03c9c8c
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash sound/core/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
> >> sound/core/pcm_native.c:691:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
>            case SNDRV_PCM_STATE_SETUP:
>            ^
>    sound/core/pcm_native.c:691:2: note: insert 'break;' to avoid fall-through
>            case SNDRV_PCM_STATE_SETUP:
>            ^
>            break; 
>    1 warning generated.
> 
> 
> vim +691 sound/core/pcm_native.c
> 
> 60f96aaecb19ca2 Takashi Sakamoto  2017-06-09  674  
> 877211f5e1b1196 Takashi Iwai      2005-11-17  675  static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> 877211f5e1b1196 Takashi Iwai      2005-11-17  676  			     struct snd_pcm_hw_params *params)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  677  {
> 877211f5e1b1196 Takashi Iwai      2005-11-17  678  	struct snd_pcm_runtime *runtime;
> 9442e691e4aec85 Takashi Iwai      2006-09-30  679  	int err, usecs;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  680  	unsigned int bits;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  681  	snd_pcm_uframes_t frames;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  682  
> 7eaa943c8ed8e91 Takashi Iwai      2008-08-08  683  	if (PCM_RUNTIME_CHECK(substream))
> 7eaa943c8ed8e91 Takashi Iwai      2008-08-08  684  		return -ENXIO;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  685  	runtime = substream->runtime;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  686  	snd_pcm_stream_lock_irq(substream);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  687  	switch (runtime->status->state) {
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  688  	case SNDRV_PCM_STATE_OPEN:
> 18b89b55d815ee4 Jaroslav Kysela   2022-01-13  689  		if (atomic_read(&substream->queued_hw_free))
> 18b89b55d815ee4 Jaroslav Kysela   2022-01-13  690  			goto __badfd;

Small explanation in case there is some confusion on clang's warning.
There should be a break here. GCC accepts implicitly falling through to
cases with just a break or a return but the kernel requires all cases to
end with either a break, fallthrough, continue, goto, or return. See
Documentation/process/deprecated.rst.

Cheers,
Nathan

> ^1da177e4c3f415 Linus Torvalds    2005-04-16 @691  	case SNDRV_PCM_STATE_SETUP:
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  692  	case SNDRV_PCM_STATE_PREPARED:
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  693  		break;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  694  	default:
> 18b89b55d815ee4 Jaroslav Kysela   2022-01-13  695  __badfd:
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  696  		snd_pcm_stream_unlock_irq(substream);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  697  		return -EBADFD;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  698  	}
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  699  	snd_pcm_stream_unlock_irq(substream);
> 8eeaa2f9e06dcfb Takashi Iwai      2014-02-10  700  #if IS_ENABLED(CONFIG_SND_PCM_OSS)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  701  	if (!substream->oss.oss)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  702  #endif
> 9c323fcbc51493f Takashi Iwai      2006-04-28  703  		if (atomic_read(&substream->mmap_count))
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  704  			return -EBADFD;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  705  
> 29bb274e9497466 Takashi Iwai      2021-02-06  706  	snd_pcm_sync_stop(substream, true);
> 1e850beea2781d3 Takashi Iwai      2019-11-17  707  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  708  	params->rmask = ~0U;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  709  	err = snd_pcm_hw_refine(substream, params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  710  	if (err < 0)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  711  		goto _error;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  712  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  713  	err = snd_pcm_hw_params_choose(substream, params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  714  	if (err < 0)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  715  		goto _error;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  716  
> f9a076bff053100 Takashi Sakamoto  2017-06-09  717  	err = fixup_unreferenced_params(substream, params);
> f9a076bff053100 Takashi Sakamoto  2017-06-09  718  	if (err < 0)
> f9a076bff053100 Takashi Sakamoto  2017-06-09  719  		goto _error;
> f9a076bff053100 Takashi Sakamoto  2017-06-09  720  
> 0dba808eae2627f Takashi Iwai      2019-11-17  721  	if (substream->managed_buffer_alloc) {
> 0dba808eae2627f Takashi Iwai      2019-11-17  722  		err = snd_pcm_lib_malloc_pages(substream,
> 0dba808eae2627f Takashi Iwai      2019-11-17  723  					       params_buffer_bytes(params));
> 0dba808eae2627f Takashi Iwai      2019-11-17  724  		if (err < 0)
> 0dba808eae2627f Takashi Iwai      2019-11-17  725  			goto _error;
> 0dba808eae2627f Takashi Iwai      2019-11-17  726  		runtime->buffer_changed = err > 0;
> 0dba808eae2627f Takashi Iwai      2019-11-17  727  	}
> 0dba808eae2627f Takashi Iwai      2019-11-17  728  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  729  	if (substream->ops->hw_params != NULL) {
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  730  		err = substream->ops->hw_params(substream, params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  731  		if (err < 0)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  732  			goto _error;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  733  	}
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  734  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  735  	runtime->access = params_access(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  736  	runtime->format = params_format(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  737  	runtime->subformat = params_subformat(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  738  	runtime->channels = params_channels(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  739  	runtime->rate = params_rate(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  740  	runtime->period_size = params_period_size(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  741  	runtime->periods = params_periods(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  742  	runtime->buffer_size = params_buffer_size(params);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  743  	runtime->info = params->info;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  744  	runtime->rate_num = params->rate_num;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  745  	runtime->rate_den = params->rate_den;
> ab69a4904b5dd4d Clemens Ladisch   2010-11-15  746  	runtime->no_period_wakeup =
> ab69a4904b5dd4d Clemens Ladisch   2010-11-15  747  			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
> ab69a4904b5dd4d Clemens Ladisch   2010-11-15  748  			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  749  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  750  	bits = snd_pcm_format_physical_width(runtime->format);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  751  	runtime->sample_bits = bits;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  752  	bits *= runtime->channels;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  753  	runtime->frame_bits = bits;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  754  	frames = 1;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  755  	while (bits % 8 != 0) {
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  756  		bits *= 2;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  757  		frames *= 2;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  758  	}
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  759  	runtime->byte_align = bits / 8;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  760  	runtime->min_align = frames;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  761  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  762  	/* Default sw params */
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  763  	runtime->tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  764  	runtime->period_step = 1;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  765  	runtime->control->avail_min = runtime->period_size;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  766  	runtime->start_threshold = 1;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  767  	runtime->stop_threshold = runtime->buffer_size;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  768  	runtime->silence_threshold = 0;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  769  	runtime->silence_size = 0;
> ead4046b2fdfd69 Clemens Ladisch   2010-05-21  770  	runtime->boundary = runtime->buffer_size;
> ead4046b2fdfd69 Clemens Ladisch   2010-05-21  771  	while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
> ead4046b2fdfd69 Clemens Ladisch   2010-05-21  772  		runtime->boundary *= 2;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  773  
> add9d56d7b37815 Takashi Iwai      2019-12-11  774  	/* clear the buffer for avoiding possible kernel info leaks */
> 618de0f4ef11acd Takashi Iwai      2020-12-18  775  	if (runtime->dma_area && !substream->ops->copy_user) {
> 618de0f4ef11acd Takashi Iwai      2020-12-18  776  		size_t size = runtime->dma_bytes;
> 618de0f4ef11acd Takashi Iwai      2020-12-18  777  
> 618de0f4ef11acd Takashi Iwai      2020-12-18  778  		if (runtime->info & SNDRV_PCM_INFO_MMAP)
> 618de0f4ef11acd Takashi Iwai      2020-12-18  779  			size = PAGE_ALIGN(size);
> 618de0f4ef11acd Takashi Iwai      2020-12-18  780  		memset(runtime->dma_area, 0, size);
> 618de0f4ef11acd Takashi Iwai      2020-12-18  781  	}
> add9d56d7b37815 Takashi Iwai      2019-12-11  782  
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  783  	snd_pcm_timer_resolution_change(substream);
> 9b0573c07f278e9 Takashi Iwai      2012-10-12  784  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);
> 9442e691e4aec85 Takashi Iwai      2006-09-30  785  
> 5371a79be97caac Rafael J. Wysocki 2020-02-12  786  	if (cpu_latency_qos_request_active(&substream->latency_pm_qos_req))
> 5371a79be97caac Rafael J. Wysocki 2020-02-12  787  		cpu_latency_qos_remove_request(&substream->latency_pm_qos_req);
> 137c171cf7ecf62 Takashi Iwai      2021-06-08  788  	usecs = period_to_usecs(runtime);
> 137c171cf7ecf62 Takashi Iwai      2021-06-08  789  	if (usecs >= 0)
> 5371a79be97caac Rafael J. Wysocki 2020-02-12  790  		cpu_latency_qos_add_request(&substream->latency_pm_qos_req,
> 5371a79be97caac Rafael J. Wysocki 2020-02-12  791  					    usecs);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  792  	return 0;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  793   _error:
> 25985edcedea639 Lucas De Marchi   2011-03-30  794  	/* hardware might be unusable from this time,
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  795  	   so we force application to retry to set
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  796  	   the correct hardware parameter settings */
> 9b0573c07f278e9 Takashi Iwai      2012-10-12  797  	snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  798  	if (substream->ops->hw_free != NULL)
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  799  		substream->ops->hw_free(substream);
> 0dba808eae2627f Takashi Iwai      2019-11-17  800  	if (substream->managed_buffer_alloc)
> 0dba808eae2627f Takashi Iwai      2019-11-17  801  		snd_pcm_lib_free_pages(substream);
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  802  	return err;
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  803  }
> ^1da177e4c3f415 Linus Torvalds    2005-04-16  804  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
Pavel Hofman Feb. 14, 2022, 12:23 p.m. UTC | #7
Dne 13. 01. 22 v 16:08 Jaroslav Kysela napsal(a):
> On 13. 01. 22 14:45, Takashi Iwai wrote:
> 
>>> Also ideally, there may be a check in hw_params, if parameters
>>> (buffers) are changed, but the implementation is not so easy. Maybe we
>>> can allow OPEN ->
>>> PREPARE transition for this case, so the applications may just restart
>>> the streaming in the most light way.
>>
>> Hmm.  Reading more about those restrictions and requirements, I feel
>> that this might be better implemented in the gadget driver side
>> locally at first.  Basically we can handle similarly: add a new local
>> flag, set it at the stream stop, and return an error at prepare until
>> hw_params gets reconfigured.  This might be even smaller changes?
> 
> Pavel reported that stop to SETUP is not enough for sox, but it's true 
> that the driver may fail in the prepare() callback until the standard 
> stream operation is not recovered. I think that sox is trying to recover 
> and it succeeds - then the I/O timeout occurs.
> 
> Ref: 
> https://lore.kernel.org/alsa-devel/9635d70f-dc12-f9ed-29f5-ce34a1d4b112@ivitera.com/ 
> 
> 
> The gadget driver (drivers/usb/gadget/function/u_audio.c) has empty 
> prepare callback at the moment.
> 
> Pavel, could you try to add the no-stream flag management to the gadget 
> driver and return an error in the prepare callback in this case?

My apology for the delay. I can make the changes to the gadget to be 
prepared for the alsa change. Let me recap to see if I understand your 
plan correctly:

* Currently the stopped stream is indicated by unsetting prm->active 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/554237f2bb62c4fcf01372e4c63d3e0062f27bac/drivers/usb/gadget/function/u_audio.c#502 
.

* A hw_params_set flag indicating that hw params have been configured 
should be set at 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/554237f2bb62c4fcf01372e4c63d3e0062f27bac/drivers/usb/gadget/function/u_audio.c#423 
. Where should it be unset? At snd_pcm_ops.stop (currently empty), at 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/554237f2bb62c4fcf01372e4c63d3e0062f27bac/drivers/usb/gadget/function/u_audio.c#494 
for active = false, or somewhere else?

* A check should be added to snd_pcm_ops.prepare which fails if the 
hw_params_set flag is unset. What error type should the fail return?

Thanks a lot for help.

Best regards,

Pavel.
diff mbox series

Patch

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;