From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 83DEDC07E96 for ; Thu, 15 Jul 2021 10:33:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 68417613BB for ; Thu, 15 Jul 2021 10:33:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241413AbhGOKgQ (ORCPT ); Thu, 15 Jul 2021 06:36:16 -0400 Received: from smtp-out2.suse.de ([195.135.220.29]:42728 "EHLO smtp-out2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231670AbhGOKgP (ORCPT ); Thu, 15 Jul 2021 06:36:15 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id AC5441FE0E; Thu, 15 Jul 2021 10:33:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1626345201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eyovRM0UMtQZ4Oa4yMcIsUCbGJmgAfI2MIXkdQ11pOo=; b=aXfGJxvbTSic4fnSh0xk2t6YUJtIcem0rdoljN+eYD84oaQMcXBjUZBw8VdP6vCgees1XZ sXPDmtYdnAB3PkMiEHLZ4n7veUDJy9hMD0uAadVVq7bsESrMTuBpjk8lApvN+0JQWb8Geo AaLNe0kFxLpFttvt2MiVei4QSmVfNdw= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1626345201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eyovRM0UMtQZ4Oa4yMcIsUCbGJmgAfI2MIXkdQ11pOo=; b=5hKJdtek9TPXmcnN3HC6h+m/YrOsTr5zRYqYQUDUuRpxhBMd3Hs4kGItsrZ43PFeoe48GZ KuROtMdIDYr/9bDQ== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 9BA66A3B9A; Thu, 15 Jul 2021 10:33:21 +0000 (UTC) Date: Thu, 15 Jul 2021 12:33:21 +0200 Message-ID: From: Takashi Iwai To: Jia-Ju Bai Cc: perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org, "linux-kernel@vger.kernel.org >> linux-kernel" Subject: Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load() In-Reply-To: <7b0fcdaf-cd4f-4728-2eae-48c151a92e10@gmail.com> References: <7b0fcdaf-cd4f-4728-2eae-48c151a92e10@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, Takashi --- 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);