diff options
author | Mark Hills <mark@xwax.org> | 2020-07-08 13:18:45 +0300 |
---|---|---|
committer | Takashi Iwai <tiwai@suse.de> | 2020-07-09 13:58:35 +0300 |
commit | 027c70021b905eb23e2f36c64add062ba218b841 (patch) | |
tree | e31ba3386c152bc8515b16d3022eb674d2d0859b /sound | |
parent | db091b0e14239f0ebd5709f3f9b5a1d662483adc (diff) | |
download | linux-027c70021b905eb23e2f36c64add062ba218b841.tar.xz |
ALSA: echoaudio: Race conditions around "opencount"
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>
Link: https://lore.kernel.org/r/20200708101848.3457-2-mark@xwax.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Diffstat (limited to 'sound')
-rw-r--r-- | sound/pci/echoaudio/echoaudio.c | 76 | ||||
-rw-r--r-- | 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 bee3271e29da..f2bc37c144d6 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 @@ dout_exit: 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 |