Re: [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Hi Clemens,

On Nov 17 2014 06:21, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> +++ b/sound/firewire/oxfw/oxfw-stream.c
>> +int snd_oxfw_stream_start_simplex(struct snd_oxfw *oxfw,
>> +				  struct amdtp_stream *stream,
>> +				  unsigned int rate, unsigned int pcm_channels)
>> +{
>> ...
>> +	if (atomic_read(substreams) == 0)
>> +		goto end;
>> +
>>  	mutex_lock(&oxfw->mutex);
> 
> What happens if hw_free is called between the calls to atomic_read() and
> mutex_lock()?

In the worst case, the codes after the mutex_lock() manage to start
duplex streams with released resources, then it causes kernel panic.

This is a bug. I should have move atmic_read() to the critical section...

> And why are the substreams counters atomic?
> Can't they just be normal variables accessed from inside the mutex?

Just for my convinience. I didn't want to write many
mutex_lock()/mutex_unlock() because wrong coding of them causes-dead
lock, then push them inner stream.c. This idea is good except for
reference couters, so I added atomic_t.

An attached patch achieves your idea over my patchset. Handling
reference counter is easy to understand (because it's arithmetric
operation) but I don't like much mutex_lock()/mutex_unlock().

Can I request you to explain about the advantages of your idea?


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-dice-use-mutex-only-instead-of-atomic_t-to-handle-cr.patch --]
[-- Type: text/x-diff; name="0001-dice-use-mutex-only-instead-of-atomic_t-to-handle-cr.patch", Size: 9361 bytes --]

From 3e9968a4063f7b5d2977970821f34d5df3b70c57 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Thu, 20 Nov 2014 10:28:08 +0900
Subject: [PATCH] dice: use mutex only instead of atomic_t to handle critical
 section easily

Dice driver has some critical sections. Some of them are protected by atomic_t
and the others are protected by mutex. But they can be merged.

Originally, I added atomic_t for a reference counter to enclose
mutex_lock()/mutex_unlock() inner -stream.c But it's cumbersome that this
driver uses some mutual primitives for stream handling.

This commit obsoletes atomic_t and use mutex only.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/dice/dice-midi.c   | 16 ++++++++++++----
 sound/firewire/dice/dice-pcm.c    | 30 ++++++++++++++++++++++++------
 sound/firewire/dice/dice-stream.c | 29 +++++++----------------------
 sound/firewire/dice/dice.c        |  4 ++++
 sound/firewire/dice/dice.h        |  2 +-
 5 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/sound/firewire/dice/dice-midi.c b/sound/firewire/dice/dice-midi.c
index 4f4eae7..74cd3b3 100644
--- a/sound/firewire/dice/dice-midi.c
+++ b/sound/firewire/dice/dice-midi.c
@@ -16,8 +16,10 @@ static int capture_open(struct snd_rawmidi_substream *substream)
 	if (err < 0)
 		goto end;
 
-	atomic_inc(&dice->substreams_counter);
+	mutex_lock(&dice->mutex);
+	dice->substreams_counter++;
 	err = snd_dice_stream_start_duplex(dice, 0);
+	mutex_unlock(&dice->mutex);
 	if (err < 0)
 		snd_dice_stream_lock_release(dice);
 end:
@@ -33,8 +35,10 @@ static int playback_open(struct snd_rawmidi_substream *substream)
 	if (err < 0)
 		goto end;
 
-	atomic_inc(&dice->substreams_counter);
+	mutex_lock(&dice->mutex);
+	dice->substreams_counter++;
 	err = snd_dice_stream_start_duplex(dice, 0);
+	mutex_unlock(&dice->mutex);
 	if (err < 0)
 		snd_dice_stream_lock_release(dice);
 end:
@@ -45,8 +49,10 @@ static int capture_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_dice *dice = substream->rmidi->private_data;
 
-	atomic_dec(&dice->substreams_counter);
+	mutex_lock(&dice->mutex);
+	dice->substreams_counter--;
 	snd_dice_stream_stop_duplex(dice);
+	mutex_unlock(&dice->mutex);
 
 	snd_dice_stream_lock_release(dice);
 	return 0;
@@ -56,8 +62,10 @@ static int playback_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_dice *dice = substream->rmidi->private_data;
 
-	atomic_dec(&dice->substreams_counter);
+	mutex_lock(&dice->mutex);
+	dice->substreams_counter--;
 	snd_dice_stream_stop_duplex(dice);
+	mutex_unlock(&dice->mutex);
 
 	snd_dice_stream_lock_release(dice);
 	return 0;
diff --git a/sound/firewire/dice/dice-pcm.c b/sound/firewire/dice/dice-pcm.c
index 3427a21..f777145 100644
--- a/sound/firewire/dice/dice-pcm.c
+++ b/sound/firewire/dice/dice-pcm.c
@@ -231,8 +231,11 @@ static int capture_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_dice *dice = substream->private_data;
 
-	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN)
-		atomic_inc(&dice->substreams_counter);
+	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&dice->mutex);
+		dice->substreams_counter++;
+		mutex_unlock(&dice->mutex);
+	}
 
 	amdtp_stream_set_pcm_format(&dice->tx_stream,
 				    params_format(hw_params));
