Merge branch 'topic/pcm-lock-refactor' into for-next

Pull PCM lock refactoring.

Signed-off-by: Takashi Iwai <tiwai@suse.de>

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index eae6d2b82d7a90ea09642599ea2757a50e404513..ca20f80f8976aacd4935de2791ac6bd2a4fefc0a 100644 (file)
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -30,6 +30,7 @@
 #include <linux/mm.h>
 #include <linux/bitops.h>
 #include <linux/pm_qos.h>
+#include <linux/refcount.h>
 
 #define snd_pcm_substream_chip(substream) ((substream)->private_data)
 #define snd_pcm_chip(pcm) ((pcm)->private_data)
@@ -439,7 +440,7 @@ struct snd_pcm_group {              /* keep linked substreams */
        spinlock_t lock;
        struct mutex mutex;
        struct list_head substreams;
-       int count;
+       refcount_t refs;
 };
 
 struct pid;

diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index bca0bdf3e33c62be75a2767b901f73df728c397a..4f45b30003472f12b033e73cf71a986dc6d9a82b 100644 (file)
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -733,9 +733,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
                        }
                }
                substream->group = &substream->self_group;
-               spin_lock_init(&substream->self_group.lock);
-               mutex_init(&substream->self_group.mutex);
-               INIT_LIST_HEAD(&substream->self_group.substreams);
+               snd_pcm_group_init(&substream->self_group);
                list_add_tail(&substream->link_list, &substream->self_group.substreams);
                atomic_set(&substream->mmap_count, 0);
                prev = substream;

diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
index c515612969a474485b2a843591a15a42605e8a0a..0b4b5dfaec181924423eff390866ab95ec8044c3 100644 (file)
--- a/sound/core/pcm_local.h
+++ b/sound/core/pcm_local.h
@@ -66,5 +66,6 @@ static inline void snd_pcm_timer_done(struct snd_pcm_substream *substream) {}
 #endif
 
 void __snd_pcm_xrun(struct snd_pcm_substream *substream);
+void snd_pcm_group_init(struct snd_pcm_group *group);
 
 #endif /* __SOUND_CORE_PCM_LOCAL_H */

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 63640d3af9db32deac7634b4e8f991df29b3c19e..0bc4aa0ac9cfeb9d8b1f543428982cd32363b37d 100644 (file)
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -85,71 +85,30 @@ static int snd_pcm_open(struct file *file, struct snd_pcm *pcm, int stream);
  *
  */
 
-static DEFINE_RWLOCK(snd_pcm_link_rwlock);
 static DECLARE_RWSEM(snd_pcm_link_rwsem);
 
-/* Writer in rwsem may block readers even during its waiting in queue,
- * and this may lead to a deadlock when the code path takes read sem
- * twice (e.g. one in snd_pcm_action_nonatomic() and another in
- * snd_pcm_stream_lock()).  As a (suboptimal) workaround, let writer to
- * sleep until all the readers are completed without blocking by writer.
- */
-static inline void down_write_nonfifo(struct rw_semaphore *lock)
+void snd_pcm_group_init(struct snd_pcm_group *group)
 {
-       while (!down_write_trylock(lock))
-               msleep(1);
+       spin_lock_init(&group->lock);
+       mutex_init(&group->mutex);
+       INIT_LIST_HEAD(&group->substreams);
+       refcount_set(&group->refs, 0);
 }
 
