diff mbox

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

Message ID 546DC356.8010207@sakamocchi.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Sakamoto Nov. 20, 2014, 10:32 a.m. UTC 
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

Comments

Clemens Ladisch Nov. 24, 2014, 1:54 p.m. UTC | #1 
Takashi Sakamoto wrote:
> On Nov 17 2014 06:21, Clemens Ladisch wrote:
>> 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

Just use a kernel compiled with CONFIG_PROVE_LOCKING.

> 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?

It makes the driver work correctly.

There's nothing wrong with atomic_t itself, but in this case, none of
the accesses would be valid outside of the lock, so it is not necessary
for them to be atomic.

In theory, it would be possible to use atomic_t to synchronize multiple
streams, but this requires that
1. two CPUs that start a stream at the same time do not both think they
   are the first, so you *must* use something like atomic_inc_and_test(),
   and
2. when one CPU is still starting a stream, code on all other CPUs must
   be able to work correctly, without synchronization.  This is not
   possible if the code requires to know that the DMA actually has
   started; consider the following code:

   void start_some_substream(...)
   {
       if (atomic_inc_and_test(substreams)) {
           mutex_lock();
           // lots of stuff to start DMA ...
           mutex_unlock();
       } else {
           // wait for the DMA to be started
       }
       ...
   }

   How could the 'else' branch ensure that the DMA engine is started?
   Just taking the mutex is not enough, because it could happen before
   the mutex_lock() of the first stream.  This is not possible without
   moving the inc_and_test into the locked region.


Regards,
Clemens
Clemens Ladisch Nov. 25, 2014, 12:04 p.m. UTC | #2 
Takashi Sakamoto wrote:
> On Nov 24 2014 22:54, Clemens Ladisch wrote:
>> In theory, it would be possible to use atomic_t to synchronize multiple
>> streams, but this requires that
>> 1. two CPUs that start a stream at the same time do not both think they
>>    are the first, ... and
>> 2. when one CPU is still starting a stream, code on all other CPUs must
>>    be able to work correctly, without synchronization.  [...]
>
> If I rewrite your pseudo code, like this:
>
>>    void start_some_substream(...)
>>    {
>>        if (atomic_inc_and_test(substreams)) {
>>            mutex_lock();
>>            // lots of stuff to start DMA ...
>
>              if (wait_for_first_packet() == ETIMEDOUT) {
>                  stop_dma();
>                  release_streams();
>                  mutex_unlock();
>                  return ETIMEDOUT;
>              }
>>            mutex_unlock();
>>        }
>>        ...
>>    }

This is an even better example of why atomic_t is incorrect:  when the
startup fails, the second call to start_some_substream() at the same
time, which just sees that substreams>0, cannot notice the error, and
thinks its own substream has been started successfully.


Regards,
Clemens
Takashi Sakamoto Nov. 25, 2014, 10:41 p.m. UTC | #3 
On Nov 15 2014 21:04, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> If I rewrite your pseudo code, like this:
>>
>>>    void start_some_substream(...)
>>>    {
>>>        if (atomic_inc_and_test(substreams)) {
>>>            mutex_lock();
>>>            // lots of stuff to start DMA ...
>>
>>              if (wait_for_first_packet() == ETIMEDOUT) {
>>                  stop_dma();
>>                  release_streams();
>>                  mutex_unlock();
>>                  return ETIMEDOUT;
>>              }
>>>            mutex_unlock();
>>>        }
>>>        ...
>>>    }
> 
> This is an even better example of why atomic_t is incorrect:  when the
> startup fails, the second call to start_some_substream() at the same
> time, which just sees that substreams>0, cannot notice the error, and
> thinks its own substream has been started successfully.

Yes. The reference counter is invalid in this pseudo code which I
showrf. I left your insistence about 'atomic_inc_and_test()' what it is.

More precisely, current implementations (bebob/fireworks/dice) are (or
should be for oxfw) like:

void start_some_substream(...)
{
    mutex_lock();

    if (atomic_read(substreams) == 0)
        goto end;

    if (streams_are_running())
        goto end;

    if (!start_isochronous_context_and_dma()) {
        release_isochronous_context_and_dma();
        release_streams();
        goto end;
    }

    if (wait_for_first_packet() == ETIMEDOUT) {
        release_isochronous_context_and_dma();
        release_streams();
    }
end:
    mutex_unlock();
    return err;
}

I believe that the reference counter with atomic_t is valid regardless
of inner/outer the lock (mutex). For this critical section, it's just a
importance that some substreams already subscribe the stream or not,
instead of the number of substreams.

If the reference counter is non-zero, it means that 'a stream is
requested to run for some substream, even if the substreams are not for
current caller'.

I don't show in the pseudo code; when the streams are already running
but the sampling transfer frequency is not the same as the one which the
caller requested, then the streams are stopped once and set sampling
transfer frequency and restart them.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
diff mbox

Patch

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