Messages in this thread | | | Subject | Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load() | From | Jia-Ju Bai <> | Date | Mon, 19 Jul 2021 10:22:40 +0800 |
| |
On 2021/7/15 18:33, Takashi Iwai wrote: > On Thu, 15 Jul 2021 12:20:36 +0200, > Jia-Ju Bai wrote: >> Hello, >> >> I find there is a possible ABBA deadlock in the SB16 driver in Linux 5.10: >> >> In snd_sb_csp_stop(): >> 876: spin_lock_irqsave(&p->chip->mixer_lock, flags); >> 882: spin_lock(&p->chip->reg_lock); >> >> In snd_sb_csp_load(): >> 614: spin_lock_irqsave(&p->chip->reg_lock, flags); >> 653: spin_lock(&p->chip->mixer_lock); >> >> When snd_sb_csp_stop() and snd_sb_csp_load() are concurrently >> executed, the deadlock can occur. >> >> I check the code and find a possible case of such concurrent execution: >> >> #CPU1: >> snd_sb16_playback_close >> snd_sb16_csp_playback_close (csp->ops.csp_stop(csp)) >> snd_sb_csp_stop >> >> #CPU2: >> snd_sb_csp_ioctl >> snd_sb_csp_riff_load >> snd_sb_csp_load_user >> snd_sb_csp_load >> >> I am not quite sure whether this possible deadlock is real and how to >> fix it if it is real. >> Any feedback would be appreciated, thanks > The impact must be quite low, as both functions run in different > state (running or stopped), so those are basically exclusive. > And, above all, there is no VM supporting this chip, hence it's only > for the real hardware and it's about very old ISA boards that maybe > only less than handful people in the world can run now. > > About the fix: just split the locks in snb_sb_csp_stop() (also > snd_sb_csp_start()) like below should suffice.
Thanks for the feedback and explanation :) The patch looks good to me.
> > --- a/sound/isa/sb/sb16_csp.c > +++ b/sound/isa/sb/sb16_csp.c > @@ -816,6 +816,7 @@ static int snd_sb_csp_start(struct snd_sb_csp * p, int sample_width, int channel > mixR = snd_sbmixer_read(p->chip, SB_DSP4_PCM_DEV + 1); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL & 0x7); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR & 0x7); > + spin_unlock_irqrestore(&p->chip->mixer_lock, flags); > > spin_lock(&p->chip->reg_lock); > set_mode_register(p->chip, 0xc0); /* c0 = STOP */ > @@ -855,6 +856,7 @@ static int snd_sb_csp_start(struct snd_sb_csp * p, int sample_width, int channel > spin_unlock(&p->chip->reg_lock); > > /* restore PCM volume */ > + spin_lock_irqsave(&p->chip->mixer_lock, flags); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR); > spin_unlock_irqrestore(&p->chip->mixer_lock, flags); > @@ -880,6 +882,7 @@ static int snd_sb_csp_stop(struct snd_sb_csp * p) > mixR = snd_sbmixer_read(p->chip, SB_DSP4_PCM_DEV + 1); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL & 0x7); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR & 0x7); > + spin_unlock_irqrestore(&p->chip->mixer_lock, flags); > > spin_lock(&p->chip->reg_lock); > if (p->running & SNDRV_SB_CSP_ST_QSOUND) { > @@ -894,6 +897,7 @@ static int snd_sb_csp_stop(struct snd_sb_csp * p) > spin_unlock(&p->chip->reg_lock); > > /* restore PCM volume */ > + spin_lock_irqsave(&p->chip->mixer_lock, flags); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL); > snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR); > spin_unlock_irqrestore(&p->chip->mixer_lock, flags);
| |