At Wed, 28 Nov 2007 02:19:48 -0500,
Dave Dillow wrote:
>
> While working on the locking in the sis7019 driver, and as also noted by
> Trent Piepho, there is a race between threads calling into the driver's
> hw_params callback and its prepare callback. This can lead to
> incorrect/inconsistent data being programmed into the audio device, and
> lead to unexpected results, if not a possible system crash. While it is
> arguably a bug in the user space program, we should protect the system
> and provide some assurance to the driver code about concurrency between
> these two callbacks.
>
> The attached patch adds a mutex to the two paths to provide the locking
> without giving up the ability for those calls to schedule, as documented
> in the API. It just to get a discussion going, as the code to be
> restructured a bit to clean up the error exit paths. It is also likely
> there are other callbacks that should be excluded when either of these
> are executing as well.
Thanks for raising this up.
Your fix is applicable, but I prefer rather fixing the whole locking
stuff in pcm_native.c. Due to the linked PCM streams, we have a huge
mess there, and there is even a potential deadlock.
Takashi
>
> This applies to the base kernel tree.
>
> Dave
> ---
> include/sound/pcm.h | 2 ++
> sound/core/pcm.c | 1 +
> sound/core/pcm_native.c | 20 +++++++++++++++++---
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 73334e0..989e84f 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -388,6 +388,8 @@ struct snd_pcm_substream {
> struct snd_info_entry *proc_prealloc_entry;
> struct snd_info_entry *proc_prealloc_max_entry;
> #endif
> + /* -- hw_param/prepare exclusion -- */
> + struct mutex hw_param_mutex;
> /* misc flags */
> unsigned int hw_opened: 1;
> };
> diff --git a/sound/core/pcm.c b/sound/core/pcm.c
> index 2743414..d728594 100644
> --- a/sound/core/pcm.c
> +++ b/sound/core/pcm.c
> @@ -651,6 +651,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
> list_add_tail(&substream->link_list, &substream->self_group.substreams);
> spin_lock_init(&substream->timer_lock);
> atomic_set(&substream->mmap_count, 0);
> + mutex_init(&substream->hw_param_mutex);
> prev = substream;
> }
> return 0;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 59b29cd..638d78a 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -373,6 +373,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> snd_assert(substream != NULL, return -ENXIO);
> runtime = substream->runtime;
> snd_assert(runtime != NULL, return -ENXIO);
> + mutex_lock(&substream->hw_param_mutex);
> snd_pcm_stream_lock_irq(substream);
> switch (runtime->status->state) {
> case SNDRV_PCM_STATE_OPEN:
> @@ -380,6 +381,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> case SNDRV_PCM_STATE_PREPARED:
> break;
> default:
> + mutex_unlock(&substream->hw_param_mutex);
> snd_pcm_stream_unlock_irq(substream);
> return -EBADFD;
> }
> @@ -387,8 +389,10 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> #if defined(CONFIG_SND_PCM_OSS) || defined(CONFIG_SND_PCM_OSS_MODULE)
> if (!substream->oss.oss)
> #endif
> - if (atomic_read(&substream->mmap_count))
> + if (atomic_read(&substream->mmap_count)) {
> + mutex_unlock(&substream->hw_param_mutex);
> return -EBADFD;
> + }
>
> params->rmask = ~0U;
> err = snd_pcm_hw_refine(substream, params);
> @@ -450,6 +454,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> remove_acceptable_latency(substream->latency_id);
> if ((usecs = period_to_usecs(runtime)) >= 0)
> set_acceptable_latency(substream->latency_id, usecs);
> + mutex_unlock(&substream->hw_param_mutex);
> return 0;
> _error:
> /* hardware might be unuseable from this time,
> @@ -458,6 +463,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> runtime->status->state = SNDRV_PCM_STATE_OPEN;
> if (substream->ops->hw_free != NULL)
> substream->ops->hw_free(substream);
> + mutex_unlock(&substream->hw_param_mutex);
> return err;
> }
>
> @@ -1345,9 +1351,10 @@ static struct action_ops snd_pcm_action_prepare = {
> static int snd_pcm_prepare(struct snd_pcm_substream *substream,
> struct file *file)
> {
> - int res;
> struct snd_card *card = substream->pcm->card;
> + struct snd_pcm_substream *s;
> int f_flags;
> + int res;
>
> if (file)
> f_flags = file->f_flags;
> @@ -1355,9 +1362,16 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream,
> f_flags = substream->f_flags;
>
> snd_power_lock(card);
> - if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0)
> + if ((res = snd_power_wait(card, SNDRV_CTL_POWER_D0)) >= 0) {
> + down_read(&snd_pcm_link_rwsem);
> + snd_pcm_group_for_each_entry(s, substream)
> + mutex_lock(&s->hw_param_mutex);
> res = snd_pcm_action_nonatomic(&snd_pcm_action_prepare,
> substream, f_flags);
> + snd_pcm_group_for_each_entry(s, substream)
> + mutex_unlock(&s->hw_param_mutex);
> + up_read(&snd_pcm_link_rwsem);
> + }
> snd_power_unlock(card);
> return res;
> }
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
