All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
To: Clemens Ladisch <clemens@ladisch.de>
Cc: tiwai@suse.de, alsa-devel@alsa-project.org,
	ffado-devel@lists.sourceforge.net
Subject: Re: [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream
Date: Mon, 24 Nov 2014 10:03:05 +0900	[thread overview]
Message-ID: <547283C9.8040206@sakamocchi.jp> (raw)
In-Reply-To: <546DC356.8010207@sakamocchi.jp>

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

Clemens,

On Nov 20 2014 19:32, Takashi Sakamoto 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, 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?

Oops. I attached a patch for Dice driver...

But my strategy for programming is nearly the same. Would you please
give me some comments so as I can post new patchsets in this week?


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp


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

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

OXFW 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 has 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/oxfw/oxfw-midi.c   | 16 ++++++++++++----
 sound/firewire/oxfw/oxfw-pcm.c    | 34 ++++++++++++++++++++++++++--------
 sound/firewire/oxfw/oxfw-stream.c | 39 +++++++++++----------------------------
 sound/firewire/oxfw/oxfw.c        |  4 ++++
 sound/firewire/oxfw/oxfw.h        |  4 ++--
 5 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/sound/firewire/oxfw/oxfw-midi.c b/sound/firewire/oxfw/oxfw-midi.c
index 94bc8d0..331838e 100644
--- a/sound/firewire/oxfw/oxfw-midi.c
+++ b/sound/firewire/oxfw/oxfw-midi.c
@@ -17,8 +17,10 @@ static int midi_capture_open(struct snd_rawmidi_substream *substream)
 	if (err < 0)
 		goto end;
 
-	atomic_inc(&oxfw->capture_substreams);
+	mutex_lock(&oxfw->mutex);
+	oxfw->capture_substreams++;
 	err = snd_oxfw_stream_start_simplex(oxfw, &oxfw->tx_stream, 0, 0);
+	mutex_unlock(&oxfw->mutex);
 	if (err < 0)
 		snd_oxfw_stream_lock_release(oxfw);
 end:
@@ -34,8 +36,10 @@ static int midi_playback_open(struct snd_rawmidi_substream *substream)
 	if (err < 0)
 		goto end;
 
-	atomic_inc(&oxfw->playback_substreams);
+	mutex_lock(&oxfw->mutex);
+	oxfw->playback_substreams++;
 	err = snd_oxfw_stream_start_simplex(oxfw, &oxfw->rx_stream, 0, 0);
+	mutex_unlock(&oxfw->mutex);
 	if (err < 0)
 		snd_oxfw_stream_lock_release(oxfw);
 end:
@@ -46,8 +50,10 @@ static int midi_capture_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_oxfw *oxfw = substream->rmidi->private_data;
 
-	atomic_dec(&oxfw->capture_substreams);
+	mutex_lock(&oxfw->mutex);
+	oxfw->capture_substreams--;
 	snd_oxfw_stream_stop_simplex(oxfw, &oxfw->tx_stream);
+	mutex_unlock(&oxfw->mutex);
 
 	snd_oxfw_stream_lock_release(oxfw);
 	return 0;
@@ -57,8 +63,10 @@ static int midi_playback_close(struct snd_rawmidi_substream *substream)
 {
 	struct snd_oxfw *oxfw = substream->rmidi->private_data;
 
-	atomic_dec(&oxfw->playback_substreams);
+	mutex_lock(&oxfw->mutex);
+	oxfw->playback_substreams--;
 	snd_oxfw_stream_stop_simplex(oxfw, &oxfw->rx_stream);
+	mutex_unlock(&oxfw->mutex);
 
 	snd_oxfw_stream_lock_release(oxfw);
 	return 0;
diff --git a/sound/firewire/oxfw/oxfw-pcm.c b/sound/firewire/oxfw/oxfw-pcm.c
index c508a1f..475d1aa 100644
--- a/sound/firewire/oxfw/oxfw-pcm.c
+++ b/sound/firewire/oxfw/oxfw-pcm.c
@@ -232,8 +232,12 @@ static int pcm_capture_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_oxfw *oxfw = substream->private_data;
 
-	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN)
-		atomic_inc(&oxfw->capture_substreams);
+	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&oxfw->mutex);
+		oxfw->capture_substreams++;
+		mutex_unlock(&oxfw->mutex);
+	}
+
 	amdtp_stream_set_pcm_format(&oxfw->tx_stream, params_format(hw_params));
 
 	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
