From patchwork Thu Nov 20 10:32:54 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Sakamoto X-Patchwork-Id: 5347371 Return-Path: X-Original-To: patchwork-alsa-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id EFCF09F387 for ; Thu, 20 Nov 2014 10:33:31 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 910C52020E for ; Thu, 20 Nov 2014 10:33:30 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id 6B63120107 for ; Thu, 20 Nov 2014 10:33:28 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id CDBF6265A23; Thu, 20 Nov 2014 11:33:26 +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,NO_DNS_FOR_FROM, RCVD_IN_DNSWL_NONE,UNPARSEABLE_RELAY autolearn=no version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id 83A8926593D; Thu, 20 Nov 2014 11:33:16 +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 2D4452659A5; Thu, 20 Nov 2014 11:33:11 +0100 (CET) Received: from smtp302.phy.lolipop.jp (smtp302.phy.lolipop.jp [210.157.22.85]) by alsa0.perex.cz (Postfix) with ESMTP id 33CF7265715 for ; Thu, 20 Nov 2014 11:33:01 +0100 (CET) Received: from smtp302.phy.lolipop.lan (HELO smtp302.phy.lolipop.jp) (172.17.1.85) (smtp-auth username m12129643-o-takashi, mechanism plain) by smtp302.phy.lolipop.jp (qpsmtpd/0.82) with ESMTPA; Thu, 20 Nov 2014 19:32:57 +0900 Received: from 127.0.0.1 (127.0.0.1) by smtp302.phy.lolipop.jp (LOLIPOP-Fsecure); Thu, 20 Nov 2014 19:32:55 +0900 (JST) X-Virus-Status: clean(LOLIPOP-Fsecure) Message-ID: <546DC356.8010207@sakamocchi.jp> Date: Thu, 20 Nov 2014 19:32:54 +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> In-Reply-To: <54691544.2020501@ladisch.de> 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 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 From 3e9968a4063f7b5d2977970821f34d5df3b70c57 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto 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 --- 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