From patchwork Wed Jul 1 12:27:20 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Hills X-Patchwork-Id: 11636215 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 470E3913 for ; Wed, 1 Jul 2020 12:28:19 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CF6D520772 for ; Wed, 1 Jul 2020 12:28:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="gmCVm4ck"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="EpJPZmAz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CF6D520772 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xwax.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 2F826165E; Wed, 1 Jul 2020 14:27:29 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 2F826165E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1593606497; bh=pEFfkALKDSC8/x2HjIbpCn/sZsK6z3BdswLJCd1sAX0=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=gmCVm4ckfv77pj3m23QHTZUUQ6rVzkbsVKISKrOy1enGdmK9zKwcqafd8q4pGqZEM ox57I6ibob0LV4fr5coPl5j/1+Hxp1IqRypcjcPbSkKK8M77Tbr8ritA2vRc8v7dg4 c7dkBckSspOspaEEdj494TmnscmvXkHJYYlIgq4k= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id C5CD6F80217; Wed, 1 Jul 2020 14:27:28 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa1.perex.cz (Postfix, from userid 50401) id 444F9F80269; Wed, 1 Jul 2020 14:27:27 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on alsa1.perex.cz X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=disabled version=3.4.0 Received: from jazz.pogo.org.uk (jazz.pogo.org.uk [213.138.114.167]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 49CDBF800EA for ; Wed, 1 Jul 2020 14:27:24 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 49CDBF800EA Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="EpJPZmAz" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=pogo.org.uk ; s=a; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender: Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=EUMW/bEashtO586ffyz1ljJ1OZGveLvglHUaZik2x+I=; b=EpJPZmAzhe6SMDO5bK0F8PaYx7 lyLsaiPKc5jkB/QeHqaetCQ/OnH5nJh8kbHei8U+uhy3bbvq7O/3KI9SgFBEBkEejBLrMJeE75KNF yW5cdKYqBsJcUW4mTxwZvyMTxLqhtPntCnjiDev5/Ci7Z/YOuWN9gyU/QwMEx6mP0HZIiSV8G9QXN SwJ6SgdXJK/xj6FscdudK5JgQt1JC/GMy1DYooVLtjJQ4MeeqZpsDfrpzZAXHENQZWRnJLn32XMSb WlxeRbidbAiwr45+3TlFSpvwWbrX3Hu608itJEbruAdrEVcakgoL/u738+WO4g93H1lNZzjpkh1Ms jwfbrNWw==; Received: from cpc1-hari17-2-0-cust102.20-2.cable.virginm.net ([86.18.4.103] helo=stax.localdomain) by jazz.pogo.org.uk with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94 (FreeBSD)) (envelope-from ) id 1jqbpj-0009kN-Ns; Wed, 01 Jul 2020 13:27:23 +0100 Received: from mark by stax.localdomain with local (Exim 4.84) (envelope-from ) id 1jqbpj-0004dz-Es; Wed, 01 Jul 2020 13:27:23 +0100 From: Mark Hills To: Takashi Iwai Subject: [PATCH 1/4] echoaudio: Race conditions around "opencount" Date: Wed, 1 Jul 2020 13:27:20 +0100 Message-Id: <20200701122723.17814-1-mark@xwax.org> X-Mailer: git-send-email 2.17.5 In-Reply-To: <2007011319580.23012@tamla.localdomain> References: <2007011319580.23012@tamla.localdomain> Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" 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 Reported-by: kernel test robot Reported-by: kernel test robot --- 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 From patchwork Wed Jul 1 12:27:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Hills X-Patchwork-Id: 11636219 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id BAA5513B4 for ; Wed, 1 Jul 2020 12:29:10 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1336B20772 for ; Wed, 1 Jul 2020 12:29:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="DdrOOtO0"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="F5dsU4L8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1336B20772 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xwax.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 99D591680; Wed, 1 Jul 2020 14:28:20 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 99D591680 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1593606548; bh=u5d1J8cKt6uvUD2dTzndH3U1+JIoqPC0PqLevjmBynY=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=DdrOOtO0rMlLLYaDjNsO8Eha6v1BJUcy9v53jBj07Vvtgrx9go4UFPJ4KBtO+9OaA CvVuXm+Yz0hBndhFiPnoS7tl1jxeb9LrhOv11+oMJ0DkP469ofov/ElrnwI+NwYl+J urCJUcXrPz/jSEtkpXSgko8hGQf/i7EJnBBTDRlA= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 89834F802A9; Wed, 1 Jul 2020 14:27:31 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa1.perex.cz (Postfix, from userid 50401) id 23872F802A8; Wed, 1 Jul 2020 14:27:29 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on alsa1.perex.cz X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=disabled version=3.4.0 Received: from jazz.pogo.org.uk (jazz.pogo.org.uk [213.138.114.167]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id DA270F8021D for ; Wed, 1 Jul 2020 14:27:24 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz DA270F8021D Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="F5dsU4L8" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=pogo.org.uk ; s=a; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender: Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=jkfDC8SYLcRAPDATfR9VguvSs7UrKj28GPMRUIHZ24I=; b=F5dsU4L8+SoW68Oce/DPJDb40C GXJSTcOnF0VyUnO5NqsAMcoQuPyDYOJmG76qHV08JEbGkBPKwK7ftHesKEN9m8nJCT3+3wxdCkLPW R710lp9i+rHzHsSjZsMZHOJAAj5Dz6w1kegKrHYFW0Rqqse1l0S7ks2MZU3Hs3mjFgfSkNipd9O84 s/dnuJMo69b4vvtOteZHD1SX1jtTumgs7Kjj8fRiwsnP0vfvPbXQ18bmqJrUqreQizmsuzx/a7Q4I poazyWnf1/VtKYowmpOotPOuci/A0BYCvQwpGz4qjfWrqvAbR2xYYhF7JpS6um2BFt5uX1Z/DRXhv FCjdbS5g==; Received: from cpc1-hari17-2-0-cust102.20-2.cable.virginm.net ([86.18.4.103] helo=stax.localdomain) by jazz.pogo.org.uk with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94 (FreeBSD)) (envelope-from ) id 1jqbpj-0009kO-OH; Wed, 01 Jul 2020 13:27:24 +0100 Received: from mark by stax.localdomain with local (Exim 4.84) (envelope-from ) id 1jqbpj-0004e2-Fm; Wed, 01 Jul 2020 13:27:23 +0100 From: Mark Hills To: Takashi Iwai Subject: [PATCH 2/4] echoaudio: Prevent races in calls to set_audio_format() Date: Wed, 1 Jul 2020 13:27:21 +0100 Message-Id: <20200701122723.17814-2-mark@xwax.org> X-Mailer: git-send-email 2.17.5 In-Reply-To: <2007011319580.23012@tamla.localdomain> References: <2007011319580.23012@tamla.localdomain> Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" The function uses chip->comm_page which needs locking against other use at the same time. Signed-off-by: Mark Hills --- sound/pci/echoaudio/echoaudio.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c index 82a49dfd2384..19002d785d8d 100644 --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -711,9 +711,22 @@ static int pcm_prepare(struct snd_pcm_substream *substream) if (snd_BUG_ON(pipe_index >= px_num(chip))) return -EINVAL; - if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index))) + + /* + * We passed checks we can do independently; now take + * exclusive control + */ + + spin_lock_irq(&chip->lock); + + if (snd_BUG_ON(!is_pipe_allocated(chip, pipe_index))) { + spin_unlock(&chip->lock); return -EINVAL; + } + set_audio_format(chip, pipe_index, &format); + spin_unlock_irq(&chip->lock); + return 0; } From patchwork Wed Jul 1 12:27:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Hills X-Patchwork-Id: 11636217 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A9D4E913 for ; Wed, 1 Jul 2020 12:28:28 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 340BE20772 for ; Wed, 1 Jul 2020 12:28:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="B1E2d/WQ"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="dXT/Ivmg" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 340BE20772 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xwax.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id A23D8168D; Wed, 1 Jul 2020 14:27:38 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz A23D8168D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1593606506; bh=FC99/WFD8ZUmXhJeyNoFxnVBVzyoKbVLMTirzteEXiE=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=B1E2d/WQYtOMzmh5Zotx/itjWGjQqiqCkP9Md50ngK4qDAFf5XTJE2AAfyhoI0oq6 X8qbOLrbHax/z8KpHKqP6W1ZzTnmwchy+DB1tJ9oZnpMRIyLJItVyrgf0Jq7u7iJve RbqQevF3DV/6yzdtoHoexqyt0JzMKk3FnGNNuteU= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 5FBA7F802A8; Wed, 1 Jul 2020 14:27:30 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa1.perex.cz (Postfix, from userid 50401) id D3DA5F8025F; Wed, 1 Jul 2020 14:27:27 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on alsa1.perex.cz X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=disabled version=3.4.0 Received: from jazz.pogo.org.uk (jazz.pogo.org.uk [213.138.114.167]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id D0A80F8020C for ; Wed, 1 Jul 2020 14:27:24 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz D0A80F8020C Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="dXT/Ivmg" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=pogo.org.uk ; s=a; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender: Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=sPd/5mS+9ReK0t094NvzAo3atIxzy2bNU5BMQ3kT/pU=; b=dXT/IvmgGy/Pg665QUMSqwB5ex UOc0yz6IuvedYxv/YzgLEaxviupplPL/PJgbwdLZLP1dGo42tqqQE9yuk4ZiSvsobjXXXSSo6GvXs SEAk6dC/IZXj0224q7u8BZ/r4BjugMgyFXyMieqGtRHhYNHxdKYVPHX6Cn4yHQ+IXJl3yZyWTd+ck 7ODQrR0nKpc4yjpDrhtHmMBjgUZd5m9KEGDefO5vCKebNfYCpczZpL+7usOYdUJCRoWXKxJYMcdJT d1Yc9OI6LW0PKCaGrtQLyqU7J3VTdznquSQxqSHE//9fCJYzZavipxAkyhNGaYh2TaH6v14gjlkV1 qJsLf4/w==; Received: from cpc1-hari17-2-0-cust102.20-2.cable.virginm.net ([86.18.4.103] helo=stax.localdomain) by jazz.pogo.org.uk with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94 (FreeBSD)) (envelope-from ) id 1jqbpj-0009kP-O7; Wed, 01 Jul 2020 13:27:23 +0100 Received: from mark by stax.localdomain with local (Exim 4.84) (envelope-from ) id 1jqbpj-0004e7-GR; Wed, 01 Jul 2020 13:27:23 +0100 From: Mark Hills To: Takashi Iwai Subject: [PATCH 3/4] echoaudio: Prevent some noise on unloading the module Date: Wed, 1 Jul 2020 13:27:22 +0100 Message-Id: <20200701122723.17814-3-mark@xwax.org> X-Mailer: git-send-email 2.17.5 In-Reply-To: <2007011319580.23012@tamla.localdomain> References: <2007011319580.23012@tamla.localdomain> Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" These are valid conditions in normal circumstances, so do not "warn" but make them for debugging. Signed-off-by: Mark Hills --- sound/pci/echoaudio/echoaudio_dsp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sound/pci/echoaudio/echoaudio_dsp.c b/sound/pci/echoaudio/echoaudio_dsp.c index f02f5b1568de..d10d0e460f0b 100644 --- a/sound/pci/echoaudio/echoaudio_dsp.c +++ b/sound/pci/echoaudio/echoaudio_dsp.c @@ -898,7 +898,7 @@ static int pause_transport(struct echoaudio *chip, u32 channel_mask) return 0; } - dev_warn(chip->card->dev, "pause_transport: No pipes to stop!\n"); + dev_dbg(chip->card->dev, "pause_transport: No pipes to stop!\n"); return 0; } @@ -924,7 +924,7 @@ static int stop_transport(struct echoaudio *chip, u32 channel_mask) return 0; } - dev_warn(chip->card->dev, "stop_transport: No pipes to stop!\n"); + dev_dbg(chip->card->dev, "stop_transport: No pipes to stop!\n"); return 0; } From patchwork Wed Jul 1 12:27:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Hills X-Patchwork-Id: 11636221 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id C487A13B4 for ; Wed, 1 Jul 2020 12:29:47 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5ABE4206CB for ; Wed, 1 Jul 2020 12:29:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="SVZaO2pr"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="n2rW9K0i" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5ABE4206CB Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xwax.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id DD5A3165D; Wed, 1 Jul 2020 14:28:57 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz DD5A3165D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1593606585; bh=kP8uWaEB3VJAlh0u+aJCpRCLuMhg06u9xvjanTBUx+8=; h=From:To:Subject:Date:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=SVZaO2prmQqir3orV78K2ggbtcCcIV6hE/N4NI969gqkLWiepx40k6JPUvJUIHo3q VfhPbXW5c4m3T69aj7ircvjs9AHdXceWYBclLPsh/p+Rim3+H7ROiy9Lzvjym5DlH1 3/PqoZkKS30UyFUNl+RoRVygPRjI6aLXH9VC3CVA= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 5D441F802C4; Wed, 1 Jul 2020 14:27:34 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa1.perex.cz (Postfix, from userid 50401) id 8A4A1F802D2; Wed, 1 Jul 2020 14:27:32 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on alsa1.perex.cz X-Spam-Level: X-Spam-Status: No, score=0.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=disabled version=3.4.0 Received: from jazz.pogo.org.uk (jazz.pogo.org.uk [213.138.114.167]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id DC62AF80229 for ; Wed, 1 Jul 2020 14:27:24 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz DC62AF80229 Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key) header.d=pogo.org.uk header.i=@pogo.org.uk header.b="n2rW9K0i" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=pogo.org.uk ; s=a; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From:Sender: Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=E/CWv6nrcEMXhygJPwpRtRJqJoLaTLZMKsrYK5+oHO4=; b=n2rW9K0ikMjQw5l+w0M+lnfx0P mflfLA5mbjDk+jvYciuaFvoijYqjVuIbjYCnPue9okzfNtIy4Uuu6C6F9KTceIawShg6jbPmEx0n/ VcV4fBvCA1lHHWBNrsV3lC+E2ytMB8uxgxkUphPhcWTQXWRW4pp9ZyI0MhUrtW11b9fvXQNYHRaNS uhuNpkpWXiIgl+D/9ikv3WlBKxtiBoDE6iZbMoXaGrsYxdDKyWLkeE39XIwh0spxgj7WQ7diOUDZo o+/in7VySBF8uwp9WVoFESJOyxR9VehzjqOEdES5hcFyZg5LmXNrMUOpWWcqMZVqYTzspeaEv13mG mya3/Gjg==; Received: from cpc1-hari17-2-0-cust102.20-2.cable.virginm.net ([86.18.4.103] helo=stax.localdomain) by jazz.pogo.org.uk with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94 (FreeBSD)) (envelope-from ) id 1jqbpj-0009kQ-P4; Wed, 01 Jul 2020 13:27:23 +0100 Received: from mark by stax.localdomain with local (Exim 4.84) (envelope-from ) id 1jqbpj-0004eC-Gr; Wed, 01 Jul 2020 13:27:23 +0100 From: Mark Hills To: Takashi Iwai Subject: [PATCH 4/4] echoaudio: Address bugs in the interrupt handling Date: Wed, 1 Jul 2020 13:27:23 +0100 Message-Id: <20200701122723.17814-4-mark@xwax.org> X-Mailer: git-send-email 2.17.5 In-Reply-To: <2007011319580.23012@tamla.localdomain> References: <2007011319580.23012@tamla.localdomain> Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" Distorted audio appears occasionally, affecting either playback or capture and requiring the affected substream to be closed by all applications and re-opened. The best way I have found to reproduce the bug is to use dmix in combination with Chromium, which opens the audio device multiple times in threads. Anecdotally, the problems appear to have increased with faster CPUs. I ruled out 32-bit counter wrapping; it often happens much earlier. Since applying this patch I have not had problems, where previously they would occur several times a day. The patch targets the following issues: * Check for progress using the counter from the hardware, not after it has been truncated to the buffer. This is a clean way to address a possible bug where if a whole ringbuffer advances between interrupts, it goes unnoticed. * Move last_period state from chip to pipe This more logically belongs as part of pipe, and code is reasier to read if it is "counter position last time a period elapsed". Now the code has no references to period count. A period is just when the regular counter crosses a threshold. This increases readability and reduces scope for bugs. * Treat period notification and buffer advance independently: This helps to clarify what is the responsibility of the interrupt handler, and what is pcm_pointer(). Removing shared state between these operations means race conditions are fixed without introducing locks. Synchronisation is only around the read of pipe->dma_counter. There may be cache line contention around "struct audiopipe" but I did not have cause to profile this. Pay attention to be robust where dma_counter wrapping is not a multiple of period_size or buffer_size. This is a revised patch based on feedback from Takashi and Giuliano. Signed-off-by: Mark Hills --- sound/pci/echoaudio/echoaudio.c | 85 +++++++++++++++++++++++---------- sound/pci/echoaudio/echoaudio.h | 8 +++- 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c index 19002d785d8d..347e96038908 100644 --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -2,6 +2,7 @@ /* * ALSA driver for Echoaudio soundcards. * Copyright (C) 2003-2004 Giuliano Pochini + * Copyright (C) 2020 Mark Hills */ #include @@ -590,7 +591,7 @@ static int init_engine(struct snd_pcm_substream *substream, /* This stuff is used by the irq handler, so it must be * initialized before chip->substream */ - chip->last_period[pipe_index] = 0; + pipe->last_period = 0; pipe->last_counter = 0; pipe->position = 0; smp_wmb(); @@ -759,7 +760,7 @@ static int pcm_trigger(struct snd_pcm_substream *substream, int cmd) pipe = chip->substream[i]->runtime->private_data; switch (pipe->state) { case PIPE_STATE_STOPPED: - chip->last_period[i] = 0; + pipe->last_period = 0; pipe->last_counter = 0; pipe->position = 0; *pipe->dma_counter = 0; @@ -807,19 +808,26 @@ static snd_pcm_uframes_t pcm_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct audiopipe *pipe = runtime->private_data; - size_t cnt, bufsize, pos; + u32 counter, step; - cnt = le32_to_cpu(*pipe->dma_counter); - pipe->position += cnt - pipe->last_counter; - pipe->last_counter = cnt; - bufsize = substream->runtime->buffer_size; - pos = bytes_to_frames(substream->runtime, pipe->position); + /* + * IRQ handling runs concurrently. Do not share tracking of + * counter with it, which would race or require locking + */ - while (pos >= bufsize) { - pipe->position -= frames_to_bytes(substream->runtime, bufsize); - pos -= bufsize; - } - return pos; + counter = le32_to_cpu(*pipe->dma_counter); /* presumed atomic */ + + step = counter - pipe->last_counter; /* handles wrapping */ + pipe->last_counter = counter; + + /* counter doesn't neccessarily wrap on a multiple of + * buffer_size, so can't derive the position; must + * accumulate */ + + pipe->position += step; + pipe->position %= frames_to_bytes(runtime, runtime->buffer_size); /* wrap */ + + return bytes_to_frames(runtime, pipe->position); } @@ -1782,14 +1790,43 @@ static const struct snd_kcontrol_new snd_echo_channels_info = { /****************************************************************************** - IRQ Handler + IRQ Handling ******************************************************************************/ +/* Check if a period has elapsed since last interrupt + * + * Don't make any updates to state; PCM core handles this with the + * correct locks. + * + * \return true if a period has elapsed, otherwise false + */ +static bool period_has_elapsed(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct audiopipe *pipe = runtime->private_data; + u32 counter, step; + size_t period_bytes; + + if (pipe->state != PIPE_STATE_STARTED) + return false; + + period_bytes = frames_to_bytes(runtime, runtime->period_size); + + counter = le32_to_cpu(*pipe->dma_counter); /* presumed atomic */ + + step = counter - pipe->last_period; /* handles wrapping */ + step -= step % period_bytes; /* acknowledge whole periods only */ + + if (step == 0) + return false; /* haven't advanced a whole period yet */ + + pipe->last_period += step; /* used exclusively by us */ + return true; +} static irqreturn_t snd_echo_interrupt(int irq, void *dev_id) { struct echoaudio *chip = dev_id; - struct snd_pcm_substream *substream; - int period, ss, st; + int ss, st; spin_lock(&chip->lock); st = service_irq(chip); @@ -1800,17 +1837,13 @@ static irqreturn_t snd_echo_interrupt(int irq, void *dev_id) /* The hardware doesn't tell us which substream caused the irq, thus we have to check all running substreams. */ for (ss = 0; ss < DSP_MAXPIPES; ss++) { + struct snd_pcm_substream *substream; + substream = chip->substream[ss]; - if (substream && ((struct audiopipe *)substream->runtime-> - private_data)->state == PIPE_STATE_STARTED) { - period = pcm_pointer(substream) / - substream->runtime->period_size; - if (period != chip->last_period[ss]) { - chip->last_period[ss] = period; - spin_unlock(&chip->lock); - snd_pcm_period_elapsed(substream); - spin_lock(&chip->lock); - } + if (substream && period_has_elapsed(substream)) { + spin_unlock(&chip->lock); + snd_pcm_period_elapsed(substream); + spin_lock(&chip->lock); } } spin_unlock(&chip->lock); diff --git a/sound/pci/echoaudio/echoaudio.h b/sound/pci/echoaudio/echoaudio.h index 6fd283e4e676..30c640931f1e 100644 --- a/sound/pci/echoaudio/echoaudio.h +++ b/sound/pci/echoaudio/echoaudio.h @@ -298,7 +298,12 @@ struct audiopipe { * the current dma position * (lower 32 bits only) */ - u32 last_counter; /* The last position, which is used + u32 last_period; /* Counter position last time a + * period elapsed + */ + u32 last_counter; /* Used exclusively by pcm_pointer + * under PCM core locks. + * The last position, which is used * to compute... */ u32 position; /* ...the number of bytes tranferred @@ -332,7 +337,6 @@ struct audioformat { struct echoaudio { spinlock_t lock; struct snd_pcm_substream *substream[DSP_MAXPIPES]; - int last_period[DSP_MAXPIPES]; struct mutex mode_mutex; u16 num_digital_modes, digital_mode_list[6]; u16 num_clock_sources, clock_source_list[10];