-#define PCM_LOCK_DEFAULT       0
-#define PCM_LOCK_IRQ   1
-#define PCM_LOCK_IRQSAVE       2
-
-static unsigned long __snd_pcm_stream_lock_mode(struct snd_pcm_substream *substream,
-                                               unsigned int mode)
-{
-       unsigned long flags = 0;
-       if (substream->pcm->nonatomic) {
-               down_read_nested(&snd_pcm_link_rwsem, SINGLE_DEPTH_NESTING);
-               mutex_lock(&substream->self_group.mutex);
-       } else {
-               switch (mode) {
-               case PCM_LOCK_DEFAULT:
-                       read_lock(&snd_pcm_link_rwlock);
-                       break;
-               case PCM_LOCK_IRQ:
-                       read_lock_irq(&snd_pcm_link_rwlock);
-                       break;
-               case PCM_LOCK_IRQSAVE:
-                       read_lock_irqsave(&snd_pcm_link_rwlock, flags);
-                       break;
-               }
-               spin_lock(&substream->self_group.lock);
-       }
-       return flags;
+/* define group lock helpers */
+#define DEFINE_PCM_GROUP_LOCK(action, mutex_action) \
+static void snd_pcm_group_ ## action(struct snd_pcm_group *group, bool nonatomic) \
+{ \
+       if (nonatomic) \
+               mutex_ ## mutex_action(&group->mutex); \
+       else \
+               spin_ ## action(&group->lock); \
 }
 
-static void __snd_pcm_stream_unlock_mode(struct snd_pcm_substream *substream,
-                                        unsigned int mode, unsigned long flags)
-{
-       if (substream->pcm->nonatomic) {
-               mutex_unlock(&substream->self_group.mutex);
-               up_read(&snd_pcm_link_rwsem);
-       } else {
-               spin_unlock(&substream->self_group.lock);
-
-               switch (mode) {
-               case PCM_LOCK_DEFAULT:
-                       read_unlock(&snd_pcm_link_rwlock);
-                       break;
-               case PCM_LOCK_IRQ:
-                       read_unlock_irq(&snd_pcm_link_rwlock);
-                       break;
-               case PCM_LOCK_IRQSAVE:
-                       read_unlock_irqrestore(&snd_pcm_link_rwlock, flags);
-                       break;
-               }
-       }
-}
+DEFINE_PCM_GROUP_LOCK(lock, lock);
+DEFINE_PCM_GROUP_LOCK(unlock, unlock);
+DEFINE_PCM_GROUP_LOCK(lock_irq, lock);
+DEFINE_PCM_GROUP_LOCK(unlock_irq, unlock);
 
 /**
  * snd_pcm_stream_lock - Lock the PCM stream
@@ -161,7 +120,7 @@ static void __snd_pcm_stream_unlock_mode(struct snd_pcm_substream *substream,
  */
 void snd_pcm_stream_lock(struct snd_pcm_substream *substream)
 {
-       __snd_pcm_stream_lock_mode(substream, PCM_LOCK_DEFAULT);
+       snd_pcm_group_lock(&substream->self_group, substream->pcm->nonatomic);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_stream_lock);
 
@@ -173,7 +132,7 @@ EXPORT_SYMBOL_GPL(snd_pcm_stream_lock);
  */
 void snd_pcm_stream_unlock(struct snd_pcm_substream *substream)
 {
-       __snd_pcm_stream_unlock_mode(substream, PCM_LOCK_DEFAULT, 0);
+       snd_pcm_group_unlock(&substream->self_group, substream->pcm->nonatomic);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_stream_unlock);
 
@@ -187,7 +146,8 @@ EXPORT_SYMBOL_GPL(snd_pcm_stream_unlock);
  */
 void snd_pcm_stream_lock_irq(struct snd_pcm_substream *substream)
 {
-       __snd_pcm_stream_lock_mode(substream, PCM_LOCK_IRQ);
+       snd_pcm_group_lock_irq(&substream->self_group,
+                              substream->pcm->nonatomic);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_stream_lock_irq);
 
@@ -199,13 +159,19 @@ EXPORT_SYMBOL_GPL(snd_pcm_stream_lock_irq);
  */
 void snd_pcm_stream_unlock_irq(struct snd_pcm_substream *substream)
 {
-       __snd_pcm_stream_unlock_mode(substream, PCM_LOCK_IRQ, 0);
+       snd_pcm_group_unlock_irq(&substream->self_group,
+                                substream->pcm->nonatomic);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_stream_unlock_irq);
 
 unsigned long _snd_pcm_stream_lock_irqsave(struct snd_pcm_substream *substream)
 {
-       return __snd_pcm_stream_lock_mode(substream, PCM_LOCK_IRQSAVE);
+       unsigned long flags = 0;
+       if (substream->pcm->nonatomic)
+               mutex_lock(&substream->self_group.mutex);
+       else
+               spin_lock_irqsave(&substream->self_group.lock, flags);
+       return flags;
 }
 EXPORT_SYMBOL_GPL(_snd_pcm_stream_lock_irqsave);
 
@@ -219,7 +185,10 @@ EXPORT_SYMBOL_GPL(_snd_pcm_stream_lock_irqsave);
 void snd_pcm_stream_unlock_irqrestore(struct snd_pcm_substream *substream,
                                      unsigned long flags)
 {
-       __snd_pcm_stream_unlock_mode(substream, PCM_LOCK_IRQSAVE, flags);
+       if (substream->pcm->nonatomic)
+               mutex_unlock(&substream->self_group.mutex);
+       else
+               spin_unlock_irqrestore(&substream->self_group.lock, flags);
 }
 EXPORT_SYMBOL_GPL(snd_pcm_stream_unlock_irqrestore);
 
@@ -1124,6 +1093,68 @@ static int snd_pcm_action_single(const struct action_ops *ops,
        return res;
 }
 
+static void snd_pcm_group_assign(struct snd_pcm_substream *substream,
+                                struct snd_pcm_group *new_group)
+{
+       substream->group = new_group;
+       list_move(&substream->link_list, &new_group->substreams);
+}
+
+/*
+ * Unref and unlock the group, but keep the stream lock;
+ * when the group becomes empty and no longer referred, destroy itself
+ */
+static void snd_pcm_group_unref(struct snd_pcm_group *group,
+                               struct snd_pcm_substream *substream)
+{
+       bool do_free;
+
+       if (!group)
+               return;
+       do_free = refcount_dec_and_test(&group->refs) &&
+               list_empty(&group->substreams);
+       snd_pcm_group_unlock(group, substream->pcm->nonatomic);
+       if (do_free)
+               kfree(group);
+}
+
+/*
+ * Lock the group inside a stream lock and reference it;
+ * return the locked group object, or NULL if not linked
+ */
+static struct snd_pcm_group *
+snd_pcm_stream_group_ref(struct snd_pcm_substream *substream)
+{
+       bool nonatomic = substream->pcm->nonatomic;
+       struct snd_pcm_group *group;
+       bool trylock;
+
+       for (;;) {
+               if (!snd_pcm_stream_linked(substream))
+                       return NULL;
+               group = substream->group;
+               /* block freeing the group object */
+               refcount_inc(&group->refs);
+
+               trylock = nonatomic ? mutex_trylock(&group->mutex) :
+                       spin_trylock(&group->lock);
+               if (trylock)
+                       break; /* OK */
+
+               /* re-lock for avoiding ABBA deadlock */
+               snd_pcm_stream_unlock(substream);
+               snd_pcm_group_lock(group, nonatomic);
+               snd_pcm_stream_lock(substream);
+
+               /* check the group again; the above opens a small race window */
+               if (substream->group == group)
+                       break; /* OK */
+               /* group changed, try again */
+               snd_pcm_group_unref(group, substream);
+       }
+       return group;
+}
+
 /*
  *  Note: call with stream lock
  */
@@ -1131,28 +1162,15 @@ static int snd_pcm_action(const struct action_ops *ops,
                          struct snd_pcm_substream *substream,
                          int state)
 {
+       struct snd_pcm_group *group;
        int res;
 
-       if (!snd_pcm_stream_linked(substream))
-               return snd_pcm_action_single(ops, substream, state);
-
-       if (substream->pcm->nonatomic) {
-               if (!mutex_trylock(&substream->group->mutex)) {
-                       mutex_unlock(&substream->self_group.mutex);
-                       mutex_lock(&substream->group->mutex);
-                       mutex_lock(&substream->self_group.mutex);
-               }
-               res = snd_pcm_action_group(ops, substream, state, 1);
-               mutex_unlock(&substream->group->mutex);
-       } else {
-               if (!spin_trylock(&substream->group->lock)) {
-                       spin_unlock(&substream->self_group.lock);
-                       spin_lock(&substream->group->lock);
-                       spin_lock(&substream->self_group.lock);
-               }
+       group = snd_pcm_stream_group_ref(substream);
+       if (group)
                res = snd_pcm_action_group(ops, substream, state, 1);
-               spin_unlock(&substream->group->lock);
-       }
+       else
+               res = snd_pcm_action_single(ops, substream, state);
+       snd_pcm_group_unref(group, substream);
        return res;
 }
 
@@ -1179,6 +1197,7 @@ static int snd_pcm_action_nonatomic(const struct action_ops *ops,
 {
        int res;
 
+       /* Guarantee the group members won't change during non-atomic action */
        down_read(&snd_pcm_link_rwsem);
        if (snd_pcm_stream_linked(substream))
                res = snd_pcm_action_group(ops, substream, state, 0);
@@ -1802,6 +1821,7 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
        struct snd_card *card;
        struct snd_pcm_runtime *runtime;
        struct snd_pcm_substream *s;
+       struct snd_pcm_group *group;
        wait_queue_entry_t wait;
        int result = 0;
        int nonblock = 0;
@@ -1818,7 +1838,6 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
        } else if (substream->f_flags & O_NONBLOCK)
                nonblock = 1;
 
-       down_read(&snd_pcm_link_rwsem);
        snd_pcm_stream_lock_irq(substream);
        /* resume pause */
        if (runtime->status->state == SNDRV_PCM_STATE_PAUSED)
@@ -1843,6 +1862,7 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
                }
                /* find a substream to drain */
                to_check = NULL;
+               group = snd_pcm_stream_group_ref(substream);
                snd_pcm_group_for_each_entry(s, substream) {
                        if (s->stream != SNDRV_PCM_STREAM_PLAYBACK)
                                continue;
@@ -1852,12 +1872,12 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
                                break;
                        }
                }
