[PATCH 0/7] ALSA: Refactoring PCM locking code

[PATCH 0/7] ALSA: Refactoring PCM locking code

[Takashi Iwai]

Hi,

this is a patch set to refactor the PCM locking code and to reduce the
global lock.  At least, this gets rid of the global rwlock, while the
global rwsem is still needed for some non-atomic ops and for avoiding
the deadlock at linking.


Takashi

===

Takashi Iwai (7):
  ALSA: pcm: Call snd_card_unref() inside in_pcm_file()
  ALSA: pcm: Unify snd_pcm_group initialization
  ALSA: pcm: Make PCM linked list consistent while re-grouping
  ALSA: pcm: Avoid confusing loop in snd_pcm_unlink()
  ALSA: pcm: More fine-grained PCM link locking
  ALSA: pcm: Remove down_write() hack for snd_pcm_link_rwsem
  ALSA: pcm: Cleanup snd_pcm_stream_lock() & co

 include/sound/pcm.h     |   3 +-
 sound/core/pcm.c        |   4 +-
 sound/core/pcm_local.h  |   1 +
 sound/core/pcm_native.c | 290 ++++++++++++++++++++++++++++--------------------
 4 files changed, 171 insertions(+), 127 deletions(-)

-- 
2.16.4

[PATCH 1/7] ALSA: pcm: Call snd_card_unref() inside in_pcm_file()

[Takashi Iwai]

The snd_card_unref() call in snd_pcm_link() looks suspicious through a
quick glance, but it's a correct usage; this is needed just because
the file descriptor check in is_pcm_file() calls the helper
snd_lookup_minor_data() that keeps the card refcount.

Despite of the correctness, the code still looks confusing.
Basically, keeping the card ref for the whole code isn't needed
as fdget() blocks the release of the opened file.  Hence it's more
understandable if snd_card_unref() is moved into is_pcm_file(), then
the caller doesn't have to take care after the call.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 818dff1de545..c72dfd1fc1ed 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1935,13 +1935,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;
 }
 
 /*
@@ -1996,7 +2002,6 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 	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);
-- 
2.16.4

[PATCH 2/7] ALSA: pcm: Unify snd_pcm_group initialization

[Takashi Iwai]

There are multiple open codes that initialize the same object.
Create a common helper function instead.

Also, use kzalloc() to be safer at creating a group object, and move
the initialization out of the critical section.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm.c        |  4 +---
 sound/core/pcm_local.h  |  1 +
 sound/core/pcm_native.c | 13 +++++++++----
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 01b9d62eef14..88a2998f4f9b 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -753,9 +753,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 c515612969a4..0b4b5dfaec18 100644
--- 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 c72dfd1fc1ed..9e4e289e5703 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -100,6 +100,13 @@ static inline void down_write_nonfifo(struct rw_semaphore *lock)
 		msleep(1);
 }
 
+void snd_pcm_group_init(struct snd_pcm_group *group)
+{
+	spin_lock_init(&group->lock);
+	mutex_init(&group->mutex);
+	INIT_LIST_HEAD(&group->substreams);
+}
+
 #define PCM_LOCK_DEFAULT	0
 #define PCM_LOCK_IRQ	1
 #define PCM_LOCK_IRQSAVE	2
@@ -1969,11 +1976,12 @@ 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;
 	}
+	snd_pcm_group_init(group);
 	down_write_nonfifo(&snd_pcm_link_rwsem);
 	write_lock_irq(&snd_pcm_link_rwlock);
 	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN ||
@@ -1989,9 +1997,6 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 	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;
 	}
-- 
2.16.4

[PATCH 3/7] ALSA: pcm: Make PCM linked list consistent while re-grouping

[Takashi Iwai]

Make a common helper to re-assign the PCM link using list_move() instead
of open code with manual list_del() and list_add_tail().  This assures
the consistency and we can get rid of snd_pcm_group.count field -- its
purpose is only to check whether the list is singular, and we can know
it by list_is_singular() call now.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h     |  1 -
 sound/core/pcm_native.c | 34 ++++++++++++++++++++--------------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index d6bd3caf6878..e1c747c70883 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -439,7 +439,6 @@ struct snd_pcm_group {		/* keep linked substreams */
 	spinlock_t lock;
 	struct mutex mutex;
 	struct list_head substreams;
