From: Mark Hills <mark@xwax.org>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: [PATCH 1/4] echoaudio: Race conditions around "opencount"
Date: Wed, 1 Jul 2020 13:27:20 +0100 [thread overview]
Message-ID: <20200701122723.17814-1-mark@xwax.org> (raw)
In-Reply-To: <2007011319580.23012@tamla.localdomain>
Use of atomics does not make these statements robust:
atomic_inc(&chip->opencount);
if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
chip->can_set_rate=0;
and
if (atomic_read(&chip->opencount)) {
if (chip->opencount) {
changed = -EAGAIN;
} else {
changed = set_digital_mode(chip, dmode);
It would be necessary to atomically increment or decrement the value
and use the returned result. And yet we still need to prevent other
threads making use of "can_set_rate" while we set it.
However in all but one case the atomic is misleading as they are already
running with "mode_mutex" held.
Decisions are made on mode setting are often intrinsically connected
to "opencount" because some operations are not permitted unless
there is sole ownership.
So instead simplify this, and use "mode_mutex" as a lock for all reference
counting and mode setting.
Signed-off-by: Mark Hills <mark@xwax.org>
---
sound/pci/echoaudio/echoaudio.c | 76 ++++++++++++++++++---------------
sound/pci/echoaudio/echoaudio.h | 6 +--
2 files changed, 45 insertions(+), 37 deletions(-)
diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c
index 0941a7a17623..82a49dfd2384 100644
--- a/sound/pci/echoaudio/echoaudio.c
+++ b/sound/pci/echoaudio/echoaudio.c
@@ -245,13 +245,20 @@ static int hw_rule_sample_rate(struct snd_pcm_hw_params *params,
SNDRV_PCM_HW_PARAM_RATE);
struct echoaudio *chip = rule->private;
struct snd_interval fixed;
+ int err;
+
+ mutex_lock(&chip->mode_mutex);
- if (!chip->can_set_rate) {
+ if (chip->can_set_rate) {
+ err = 0;
+ } else {
snd_interval_any(&fixed);
fixed.min = fixed.max = chip->sample_rate;
- return snd_interval_refine(rate, &fixed);
+ err = snd_interval_refine(rate, &fixed);
}
- return 0;
+
+ mutex_unlock(&chip->mode_mutex);
+ return err;
}
@@ -322,7 +329,7 @@ static int pcm_open(struct snd_pcm_substream *substream,
SNDRV_PCM_HW_PARAM_RATE, -1)) < 0)
return err;
- /* Finally allocate a page for the scatter-gather list */
+ /* Allocate a page for the scatter-gather list */
if ((err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
&chip->pci->dev,
PAGE_SIZE, &pipe->sgpage)) < 0) {
@@ -330,6 +337,17 @@ static int pcm_open(struct snd_pcm_substream *substream,
return err;
}
+ /*
+ * Sole ownership required to set the rate
+ */
+
+ dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+ chip->opencount, chip->can_set_rate, chip->rate_set);
+
+ chip->opencount++;
+ if (chip->opencount > 1 && chip->rate_set)
+ chip->can_set_rate = 0;
+
return 0;
}
@@ -353,12 +371,7 @@ static int pcm_analog_in_open(struct snd_pcm_substream *substream)
hw_rule_capture_format_by_channels, NULL,
SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
return err;
- atomic_inc(&chip->opencount);
- if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
- chip->can_set_rate=0;
- dev_dbg(chip->card->dev, "pcm_analog_in_open cs=%d oc=%d r=%d\n",
- chip->can_set_rate, atomic_read(&chip->opencount),
- chip->sample_rate);
+
return 0;
}
@@ -388,12 +401,7 @@ static int pcm_analog_out_open(struct snd_pcm_substream *substream)
NULL,
SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
return err;
- atomic_inc(&chip->opencount);
- if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
- chip->can_set_rate=0;
- dev_dbg(chip->card->dev, "pcm_analog_out_open cs=%d oc=%d r=%d\n",
- chip->can_set_rate, atomic_read(&chip->opencount),
- chip->sample_rate);
+
return 0;
}
@@ -429,10 +437,6 @@ static int pcm_digital_in_open(struct snd_pcm_substream *substream)
SNDRV_PCM_HW_PARAM_CHANNELS, -1)) < 0)
goto din_exit;
- atomic_inc(&chip->opencount);
- if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
- chip->can_set_rate=0;
-
din_exit:
mutex_unlock(&chip->mode_mutex);
return err;
@@ -471,9 +475,7 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
NULL, SNDRV_PCM_HW_PARAM_CHANNELS,
-1)) < 0)
goto dout_exit;
- atomic_inc(&chip->opencount);
- if (atomic_read(&chip->opencount) > 1 && chip->rate_set)
- chip->can_set_rate=0;
+
dout_exit:
mutex_unlock(&chip->mode_mutex);
return err;
@@ -488,23 +490,29 @@ static int pcm_digital_out_open(struct snd_pcm_substream *substream)
static int pcm_close(struct snd_pcm_substream *substream)
{
struct echoaudio *chip = snd_pcm_substream_chip(substream);
- int oc;
/* Nothing to do here. Audio is already off and pipe will be
* freed by its callback
*/
- atomic_dec(&chip->opencount);
- oc = atomic_read(&chip->opencount);
- dev_dbg(chip->card->dev, "pcm_close oc=%d cs=%d rs=%d\n", oc,
- chip->can_set_rate, chip->rate_set);
- if (oc < 2)
+ mutex_lock(&chip->mode_mutex);
+
+ dev_dbg(chip->card->dev, "pcm_open opencount=%d can_set_rate=%d, rate_set=%d",
+ chip->opencount, chip->can_set_rate, chip->rate_set);
+
+ chip->opencount--;
+
+ switch (chip->opencount) {
+ case 1:
chip->can_set_rate = 1;
- if (oc == 0)
+ break;
+
+ case 0:
chip->rate_set = 0;
- dev_dbg(chip->card->dev, "pcm_close2 oc=%d cs=%d rs=%d\n", oc,
- chip->can_set_rate, chip->rate_set);
+ break;
+ }
+ mutex_unlock(&chip->mode_mutex);
return 0;
}
@@ -1409,7 +1417,7 @@ static int snd_echo_digital_mode_put(struct snd_kcontrol *kcontrol,
/* Do not allow the user to change the digital mode when a pcm
device is open because it also changes the number of channels
and the allowed sample rates */
- if (atomic_read(&chip->opencount)) {
+ if (chip->opencount) {
changed = -EAGAIN;
} else {
changed = set_digital_mode(chip, dmode);
@@ -1874,7 +1882,7 @@ static int snd_echo_create(struct snd_card *card,
chip->card = card;
chip->pci = pci;
chip->irq = -1;
- atomic_set(&chip->opencount, 0);
+ chip->opencount = 0;
mutex_init(&chip->mode_mutex);
chip->can_set_rate = 1;
} else {
diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h
index be4d0489394a..6fd283e4e676 100644
--- a/sound/pci/echoaudio/echoaudio.h
+++ b/sound/pci/echoaudio/echoaudio.h
@@ -336,7 +336,7 @@ struct echoaudio {
struct mutex mode_mutex;
u16 num_digital_modes, digital_mode_list[6];
u16 num_clock_sources, clock_source_list[10];
- atomic_t opencount;
+ unsigned int opencount; /* protected by mode_mutex */
struct snd_kcontrol *clock_src_ctl;
struct snd_pcm *analog_pcm, *digital_pcm;
struct snd_card *card;
@@ -353,8 +353,8 @@ struct echoaudio {
struct timer_list timer;
char tinuse; /* Timer in use */
char midi_full; /* MIDI output buffer is full */
- char can_set_rate;
- char rate_set;
+ char can_set_rate; /* protected by mode_mutex */
+ char rate_set; /* protected by mode_mutex */
/* This stuff is used mainly by the lowlevel code */
struct comm_page *comm_page; /* Virtual address of the memory
--
2.17.5
next prev parent reply other threads:[~2020-07-01 12:28 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-16 13:13 echoaudio: Fix some long standing bugs Mark Hills
2020-06-16 13:17 ` [PATCH 1/3] echoaudio: Race conditions around "opencount" Mark Hills
2020-06-16 13:17 ` [PATCH 2/3] echoaudio: Prevent races in calls to set_audio_format() Mark Hills
2020-06-16 13:24 ` Takashi Iwai
2020-06-16 13:17 ` [PATCH 3/3] echoaudio: Address bugs in the interrupt handling Mark Hills
2020-06-16 13:35 ` Takashi Iwai
2020-06-16 14:01 ` Mark Hills
2020-06-16 14:18 ` Takashi Iwai
2020-06-17 10:51 ` Mark Hills
2020-06-18 8:17 ` Takashi Iwai
2020-06-18 11:07 ` Mark Hills
2020-06-18 11:21 ` Takashi Iwai
2020-06-18 12:29 ` Mark Hills
2020-06-18 13:22 ` Mark Hills
2020-06-16 19:46 ` Giuliano Pochini
2020-06-17 10:57 ` Mark Hills
2020-06-16 22:01 ` Giuliano Pochini
2020-06-17 11:14 ` Mark Hills
2020-06-19 19:56 ` Giuliano Pochini
2020-06-19 21:21 ` Mark Hills
2020-06-28 22:02 ` Giuliano Pochini
2020-07-01 12:25 ` Mark Hills
2020-07-01 14:51 ` Giuliano Pochini
2020-07-01 12:25 ` echoaudio: Fix some long standing bugs Mark Hills
2020-07-01 12:27 ` Mark Hills [this message]
2020-07-01 16:37 ` [PATCH 1/4] echoaudio: Race conditions around "opencount" kernel test robot
2020-07-01 17:32 ` kernel test robot
2020-07-02 9:53 ` Mark Hills
2020-07-07 8:28 ` Takashi Iwai
2020-07-08 10:16 ` Mark Hills
2020-07-08 10:18 ` [PATCH 1/5] echoaudio: Remove redundant check Mark Hills
2020-07-09 11:00 ` Takashi Iwai
2020-07-08 10:18 ` [PATCH 2/5] echoaudio: Race conditions around "opencount" Mark Hills
2020-07-09 11:00 ` Takashi Iwai
2020-07-08 10:18 ` [PATCH 3/5] echoaudio: Prevent races in calls to set_audio_format() Mark Hills
2020-07-09 11:00 ` Takashi Iwai
2020-07-08 10:18 ` [PATCH 4/5] echoaudio: Prevent some noise on unloading the module Mark Hills
2020-07-09 11:00 ` Takashi Iwai
2020-07-08 10:18 ` [PATCH 5/5] echoaudio: Address bugs in the interrupt handling Mark Hills
2020-07-09 11:01 ` Takashi Iwai
2020-07-01 12:27 ` [PATCH 2/4] echoaudio: Prevent races in calls to set_audio_format() Mark Hills
2020-07-01 12:27 ` [PATCH 3/4] echoaudio: Prevent some noise on unloading the module Mark Hills
2020-07-01 12:27 ` [PATCH 4/4] echoaudio: Address bugs in the interrupt handling Mark Hills
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=20200701122723.17814-1-mark@xwax.org \
--to=mark@xwax.org \
--cc=alsa-devel@alsa-project.org \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).