@@ -244,8 +248,12 @@ static int pcm_playback_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_oxfw *oxfw = substream->private_data;
 
-	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN)
-		atomic_inc(&oxfw->playback_substreams);
+	if (substream->runtime->status->state == SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&oxfw->mutex);
+		oxfw->playback_substreams++;
+		mutex_unlock(&oxfw->mutex);
+	}
+
 	amdtp_stream_set_pcm_format(&oxfw->rx_stream, params_format(hw_params));
 
 	return snd_pcm_lib_alloc_vmalloc_buffer(substream,
@@ -256,8 +264,11 @@ static int pcm_capture_hw_free(struct snd_pcm_substream *substream)
 {
 	struct snd_oxfw *oxfw = substream->private_data;
 
-	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN)
-		atomic_dec(&oxfw->capture_substreams);
+	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&oxfw->mutex);
+		oxfw->capture_substreams--;
+		mutex_unlock(&oxfw->mutex);
+	}
 
 	snd_oxfw_stream_stop_simplex(oxfw, &oxfw->tx_stream);
 
@@ -267,8 +278,11 @@ static int pcm_playback_hw_free(struct snd_pcm_substream *substream)
 {
 	struct snd_oxfw *oxfw = substream->private_data;
 
-	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN)
-		atomic_dec(&oxfw->playback_substreams);
+	if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) {
+		mutex_lock(&oxfw->mutex);
+		oxfw->playback_substreams--;
+		mutex_unlock(&oxfw->mutex);
+	}
 
 	snd_oxfw_stream_stop_simplex(oxfw, &oxfw->rx_stream);
 
@@ -281,8 +295,10 @@ static int pcm_capture_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int err;
 
+	mutex_lock(&oxfw->mutex);
 	err = snd_oxfw_stream_start_simplex(oxfw, &oxfw->tx_stream,
 					    runtime->rate, runtime->channels);
+	mutex_unlock(&oxfw->mutex);
 	if (err < 0)
 		goto end;
 