-	int count;
 };
 
 struct pid;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 9e4e289e5703..1a56bb1ad780 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1131,6 +1131,13 @@ 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);
+}
+
 /*
  *  Note: call with stream lock
  */
@@ -1995,14 +2002,10 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 		goto _end;
 	}
 	if (!snd_pcm_stream_linked(substream)) {
-		substream->group = group;
+		snd_pcm_group_assign(substream, group);
 		group = NULL;
-		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(substream1, substream->group);
  _end:
 	write_unlock_irq(&snd_pcm_link_rwlock);
 	up_write(&snd_pcm_link_rwsem);
@@ -2015,14 +2018,13 @@ 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_group_assign(substream, &substream->self_group);
 }
 
 static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_substream *s;
+	struct snd_pcm_group *group;
 	int res = 0;
 
 	down_write_nonfifo(&snd_pcm_link_rwsem);
@@ -2031,16 +2033,20 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 		res = -EALREADY;
 		goto _end;
 	}
-	list_del(&substream->link_list);
-	substream->group->count--;
-	if (substream->group->count == 1) {	/* detach the last stream, too */
+
+	group = substream->group;
+
+	relink_to_local(substream);
+
+	/* detach the last stream, too */
+	if (list_is_singular(&group->substreams)) {
 		snd_pcm_group_for_each_entry(s, substream) {
 			relink_to_local(s);
 			break;
 		}
-		kfree(substream->group);
+		kfree(group);
 	}
-	relink_to_local(substream);
+
        _end:
 	write_unlock_irq(&snd_pcm_link_rwlock);
 	up_write(&snd_pcm_link_rwsem);
-- 
2.16.4

[PATCH 4/7] ALSA: pcm: Avoid confusing loop in snd_pcm_unlink()

[Takashi Iwai]

The snd_pcm_group_for_each_entry() loop found in snd_pcm_unlink() is
only for taking the first list entry.  Use list_first_entry() to make
clearer.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 1a56bb1ad780..fb45386270d5 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2023,7 +2023,6 @@ static void relink_to_local(struct snd_pcm_substream *substream)
 
 static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 {
-	struct snd_pcm_substream *s;
 	struct snd_pcm_group *group;
 	int res = 0;
 
@@ -2040,10 +2039,9 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 
 	/* detach the last stream, too */
 	if (list_is_singular(&group->substreams)) {
-		snd_pcm_group_for_each_entry(s, substream) {
-			relink_to_local(s);
-			break;
-		}
+		relink_to_local(list_first_entry(&group->substreams,
+						 struct snd_pcm_substream,
+						 link_list));
 		kfree(group);
 	}
 
-- 
2.16.4

[PATCH 5/7] ALSA: pcm: More fine-grained PCM link locking

[Takashi Iwai]

We have currently two global locks, a rwlock and a rwsem, that are
used for managing linking the PCM streams.  Due to these global locks,
once when a linked stream is used, the lock granularity suffers a
lot.

This patch attempts to eliminate the former global lock for atomic
ops.  The latter rwsem needs remaining because of the loosy way of the
loop calls in snd_pcm_action_nonatomic(), as well as for avoiding the
deadlock at linking.  However, these are used far rarely, actually
only by two actions (prepare and  reset), where both are no timing
critical ones.  So this can be still seen as a good improvement.

The basic strategy to eliminate the rwlock is to assure group->lock at
linking and unlinking.  Since we already takes the group lock whenever
taking the all substream locks under the group, this shouldn't be a
big problem.  But there is one pitfall: snd_pcm_action() does
re-locking for avoiding ABBA deadlocks, and this opens a small race
window for dereferencing the substream group object.  This includes
the unlink of group during that window, too.

For addressing these corner cases, two new tricks are introduced:
- After re-locking, the group assigned to the stream is checked again;
  if the group is changed, we retry the whole procedure.
- Introduce a refcount to snd_pcm_group object, so that it's freed
  only when it's empty and really no one refers to it.

Along with these changes, there are a significant amount of code
refactoring.  The complex group re-lock & ref code is factored out to
snd_pcm_stream_group_ref() function, for example.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/pcm.h     |   2 +
 sound/core/pcm_native.c | 166 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 124 insertions(+), 44 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index e1c747c70883..3bde24575a99 100644
--- 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,6 +440,7 @@ struct snd_pcm_group {		/* keep linked substreams */
 	spinlock_t lock;
 	struct mutex mutex;
 	struct list_head substreams;
+	refcount_t refs;
 };
 
 struct pid;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index fb45386270d5..cbde23fc67a9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -85,7 +85,6 @@ 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,
@@ -105,8 +104,24 @@ void snd_pcm_group_init(struct snd_pcm_group *group)
 	spin_lock_init(&group->lock);
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->substreams);
+	refcount_set(&group->refs, 0);
 }
 