@@ -245,8 +248,11 @@ static int playback_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_dice *dice = substream->private_data;
 
-	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN)
-		atomic_inc(&dice->substreams_counter);
+	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&dice->mutex);
+		dice->substreams_counter++;
+		mutex_unlock(&dice->mutex);
+	}
 
 	amdtp_stream_set_pcm_format(&dice->rx_stream,
 				    params_format(hw_params));
@@ -259,11 +265,15 @@ static int capture_hw_free(struct snd_pcm_substream *substream)
 {
 	struct snd_dice *dice = substream->private_data;
 
+	mutex_lock(&dice->mutex);
+
 	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN)
-		atomic_dec(&dice->substreams_counter);
+		dice->substreams_counter--;
 
 	snd_dice_stream_stop_duplex(dice);
 
+	mutex_unlock(&dice->mutex);
+
 	return snd_pcm_lib_free_vmalloc_buffer(substream);
 }
 
@@ -271,11 +281,15 @@ static int playback_hw_free(struct snd_pcm_substream *substream)
 {
 	struct snd_dice *dice = substream->private_data;
 
+	mutex_lock(&dice->mutex);
+
 	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN)
-		atomic_dec(&dice->substreams_counter);
+		dice->substreams_counter--;
 
 	snd_dice_stream_stop_duplex(dice);
 
+	mutex_unlock(&dice->mutex);
+
 	return snd_pcm_lib_free_vmalloc_buffer(substream);
 }
 
@@ -284,7 +298,9 @@ static int capture_prepare(struct snd_pcm_substream *substream)
 	struct snd_dice *dice = substream->private_data;
 	int err;
 
+	mutex_lock(&dice->mutex);
 	err = snd_dice_stream_start_duplex(dice, substream->runtime->rate);
+	mutex_unlock(&dice->mutex);
 	if (err >= 0)
 		amdtp_stream_pcm_prepare(&dice->tx_stream);
 
@@ -295,7 +311,9 @@ static int playback_prepare(struct snd_pcm_substream *substream)
 	struct snd_dice *dice = substream->private_data;
 	int err;
 
+	mutex_lock(&dice->mutex);
 	err = snd_dice_stream_start_duplex(dice, substream->runtime->rate);
+	mutex_unlock(&dice->mutex);
 	if (err >= 0)
 		amdtp_stream_pcm_prepare(&dice->rx_stream);
 
diff --git a/sound/firewire/dice/dice-stream.c b/sound/firewire/dice/dice-stream.c
index 37d1aab..edb6777 100644
--- a/sound/firewire/dice/dice-stream.c
+++ b/sound/firewire/dice/dice-stream.c
@@ -193,11 +193,9 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate)
 	enum cip_flags sync_mode;
 	int err = 0;
 
-	if (atomic_read(&dice->substreams_counter) == 0)
+	if (dice->substreams_counter == 0)
 		goto end;
 
-	mutex_lock(&dice->mutex);
-
 	err = get_sync_mode(dice, &sync_mode);
 	if (err < 0)
 		goto end;