@@ -296,8 +312,10 @@ static int pcm_playback_prepare(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	int err;
 
+	mutex_lock(&oxfw->mutex);
 	err = snd_oxfw_stream_start_simplex(oxfw, &oxfw->rx_stream,
 					    runtime->rate, runtime->channels);
+	mutex_unlock(&oxfw->mutex);
 	if (err < 0)
 		goto end;
 
diff --git a/sound/firewire/oxfw/oxfw-stream.c b/sound/firewire/oxfw/oxfw-stream.c
index b1dcc68..00870df 100644
--- a/sound/firewire/oxfw/oxfw-stream.c
+++ b/sound/firewire/oxfw/oxfw-stream.c
@@ -219,8 +219,6 @@ int snd_oxfw_stream_init_simplex(struct snd_oxfw *oxfw,
 		s_dir = AMDTP_OUT_STREAM;
 	}
 
-	mutex_lock(&oxfw->mutex);
-
 	err = cmp_connection_init(conn, oxfw->unit, c_dir, 0);
 	if (err < 0)
 		goto end;
@@ -236,7 +234,6 @@ int snd_oxfw_stream_init_simplex(struct snd_oxfw *oxfw,
 	if (stream == &oxfw->tx_stream)
 		oxfw->tx_stream.flags |= CIP_SKIP_INIT_DBC_CHECK;
 end:
-	mutex_unlock(&oxfw->mutex);
 	return err;
 }
 
@@ -247,17 +244,17 @@ int snd_oxfw_stream_start_simplex(struct snd_oxfw *oxfw,
 	struct amdtp_stream *opposite;
 	struct snd_oxfw_stream_formation formation;
 	enum avc_general_plug_dir dir;
-	atomic_t *substreams, *opposite_substreams;
+	unsigned int substreams, opposite_substreams;
 	int err = 0;
 
 	if (stream == &oxfw->tx_stream) {
-		substreams = &oxfw->capture_substreams;
+		substreams = oxfw->capture_substreams;
 		opposite = &oxfw->rx_stream;
-		opposite_substreams = &oxfw->playback_substreams;
+		opposite_substreams = oxfw->playback_substreams;
 		dir = AVC_GENERAL_PLUG_DIR_OUT;
 	} else {
-		substreams = &oxfw->playback_substreams;
-		opposite_substreams = &oxfw->capture_substreams;
+		substreams = oxfw->playback_substreams;
+		opposite_substreams = oxfw->capture_substreams;
 
 		if (oxfw->has_output)
 			opposite = &oxfw->rx_stream;
@@ -267,11 +264,9 @@ int snd_oxfw_stream_start_simplex(struct snd_oxfw *oxfw,
 		dir = AVC_GENERAL_PLUG_DIR_IN;
 	}
 
-	if (atomic_read(substreams) == 0)
+	if (substreams == 0)
 		goto end;
 
-	mutex_lock(&oxfw->mutex);
-
 	/*
 	 * Considering JACK/FFADO streaming:
 	 * TODO: This can be removed hwdep functionality becomes popular.
@@ -310,7 +305,7 @@ int snd_oxfw_stream_start_simplex(struct snd_oxfw *oxfw,
 
 		/* Start opposite stream if needed. */
 		if (opposite && !amdtp_stream_running(opposite) &&
-		    (atomic_read(opposite_substreams) > 0)) {
+		    (opposite_substreams > 0)) {
 			err = start_stream(oxfw, opposite, rate, 0);
 			if (err < 0) {
 				dev_err(&oxfw->unit->device,
@@ -328,22 +323,17 @@ int snd_oxfw_stream_start_simplex(struct snd_oxfw *oxfw,
 				"fail to start stream: %d\n", err);
 	}
 end:
-	mutex_unlock(&oxfw->mutex);
 	return err;
 }
 
 void snd_oxfw_stream_stop_simplex(struct snd_oxfw *oxfw,
 				  struct amdtp_stream *stream)
 {
-	if (((stream == &oxfw->tx_stream) &&
-	     (atomic_read(&oxfw->capture_substreams) > 0)) ||
-	    ((stream == &oxfw->rx_stream) &&
-	     (atomic_read(&oxfw->playback_substreams) > 0)))
+	if (((stream == &oxfw->tx_stream) && (oxfw->capture_substreams > 0)) ||
+	    ((stream == &oxfw->rx_stream) && (oxfw->playback_substreams > 0)))
 		return;
 
-	mutex_lock(&oxfw->mutex);
 	stop_stream(oxfw, stream);
-	mutex_unlock(&oxfw->mutex);
 }
 
 void snd_oxfw_stream_destroy_simplex(struct snd_oxfw *oxfw,
@@ -356,14 +346,10 @@ void snd_oxfw_stream_destroy_simplex(struct snd_oxfw *oxfw,
 	else
 		conn = &oxfw->in_conn;
 
-	mutex_lock(&oxfw->mutex);
-
 	stop_stream(oxfw, stream);
 
 	amdtp_stream_destroy(stream);
 	cmp_connection_destroy(conn);
-
-	mutex_unlock(&oxfw->mutex);
 }
 
 void snd_oxfw_stream_update_simplex(struct snd_oxfw *oxfw,
@@ -376,13 +362,10 @@ void snd_oxfw_stream_update_simplex(struct snd_oxfw *oxfw,
 	else
 		conn = &oxfw->in_conn;
 
-	if (cmp_connection_update(conn) < 0) {
-		mutex_lock(&oxfw->mutex);
+	if (cmp_connection_update(conn) < 0)
 		stop_stream(oxfw, stream);
-		mutex_unlock(&oxfw->mutex);
-	} else {
+	else
 		amdtp_stream_update(stream);
-	}
 }
 
 int snd_oxfw_stream_get_current_formation(struct snd_oxfw *oxfw,
diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
index 40cd2c0..60c68ce 100644
--- a/sound/firewire/oxfw/oxfw.c
+++ b/sound/firewire/oxfw/oxfw.c
@@ -214,10 +214,14 @@ static void oxfw_bus_reset(struct fw_unit *unit)
 {
 	struct snd_oxfw *oxfw = dev_get_drvdata(&unit->device);
 
+	mutex_lock(&oxfw->mutex);
+
 	fcp_bus_reset(oxfw->unit);
 	snd_oxfw_stream_update_simplex(oxfw, &oxfw->rx_stream);
 	if (oxfw->has_output)
 		snd_oxfw_stream_update_simplex(oxfw, &oxfw->tx_stream);
+
+	mutex_unlock(&oxfw->mutex);
 }
 
 static void oxfw_remove(struct fw_unit *unit)
diff --git a/sound/firewire/oxfw/oxfw.h b/sound/firewire/oxfw/oxfw.h
index d2f8ee5..65b95f2 100644
--- a/sound/firewire/oxfw/oxfw.h
+++ b/sound/firewire/oxfw/oxfw.h
@@ -56,8 +56,8 @@ struct snd_oxfw {
 	struct cmp_connection in_conn;
 	struct amdtp_stream tx_stream;
 	struct amdtp_stream rx_stream;
-	atomic_t capture_substreams;
-	atomic_t playback_substreams;
+	unsigned int capture_substreams;
+	unsigned int playback_substreams;
 
 	unsigned int midi_input_ports;
 	unsigned int midi_output_ports;
-- 
2.1.0


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



  reply	other threads:[~2014-11-24  1:03 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-26 13:03 [PATCH 00/29 v2] ALSA: Enhancement for existed FireWire drivers Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 01/29] ALSA: dice: Rename structure and its members Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 02/29] ALSA: dice: Move file to its own directory Takashi Sakamoto
2014-11-18 12:57   ` Clemens Ladisch
2014-11-18 15:29     ` Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 03/29] ALSA: dice: Split transaction functionality into a file Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 04/29] ALSA: dice: Split stream " Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 05/29] ALSA: dice: Split PCM " Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 06/29] ALSA: dice: Split hwdep " Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 07/29] ALSA: dice: Split proc interface " Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 08/29] ALSA: dice: Add new functions for constraints of PCM parameters Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 09/29] ALSA: dice: Change the way to start stream Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 10/29] ALSA: dice: Add support for duplex streams with synchronization Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 11/29] ALSA: dice: Support for non SYT-Match sampling clock source mode Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 12/29] ALSA: dice: Add support for capturing PCM samples Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 13/29] ALSA: dice: Add support for MIDI capture/playback Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 14/29] ALSA: dice: remove experimental state Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 15/29] ALSA: speakers: Rename to oxfw and rename some members Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 16/29] ALSA: oxfw: Move to its own directory Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 17/29] ALSA: oxfw: Split stream functionality to a new file and add a header file Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 18/29] ALSA: oxfw: Split PCM functionality to a new file Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 19/29] ALSA: oxfw: Split control " Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 20/29] ALSA: oxfw: Change the way to name card Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 21/29] ALSA: oxfw: Add support for AV/C stream format command to get/set supported stream formation Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 22/29] ALSA: oxfw: Change the way to make PCM rules/constraints Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 23/29] ALSA: oxfw: Add proc interface for debugging purpose Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 24/29] ALSA: oxfw: Change the way to start stream Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 25/29] ALSA: oxfw: Add support for Behringer/Mackie devices Takashi Sakamoto
2014-11-16 20:57   ` Clemens Ladisch
2014-11-18 15:24     ` Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream Takashi Sakamoto
2014-11-16 21:21   ` Clemens Ladisch
2014-11-20 10:32     ` Takashi Sakamoto
2014-11-24  1:03       ` Takashi Sakamoto [this message]
2014-11-24 13:54       ` Clemens Ladisch
     [not found]         ` <54746395.4070807@sakamocchi.jp>
2014-11-25 12:04           ` Clemens Ladisch
2014-11-25 22:41             ` Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 27/29] ALSA: oxfw: add support for capturing PCM samples Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 28/29] ALSA: oxfw: Add support for capture/playback MIDI messages Takashi Sakamoto
2014-10-26 13:03 ` [PATCH 29/29] ALSA: oxfw: Add hwdep interface Takashi Sakamoto
2014-10-26 13:29 ` [PATCH] hwdep: add OXFW driver support Takashi Sakamoto
2014-10-26 13:43   ` [PATCH alsa-lib] " Takashi Sakamoto
2014-10-26 16:51 ` [PATCH 00/29 v2] ALSA: Enhancement for existed FireWire drivers Stefan Richter
2014-11-14 10:50 ` Takashi Iwai
2014-11-14 12:08   ` Clemens Ladisch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=547283C9.8040206@sakamocchi.jp \
    --to=o-takashi@sakamocchi.jp \
    --cc=alsa-devel@alsa-project.org \
    --cc=clemens@ladisch.de \
    --cc=ffado-devel@lists.sourceforge.net \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.