+               snd_pcm_group_unref(group, substream);
                if (!to_check)
                        break; /* all drained */
                init_waitqueue_entry(&wait, current);
                add_wait_queue(&to_check->sleep, &wait);
                snd_pcm_stream_unlock_irq(substream);
-               up_read(&snd_pcm_link_rwsem);
                if (runtime->no_period_wakeup)
                        tout = MAX_SCHEDULE_TIMEOUT;
                else {
@@ -1869,9 +1889,17 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
                        tout = msecs_to_jiffies(tout * 1000);
                }
                tout = schedule_timeout_interruptible(tout);
-               down_read(&snd_pcm_link_rwsem);
+
                snd_pcm_stream_lock_irq(substream);
-               remove_wait_queue(&to_check->sleep, &wait);
+               group = snd_pcm_stream_group_ref(substream);
+               snd_pcm_group_for_each_entry(s, substream) {
+                       if (s->runtime == to_check) {
+                               remove_wait_queue(&to_check->sleep, &wait);
+                               break;
+                       }
+               }
+               snd_pcm_group_unref(group, substream);
+
                if (card->shutdown) {
                        result = -ENODEV;
                        break;
@@ -1891,7 +1919,6 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
 
  unlock:
        snd_pcm_stream_unlock_irq(substream);
-       up_read(&snd_pcm_link_rwsem);
 
        return result;
 }