+/* 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); \
+}
+
+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);
+
 #define PCM_LOCK_DEFAULT	0
 #define PCM_LOCK_IRQ	1
 #define PCM_LOCK_IRQSAVE	2
@@ -116,21 +131,19 @@ static unsigned long __snd_pcm_stream_lock_mode(struct snd_pcm_substream *substr
 {
 	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);
+			spin_lock(&substream->self_group.lock);
 			break;
 		case PCM_LOCK_IRQ:
-			read_lock_irq(&snd_pcm_link_rwlock);
+			spin_lock_irq(&substream->self_group.lock);
 			break;
 		case PCM_LOCK_IRQSAVE:
-			read_lock_irqsave(&snd_pcm_link_rwlock, flags);
+			spin_lock_irqsave(&substream->self_group.lock, flags);
 			break;
 		}
-		spin_lock(&substream->self_group.lock);
 	}
 	return flags;
 }
@@ -140,19 +153,16 @@ static void __snd_pcm_stream_unlock_mode(struct snd_pcm_substream *substream,
 {
 	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);
+			spin_unlock(&substream->self_group.lock);
 			break;
 		case PCM_LOCK_IRQ:
-			read_unlock_irq(&snd_pcm_link_rwlock);
+			spin_unlock_irq(&substream->self_group.lock);
 			break;
 		case PCM_LOCK_IRQSAVE:
-			read_unlock_irqrestore(&snd_pcm_link_rwlock, flags);
+			spin_unlock_irqrestore(&substream->self_group.lock, flags);
 			break;
 		}
 	}
@@ -1138,6 +1148,61 @@ static void snd_pcm_group_assign(struct snd_pcm_substream *substream,
 	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
  */
@@ -1145,28 +1210,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);
-		}
+	group = snd_pcm_stream_group_ref(substream);
+	if (group)
 		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);
-		}
-		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;
 }
 
@@ -1193,6 +1245,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);
@@ -1821,6 +1874,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;
@@ -1837,7 +1891,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)
@@ -1862,6 +1915,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;
@@ -1871,12 +1925,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 {
@@ -1888,9 +1942,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;
@@ -1910,7 +1972,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;
 }
@@ -1972,7 +2033,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)
@@ -1989,8 +2051,8 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 		goto _nolock;
 	}
 	snd_pcm_group_init(group);
+
 	down_write_nonfifo(&snd_pcm_link_rwsem);
-	write_lock_irq(&snd_pcm_link_rwlock);
 	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN ||
 	    substream->runtime->status->state != substream1->runtime->status->state ||
 	    substream->pcm->nonatomic != substream1->pcm->nonatomic) {
@@ -2001,13 +2063,21 @@ 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)) {
 		snd_pcm_group_assign(substream, group);
-		group = NULL;
+		group = NULL; /* assigned, don't free this one below */
 	}
