On Mon, 19 Jun 2017 14:10:00 +0200,
Takashi Sakamoto wrote:
>
> On Jun 19 2017 02:23, Takashi Iwai wrote:
> >> I'll post a patch to take 'snd_pcm_reset()' to use
> >> 'snd_pcm_action_lock_irq()' instead of
> >> 'snd_pcm_action_nonatomic()'. How do you think about it?
> >
> > Conceptually seen, that's not right. snd_pcm_reset() calls PCM ioctl
> > ops, and this is supposed to be always non-atomic (it's a kind of
> > ioctl wrapper, after all). So the correct fix would be simply to wrap
> > only the snd_pcm_playback_silence() call with the stream lock.
>
> I once investigated for this idea. But I was discouraged because of
> several resons:
>
> 1. There's a semantic contradiction. 'snd_pcm_action_nonatomic()'
> includes atomic operation. The idea perhaps puzzles developers.
Oh no, that's a big misunderstanding. snd_pcm_action_nonatomic()
doesn't mean to prohibit the critical section protected via a spinlock
inside its execution code. Instead, it's the code you can't call
*from* spinlocked context.
> 2. In 'snd_pcm_pre_reset()', 'snd_pcm_do_reset()' and
> 'snd_pcm_post_reset()', parameters of runtime for PCM substream are
> also changed or referred. They can be changed in other functions
> concurrently, thus should be locked.
They are protected in terms of the mutex.
> 3. Currently there're a few drivers which implements the local
> ioctl. All of them handle the callback without calling task
> scheduler. It's reasonable against the cost to have an additional
> restraint just for
> context with the reset command.
But the local ioctl *is* always non-atomic, that's the reason I wrote
"conceptually seen". If you don't want it to be non-atomic, you'd
need to introduce yet another ops for doing that.
>
> Well, this is a revised version.
>
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7e8f3656b695..c74bee099155 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1593,7 +1593,11 @@ static int snd_pcm_pre_reset(struct
> snd_pcm_substream *substream, int state)
> static int snd_pcm_do_reset(struct snd_pcm_substream *substream, int
> state)
> {
> struct snd_pcm_runtime *runtime = substream->runtime;
> - int err = substream->ops->ioctl(substream,
> SNDRV_PCM_IOCTL1_RESET, NULL);
> + int err;
> +
> + snd_pcm_stream_unlock_irq(substream);
> + err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_RESET,
> NULL);
> + snd_pcm_stream_lock_irq(substream);
> if (err < 0)
> return err;
> runtime->hw_ptr_base = 0;
> @@ -1621,7 +1625,7 @@ static const struct action_ops
> snd_pcm_action_reset = {
>
> static int snd_pcm_reset(struct snd_pcm_substream *substream)
> {
> - return snd_pcm_action_nonatomic(&snd_pcm_action_reset,
> substream, 0);
> + return snd_pcm_action_lock_irq(&snd_pcm_action_reset, substream, 0);
> }
>
> /*
>
> There's still a contradiction about atomicity but as a whole better
> than before.
No, this code is just wrong... The PCM locking doesn't work like
that.
Imagine multiple PCM streams are linked. Then you'll be screwed up.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
