[PATCH 1/3] echoaudio: Race conditions around "opencount"
Mark Hills
mark at xwax.org
Tue Jun 16 15:17:41 CEST 2020
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 at 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
More information about the Alsa-devel
mailing list