-	snd_pcm_group_assign(substream1, substream->group);
+	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:
 	kfree(group);
@@ -2018,22 +2088,27 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 
 static void relink_to_local(struct snd_pcm_substream *substream)
 {
+	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_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);
+
 	if (!snd_pcm_stream_linked(substream)) {
 		res = -EALREADY;
 		goto _end;
 	}
 
 	group = substream->group;
+	snd_pcm_group_lock_irq(group, nonatomic);
 
 	relink_to_local(substream);
 
@@ -2042,11 +2117,14 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 		relink_to_local(list_first_entry(&group->substreams,
 						 struct snd_pcm_substream,
 						 link_list));
-		kfree(group);
+		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;
 }
-- 
2.16.4

[PATCH 6/7] ALSA: pcm: Remove down_write() hack for snd_pcm_link_rwsem

[Takashi Iwai]

Remove the hackish down_write_nonfifo() that was introduced as a
workaround of rwsem deadlock.

It used to be a problem for non-atomic PCM streams that take the rwsem
for the locking and hit the high lock contention.  Since the current
PCM locking refactoring, we'll no longer hit it as the hot code-paths
don't take global locks.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index cbde23fc67a9..f450083eb073 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -87,18 +87,6 @@ static int snd_pcm_open(struct file *file, struct snd_pcm *pcm, int stream);
 
 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)
-{
-	while (!down_write_trylock(lock))
-		msleep(1);
-}
-
 void snd_pcm_group_init(struct snd_pcm_group *group)
 {
 	spin_lock_init(&group->lock);
@@ -2052,7 +2040,7 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd)
 	}
 	snd_pcm_group_init(group);
 
-	down_write_nonfifo(&snd_pcm_link_rwsem);
+	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) {
@@ -2100,7 +2088,7 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream)
 	bool do_free = false;
 	int res = 0;
 
-	down_write_nonfifo(&snd_pcm_link_rwsem);
+	down_write(&snd_pcm_link_rwsem);
 
 	if (!snd_pcm_stream_linked(substream)) {
 		res = -EALREADY;
-- 
2.16.4

[PATCH 7/7] ALSA: pcm: Cleanup snd_pcm_stream_lock() & co

[Takashi Iwai]

After the previous code refactoring, the PCM stream locking code
became nothing but the PCM group lock with self_group object.  Use the
existing helper function for simplifying the code.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 68 ++++++++++++-------------------------------------
 1 file changed, 16 insertions(+), 52 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f450083eb073..024e32acbc25 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -110,52 +110,6 @@ DEFINE_PCM_GROUP_LOCK(unlock, unlock);
 DEFINE_PCM_GROUP_LOCK(lock_irq, lock);
 DEFINE_PCM_GROUP_LOCK(unlock_irq, unlock);
 
-#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) {
-		mutex_lock(&substream->self_group.mutex);
-	} else {
-		switch (mode) {
-		case PCM_LOCK_DEFAULT:
-			spin_lock(&substream->self_group.lock);
-			break;
-		case PCM_LOCK_IRQ:
-			spin_lock_irq(&substream->self_group.lock);
-			break;
-		case PCM_LOCK_IRQSAVE:
-			spin_lock_irqsave(&substream->self_group.lock, flags);
-			break;
-		}
-	}
-	return flags;
-}
-
-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);
-	} else {
-		switch (mode) {
-		case PCM_LOCK_DEFAULT:
-			spin_unlock(&substream->self_group.lock);
-			break;
-		case PCM_LOCK_IRQ:
-			spin_unlock_irq(&substream->self_group.lock);
-			break;
-		case PCM_LOCK_IRQSAVE:
-			spin_unlock_irqrestore(&substream->self_group.lock, flags);
-			break;
-		}
-	}
-}
-
 /**
  * snd_pcm_stream_lock - Lock the PCM stream
  * @substream: PCM substream
@@ -166,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);
 
@@ -178,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);
 
@@ -192,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);
 
@@ -204,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);
 
@@ -224,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);
 
-- 
2.16.4