@@ -1930,13 +1957,19 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream)
 static bool is_pcm_file(struct file *file)
 {
        struct inode *inode = file_inode(file);
+       struct snd_pcm *pcm;
        unsigned int minor;
 
        if (!S_ISCHR(inode->i_mode) || imajor(inode) != snd_major)
                return false;
        minor = iminor(inode);
-       return snd_lookup_minor_data(minor, SNDRV_DEVICE_TYPE_PCM_PLAYBACK) ||
-               snd_lookup_minor_data(minor, SNDRV_DEVICE_TYPE_PCM_CAPTURE);
+       pcm = snd_lookup_minor_data(minor, SNDRV_DEVICE_TYPE_PCM_PLAYBACK);
+       if (!pcm)
+               pcm = snd_lookup_minor_data(minor, SNDRV_DEVICE_TYPE_PCM_CAPTURE);
+       if (!pcm)
+               return false;
+       snd_card_unref(pcm->card);
+       return true;
 }
 
 /*
@@ -1947,7 +1980,8 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
        int res = 0;
        struct snd_pcm_file *pcm_file;
        struct snd_pcm_substream *substream1;
-       struct snd_pcm_group *group;
+       struct snd_pcm_group *group, *target_group;
+       bool nonatomic = substream->pcm->nonatomic;
        struct fd f = fdget(fd);
 
        if (!f.file)
@@ -1958,13 +1992,14 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
        }
        pcm_file = f.file->private_data;
        substream1 = pcm_file->substream;
-       group = kmalloc(sizeof(*group), GFP_KERNEL);
+       group = kzalloc(sizeof(*group), GFP_KERNEL);
        if (!group) {
                res = -ENOMEM;
                goto _nolock;
        }
-       down_write_nonfifo(&snd_pcm_link_rwsem);
-       write_lock_irq(&snd_pcm_link_rwlock);
+       snd_pcm_group_init(group);
+
+       down_write(&snd_pcm_link_rwsem);
        if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN ||
            substream->runtime->status->state != substream1->runtime->status->state ||
            substream->pcm->nonatomic != substream1->pcm->nonatomic) {
@@ -1975,23 +2010,23 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
                res = -EALREADY;
                goto _end;
        }
+
+       snd_pcm_stream_lock_irq(substream);
        if (!snd_pcm_stream_linked(substream)) {
-               substream->group = group;
-               group = NULL;
-               spin_lock_init(&substream->group->lock);
-               mutex_init(&substream->group->mutex);
-               INIT_LIST_HEAD(&substream->group->substreams);
-               list_add_tail(&substream->link_list, &substream->group->substreams);
-               substream->group->count = 1;
-       }
-       list_add_tail(&substream1->link_list, &substream->group->substreams);
-       substream->group->count++;
-       substream1->group = substream->group;
+               snd_pcm_group_assign(substream, group);
+               group = NULL; /* assigned, don't free this one below */
+       }
+       target_group = substream->group;
+       snd_pcm_stream_unlock_irq(substream);
+
+       snd_pcm_group_lock_irq(target_group, nonatomic);
+       snd_pcm_stream_lock(substream1);
+       snd_pcm_group_assign(substream1, target_group);
+       snd_pcm_stream_unlock(substream1);
+       snd_pcm_group_unlock_irq(target_group, nonatomic);
  _end:
-       write_unlock_irq(&snd_pcm_link_rwlock);
        up_write(&snd_pcm_link_rwsem);
  _nolock:
-       snd_card_unref(substream1->pcm->card);
        kfree(group);
  _badf:
        fdput(f);
@@ -2000,34 +2035,43 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 
 static void relink_to_local(struct snd_pcm_substream *substream)
 {
-       substream->group = &substream->self_group;
-       INIT_LIST_HEAD(&substream->self_group.substreams);
-       list_add_tail(&substream->link_list, &substream->self_group.substreams);
+       snd_pcm_stream_lock(substream);
+       snd_pcm_group_assign(substream, &substream->self_group);
+       snd_pcm_stream_unlock(substream);
 }
 
 static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 {
-       struct snd_pcm_substream *s;
+       struct snd_pcm_group *group;
+       bool nonatomic = substream->pcm->nonatomic;
+       bool do_free = false;
        int res = 0;
 
-       down_write_nonfifo(&snd_pcm_link_rwsem);
-       write_lock_irq(&snd_pcm_link_rwlock);
+       down_write(&snd_pcm_link_rwsem);
+
        if (!snd_pcm_stream_linked(substream)) {
                res = -EALREADY;
                goto _end;
        }
-       list_del(&substream->link_list);
-       substream->group->count--;
-       if (substream->group->count == 1) {     /* detach the last stream, too */
-               snd_pcm_group_for_each_entry(s, substream) {
-                       relink_to_local(s);
-                       break;
-               }
-               kfree(substream->group);
-       }
+
+       group = substream->group;
+       snd_pcm_group_lock_irq(group, nonatomic);
+
        relink_to_local(substream);
+
+       /* detach the last stream, too */
+       if (list_is_singular(&group->substreams)) {
+               relink_to_local(list_first_entry(&group->substreams,
+                                                struct snd_pcm_substream,
+                                                link_list));
+               do_free = !refcount_read(&group->refs);
+       }
+
+       snd_pcm_group_unlock_irq(group, nonatomic);
+       if (do_free)
+               kfree(group);
+
        _end:
-       write_unlock_irq(&snd_pcm_link_rwlock);
        up_write(&snd_pcm_link_rwsem);
        return res;
 }