From patchwork Mon Nov 24 01:03:05 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Sakamoto X-Patchwork-Id: 5362631 Return-Path: X-Original-To: patchwork-alsa-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id EDABDC11AC for ; Mon, 24 Nov 2014 01:03:38 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B19F22034B for ; Mon, 24 Nov 2014 01:03:37 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id 95E4120306 for ; Mon, 24 Nov 2014 01:03:34 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id BC72C261A50; Mon, 24 Nov 2014 02:03:32 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id A8FAD261A53; Mon, 24 Nov 2014 02:03:25 +0100 (CET) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id A3D0E261A50; Mon, 24 Nov 2014 02:03:22 +0100 (CET) Received: from smtp310.phy.lolipop.jp (smtp310.phy.lolipop.jp [210.157.22.78]) by alsa0.perex.cz (Postfix) with ESMTP id B6845261A5E for ; Mon, 24 Nov 2014 02:03:13 +0100 (CET) Received: from smtp310.phy.lolipop.lan (HELO smtp310.phy.lolipop.jp) (172.17.1.10) (smtp-auth username m12129643-o-takashi, mechanism plain) by smtp310.phy.lolipop.jp (qpsmtpd/0.82) with ESMTPA; Mon, 24 Nov 2014 10:03:07 +0900 Received: from 127.0.0.1 (127.0.0.1) by smtp310.phy.lolipop.jp (LOLIPOP-Fsecure); Mon, 24 Nov 2014 10:03:05 +0900 (JST) X-Virus-Status: clean(LOLIPOP-Fsecure) Message-ID: <547283C9.8040206@sakamocchi.jp> Date: Mon, 24 Nov 2014 10:03:05 +0900 From: Takashi Sakamoto User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Clemens Ladisch References: <1414328610-12729-1-git-send-email-o-takashi@sakamocchi.jp> <1414328610-12729-27-git-send-email-o-takashi@sakamocchi.jp> <54691544.2020501@ladisch.de> <546DC356.8010207@sakamocchi.jp> In-Reply-To: <546DC356.8010207@sakamocchi.jp> Cc: tiwai@suse.de, alsa-devel@alsa-project.org, ffado-devel@lists.sourceforge.net Subject: Re: [alsa-devel] [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP 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 From cecd70d492e343e802f71e9cca94b94fb46efa93 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto 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 --- 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