@@ -271,23 +269,18 @@ int snd_dice_stream_start_duplex(struct snd_dice *dice, unsigned int rate)
 		}
 	}
 end:
-	mutex_unlock(&dice->mutex);
 	return err;
 }
 
 void snd_dice_stream_stop_duplex(struct snd_dice *dice)
 {
-	if (atomic_read(&dice->substreams_counter) > 0)
+	if (dice->substreams_counter > 0)
 		return;
 
-	mutex_lock(&dice->mutex);
-
 	snd_dice_transaction_clear_enable(dice);
 
 	stop_stream(dice, &dice->tx_stream);
 	stop_stream(dice, &dice->rx_stream);
-
-	mutex_unlock(&dice->mutex);
 }
 
 static int init_stream(struct snd_dice *dice, struct amdtp_stream *stream)
@@ -332,7 +325,7 @@ int snd_dice_stream_init_duplex(struct snd_dice *dice)
 {
 	int err;
 
-	atomic_set(&dice->substreams_counter, 0);
+	dice->substreams_counter = 0;
 
 	err = init_stream(dice, &dice->tx_stream);
 	if (err < 0)
@@ -345,8 +338,6 @@ end:
 
 void snd_dice_stream_destroy_duplex(struct snd_dice *dice)
 {
-	mutex_lock(&dice->mutex);
-
 	snd_dice_transaction_clear_enable(dice);
 
 	stop_stream(dice, &dice->tx_stream);
@@ -355,13 +346,14 @@ void snd_dice_stream_destroy_duplex(struct snd_dice *dice)
 	stop_stream(dice, &dice->rx_stream);
 	destroy_stream(dice, &dice->rx_stream);
 
-	atomic_set(&dice->substreams_counter, 0);
-
-	mutex_unlock(&dice->mutex);
+	dice->substreams_counter = 0;
 }
 
 void snd_dice_stream_update_duplex(struct snd_dice *dice)
 {
+	/* The enable register becomes initialized, then streams are stopped. */
+	dice->global_enabled = false;
+
 	/*
 	 * On a bus reset, the DICE firmware disables streaming and then goes
 	 * off contemplating its own navel for hundreds of milliseconds before
@@ -370,18 +362,11 @@ void snd_dice_stream_update_duplex(struct snd_dice *dice)
 	 * to stop so that the application can restart them in an orderly
 	 * manner.
 	 */
-	mutex_lock(&dice->mutex);
-
-	/* The enable register becomes initialized, then streams are stopped. */
-	dice->global_enabled = false;
-
 	stop_stream(dice, &dice->rx_stream);
 	stop_stream(dice, &dice->tx_stream);
 
 	fw_iso_resources_update(&dice->rx_resources);
 	fw_iso_resources_update(&dice->tx_resources);
-
-	mutex_unlock(&dice->mutex);
 }
 
 static void dice_lock_changed(struct snd_dice *dice)
diff --git a/sound/firewire/dice/dice.c b/sound/firewire/dice/dice.c
index b6b2c92..2a5605f 100644
--- a/sound/firewire/dice/dice.c
+++ b/sound/firewire/dice/dice.c
@@ -316,10 +316,14 @@ static void dice_update(struct fw_unit *unit)
 {
 	struct snd_dice *dice = dev_get_drvdata(&unit->device);
 
+	mutex_lock(&dice->mutex);
+
 	/* The handler address register becomes initialized. */
 	snd_dice_transaction_reinit(dice);
 
 	snd_dice_stream_update_duplex(dice);
+
+	mutex_unlock(&dice->mutex);
 }
 
 #define DICE_INTERFACE	0x000001
diff --git a/sound/firewire/dice/dice.h b/sound/firewire/dice/dice.h
index 40552ca..d8b721c 100644
--- a/sound/firewire/dice/dice.h
+++ b/sound/firewire/dice/dice.h
@@ -77,7 +77,7 @@ struct snd_dice {
 	struct amdtp_stream rx_stream;
 	bool global_enabled;
 	struct completion clock_accepted;
-	atomic_t substreams_counter;
+	unsigned int substreams_counter;
 };
 
 enum snd_dice_addr_type {
-- 
2.1.0


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]