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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0B34BC28CF5 for ; Wed, 26 Jan 2022 15:24:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242777AbiAZPYh (ORCPT ); Wed, 26 Jan 2022 10:24:37 -0500 Received: from smtp-out2.suse.de ([195.135.220.29]:37366 "EHLO smtp-out2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236079AbiAZPYh (ORCPT ); Wed, 26 Jan 2022 10:24:37 -0500 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 343C01F3B0; Wed, 26 Jan 2022 15:24:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1643210676; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9CXY7MFbJ1saOKK9+Yf3pb9u+zx6DrRm92wfkukypJU=; b=j9C4LuPOb5XoFVk6uxHIO8WC5JbJfuf9jmV9L6Kx2lRSXZEqcFPNULeh0IL+8Klm088Mhv VVABBZSIxItC72DaJhTo4TIEe8RAPpl51T+Jup4gOlFOoA78p9Rg2std/7tu5wRVElYPkj 5rM1DiS2T2ol6/t79eDHWxsAlWOJRfA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1643210676; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9CXY7MFbJ1saOKK9+Yf3pb9u+zx6DrRm92wfkukypJU=; b=Zcv47eTlQd7aDCA72boLRhzkKoNf/ZcDAsLb7bnW2iDfHVLc2QbqnzOKDYdAl7axDb5SQH b6sfpqO/K/rKaIBA== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 1D799A3B81; Wed, 26 Jan 2022 15:24:36 +0000 (UTC) Date: Wed, 26 Jan 2022 16:24:36 +0100 Message-ID: From: Takashi Iwai To: Alexander Sergeyev Cc: Jeremy Szu , tiwai@suse.com, "moderated list:SOUND" , Kailang Yang , open list , Huacai Chen , Jian-Hong Pan , Hui Wang , PeiSen Hou Subject: Re: [PATCH 1/4] ALSA: hda/realtek: fix mute/micmute LEDs for HP 855 G8 In-Reply-To: <20220122205637.7gzurdu7xl4sthxw@localhost.localdomain> References: <20220112201824.qmphnz2hx4frda6e@localhost.localdomain> <20220113183141.kla37mbqmo4x6wxp@localhost.localdomain> <20220114183720.n46wealclg6spxkp@localhost.localdomain> <20220115152215.kprws5nja2i43qax@localhost.localdomain> <20220119093249.eaxem33bjqjxcher@localhost.localdomain> <20220122190522.ycaygrqcen7d3hj2@localhost.localdomain> <20220122205637.7gzurdu7xl4sthxw@localhost.localdomain> 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=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 22 Jan 2022 21:56:37 +0100, Alexander Sergeyev wrote: > > On Sat, Jan 22, 2022 at 10:05:24PM +0300, Alexander Sergeyev wrote: > > I'm not sure about kernel log buffering or maybe the device support for > > pipelining, but is it okay that alc_update_coefex_idx() seem to overlap? > > Given that the CPU number is the same in alc_update_coefex_idx(), it seems > these calls execution is interrupted and interleaved on the same core. > > And, actually, there are two LEDs to set (mute and micmute). Am I onto > something here? That's an interesting finding, and yes, such a race is quite possible. Below is a quick fix as an attempt to cover it. Could you give it a try? BTW, the fix for the unbind problem was submitted. It's a slightly more simplified version than what I posted here beforehand. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda: realtek: Fix race at concurrent COEF updates The COEF access is done with two steps: setting the index then read or write the data. When multiple COEF accesses are performed concurrently, the index and data might be paired unexpectedly. In most cases, this isn't a big problem as the COEF setup is done at the initialization, but some dynamic changes like the mute LED may hit such a race. For avoiding the racy COEF accesses, this patch introduces a new mutex coef_mutex to alc_spec, and wrap the COEF accessing functions with it. Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_realtek.c | 61 ++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 668274e52674..a5677be0a405 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -98,6 +98,7 @@ struct alc_spec { unsigned int gpio_mic_led_mask; struct alc_coef_led mute_led_coef; struct alc_coef_led mic_led_coef; + struct mutex coef_mutex; hda_nid_t headset_mic_pin; hda_nid_t headphone_mic_pin; @@ -137,8 +138,8 @@ struct alc_spec { * COEF access helper functions */ -static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, - unsigned int coef_idx) +static int __alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx) { unsigned int val; @@ -147,28 +148,61 @@ static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, return val; } +static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx) +{ + struct alc_spec *spec = codec->spec; + unsigned int val; + + mutex_lock(&spec->coef_mutex); + val = __alc_read_coefex_idx(codec, nid, coef_idx); + mutex_unlock(&spec->coef_mutex); + return val; +} + #define alc_read_coef_idx(codec, coef_idx) \ alc_read_coefex_idx(codec, 0x20, coef_idx) -static void alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, - unsigned int coef_idx, unsigned int coef_val) +static void __alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int coef_val) { snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_COEF_INDEX, coef_idx); snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_PROC_COEF, coef_val); } +static void alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int coef_val) +{ + struct alc_spec *spec = codec->spec; + + mutex_lock(&spec->coef_mutex); + __alc_write_coefex_idx(codec, nid, coef_idx, coef_val); + mutex_unlock(&spec->coef_mutex); +} + #define alc_write_coef_idx(codec, coef_idx, coef_val) \ alc_write_coefex_idx(codec, 0x20, coef_idx, coef_val) +static void __alc_update_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int mask, + unsigned int bits_set) +{ + unsigned int val = __alc_read_coefex_idx(codec, nid, coef_idx); + + if (val != -1) + __alc_write_coefex_idx(codec, nid, coef_idx, + (val & ~mask) | bits_set); +} + static void alc_update_coefex_idx(struct hda_codec *codec, hda_nid_t nid, unsigned int coef_idx, unsigned int mask, unsigned int bits_set) { - unsigned int val = alc_read_coefex_idx(codec, nid, coef_idx); + struct alc_spec *spec = codec->spec; - if (val != -1) - alc_write_coefex_idx(codec, nid, coef_idx, - (val & ~mask) | bits_set); + mutex_lock(&spec->coef_mutex); + __alc_update_coefex_idx(codec, nid, coef_idx, mask, bits_set); + mutex_unlock(&spec->coef_mutex); } #define alc_update_coef_idx(codec, coef_idx, mask, bits_set) \ @@ -201,13 +235,17 @@ struct coef_fw { static void alc_process_coef_fw(struct hda_codec *codec, const struct coef_fw *fw) { + struct alc_spec *spec = codec->spec; + + mutex_lock(&spec->coef_mutex); for (; fw->nid; fw++) { if (fw->mask == (unsigned short)-1) - alc_write_coefex_idx(codec, fw->nid, fw->idx, fw->val); + __alc_write_coefex_idx(codec, fw->nid, fw->idx, fw->val); else - alc_update_coefex_idx(codec, fw->nid, fw->idx, - fw->mask, fw->val); + __alc_update_coefex_idx(codec, fw->nid, fw->idx, + fw->mask, fw->val); } + mutex_unlock(&spec->coef_mutex); } /* @@ -1153,6 +1191,7 @@ static int alc_alloc_spec(struct hda_codec *codec, hda_nid_t mixer_nid) codec->spdif_status_reset = 1; codec->forced_resume = 1; codec->patch_ops = alc_patch_ops; + mutex_init(&spec->coef_mutex); err = alc_codec_rename_from_preset(codec); if (err < 0) { -- 2.31.1 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 3682DC28CF5 for ; Wed, 26 Jan 2022 15:25:38 +0000 (UTC) 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 DB3EA1B53; Wed, 26 Jan 2022 16:24:45 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz DB3EA1B53 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1643210735; bh=hZrDXnAPEqekc+aCczkWOeVJ3QOSLSD9EN0EMfKoOmw=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=U7GPal+5XoWl+G+3M6aVyrrjsEmWzhiyd09GSMkWO7/y+qz/4VzfCQSfWp6Y6xvv6 G+KGagxDgG3NSl58LXqPLIlbpHC0/yYwzpm3UO+PvMK2QsY6wQ8N8t0qzF/AyOVkOl atdbmYUn2X/OmzlT27ts+lym0Q0wumaT9rRrqoOw= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 78FF0F8047D; Wed, 26 Jan 2022 16:24:45 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id E7B4AF8049C; Wed, 26 Jan 2022 16:24:42 +0100 (CET) Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id DF155F800C8 for ; Wed, 26 Jan 2022 16:24:36 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz DF155F800C8 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="j9C4LuPO"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="Zcv47eTl" Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 343C01F3B0; Wed, 26 Jan 2022 15:24:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1643210676; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9CXY7MFbJ1saOKK9+Yf3pb9u+zx6DrRm92wfkukypJU=; b=j9C4LuPOb5XoFVk6uxHIO8WC5JbJfuf9jmV9L6Kx2lRSXZEqcFPNULeh0IL+8Klm088Mhv VVABBZSIxItC72DaJhTo4TIEe8RAPpl51T+Jup4gOlFOoA78p9Rg2std/7tu5wRVElYPkj 5rM1DiS2T2ol6/t79eDHWxsAlWOJRfA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1643210676; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9CXY7MFbJ1saOKK9+Yf3pb9u+zx6DrRm92wfkukypJU=; b=Zcv47eTlQd7aDCA72boLRhzkKoNf/ZcDAsLb7bnW2iDfHVLc2QbqnzOKDYdAl7axDb5SQH b6sfpqO/K/rKaIBA== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 1D799A3B81; Wed, 26 Jan 2022 15:24:36 +0000 (UTC) Date: Wed, 26 Jan 2022 16:24:36 +0100 Message-ID: From: Takashi Iwai To: Alexander Sergeyev Subject: Re: [PATCH 1/4] ALSA: hda/realtek: fix mute/micmute LEDs for HP 855 G8 In-Reply-To: <20220122205637.7gzurdu7xl4sthxw@localhost.localdomain> References: <20220112201824.qmphnz2hx4frda6e@localhost.localdomain> <20220113183141.kla37mbqmo4x6wxp@localhost.localdomain> <20220114183720.n46wealclg6spxkp@localhost.localdomain> <20220115152215.kprws5nja2i43qax@localhost.localdomain> <20220119093249.eaxem33bjqjxcher@localhost.localdomain> <20220122190522.ycaygrqcen7d3hj2@localhost.localdomain> <20220122205637.7gzurdu7xl4sthxw@localhost.localdomain> 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=US-ASCII Cc: "moderated list:SOUND" , Kailang Yang , Jeremy Szu , Huacai Chen , open list , tiwai@suse.com, Hui Wang , PeiSen Hou , Jian-Hong Pan 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" On Sat, 22 Jan 2022 21:56:37 +0100, Alexander Sergeyev wrote: > > On Sat, Jan 22, 2022 at 10:05:24PM +0300, Alexander Sergeyev wrote: > > I'm not sure about kernel log buffering or maybe the device support for > > pipelining, but is it okay that alc_update_coefex_idx() seem to overlap? > > Given that the CPU number is the same in alc_update_coefex_idx(), it seems > these calls execution is interrupted and interleaved on the same core. > > And, actually, there are two LEDs to set (mute and micmute). Am I onto > something here? That's an interesting finding, and yes, such a race is quite possible. Below is a quick fix as an attempt to cover it. Could you give it a try? BTW, the fix for the unbind problem was submitted. It's a slightly more simplified version than what I posted here beforehand. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda: realtek: Fix race at concurrent COEF updates The COEF access is done with two steps: setting the index then read or write the data. When multiple COEF accesses are performed concurrently, the index and data might be paired unexpectedly. In most cases, this isn't a big problem as the COEF setup is done at the initialization, but some dynamic changes like the mute LED may hit such a race. For avoiding the racy COEF accesses, this patch introduces a new mutex coef_mutex to alc_spec, and wrap the COEF accessing functions with it. Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_realtek.c | 61 ++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 668274e52674..a5677be0a405 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -98,6 +98,7 @@ struct alc_spec { unsigned int gpio_mic_led_mask; struct alc_coef_led mute_led_coef; struct alc_coef_led mic_led_coef; + struct mutex coef_mutex; hda_nid_t headset_mic_pin; hda_nid_t headphone_mic_pin; @@ -137,8 +138,8 @@ struct alc_spec { * COEF access helper functions */ -static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, - unsigned int coef_idx) +static int __alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx) { unsigned int val; @@ -147,28 +148,61 @@ static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, return val; } +static int alc_read_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx) +{ + struct alc_spec *spec = codec->spec; + unsigned int val; + + mutex_lock(&spec->coef_mutex); + val = __alc_read_coefex_idx(codec, nid, coef_idx); + mutex_unlock(&spec->coef_mutex); + return val; +} + #define alc_read_coef_idx(codec, coef_idx) \ alc_read_coefex_idx(codec, 0x20, coef_idx) -static void alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, - unsigned int coef_idx, unsigned int coef_val) +static void __alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int coef_val) { snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_COEF_INDEX, coef_idx); snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_PROC_COEF, coef_val); } +static void alc_write_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int coef_val) +{ + struct alc_spec *spec = codec->spec; + + mutex_lock(&spec->coef_mutex); + __alc_write_coefex_idx(codec, nid, coef_idx, coef_val); + mutex_unlock(&spec->coef_mutex); +} + #define alc_write_coef_idx(codec, coef_idx, coef_val) \ alc_write_coefex_idx(codec, 0x20, coef_idx, coef_val) +static void __alc_update_coefex_idx(struct hda_codec *codec, hda_nid_t nid, + unsigned int coef_idx, unsigned int mask, + unsigned int bits_set) +{ + unsigned int val = __alc_read_coefex_idx(codec, nid, coef_idx); + + if (val != -1) + __alc_write_coefex_idx(codec, nid, coef_idx, + (val & ~mask) | bits_set); +} + static void alc_update_coefex_idx(struct hda_codec *codec, hda_nid_t nid, unsigned int coef_idx, unsigned int mask, unsigned int bits_set) { - unsigned int val = alc_read_coefex_idx(codec, nid, coef_idx); + struct alc_spec *spec = codec->spec; - if (val != -1) - alc_write_coefex_idx(codec, nid, coef_idx, - (val & ~mask) | bits_set); + mutex_lock(&spec->coef_mutex); + __alc_update_coefex_idx(codec, nid, coef_idx, mask, bits_set); + mutex_unlock(&spec->coef_mutex); } #define alc_update_coef_idx(codec, coef_idx, mask, bits_set) \ @@ -201,13 +235,17 @@ struct coef_fw { static void alc_process_coef_fw(struct hda_codec *codec, const struct coef_fw *fw) { + struct alc_spec *spec = codec->spec; + + mutex_lock(&spec->coef_mutex); for (; fw->nid; fw++) { if (fw->mask == (unsigned short)-1) - alc_write_coefex_idx(codec, fw->nid, fw->idx, fw->val); + __alc_write_coefex_idx(codec, fw->nid, fw->idx, fw->val); else - alc_update_coefex_idx(codec, fw->nid, fw->idx, - fw->mask, fw->val); + __alc_update_coefex_idx(codec, fw->nid, fw->idx, + fw->mask, fw->val); } + mutex_unlock(&spec->coef_mutex); } /* @@ -1153,6 +1191,7 @@ static int alc_alloc_spec(struct hda_codec *codec, hda_nid_t mixer_nid) codec->spdif_status_reset = 1; codec->forced_resume = 1; codec->patch_ops = alc_patch_ops; + mutex_init(&spec->coef_mutex); err = alc_codec_rename_from_preset(codec); if (err < 0) { -- 2.31.1