[PATCH] ALSA: pcm: accept the OPEN state for snd_pcm_stop()
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@ivit... 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(-)
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;
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@ivit... 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);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;+__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);-- 2.33.1
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@ivit... 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
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@ivit... 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
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@ivit...
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
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@ivit...
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/+/554237... .
* 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/+/554237... . Where should it be unset? At snd_pcm_ops.stop (currently empty), at https://kernel.googlesource.com/pub/scm/linux/kernel/git/gregkh/usb/+/554237... 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.
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... 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@i...) 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/18b89b55d815ee4e4f78fa96507d2ad7a03c... 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
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... 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@i...) 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/18b89b55d815ee4e4f78fa96507d2ad7a03c... 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
participants (5)
-
Jaroslav Kysela -
kernel test robot -
Nathan Chancellor -
Pavel Hofman -
Takashi Iwai