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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C99A6C433F5 for ; Wed, 17 Nov 2021 21:02:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A6BD061B31 for ; Wed, 17 Nov 2021 21:02:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239971AbhKQVFY (ORCPT ); Wed, 17 Nov 2021 16:05:24 -0500 Received: from smtp-out2.suse.de ([195.135.220.29]:38432 "EHLO smtp-out2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237879AbhKQVFX (ORCPT ); Wed, 17 Nov 2021 16:05:23 -0500 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id C1D001FD29; Wed, 17 Nov 2021 21:02:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1637182943; 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=f0dTyNA6tM9d482fDoLe3J6N6rsxYoJzYFp/3XEYQmc=; b=qZtDiXgODB3bu0olnFOEtBN6nSbdCrouXjZMmjeBL2LAUlnv6gtoOX5KrwTZqRwUt5gI9A E59O3l9gI8vKTuyzN4LFbLD26nR+nBHNjlSlMRv8CMGzA4TRiV+JiT+9X3J//86sGCvYZh MmnB3pJ608zW2aJYfwoaorFIstMynno= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1637182943; 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=f0dTyNA6tM9d482fDoLe3J6N6rsxYoJzYFp/3XEYQmc=; b=nDnnEbpTK4b21fHx7ls3/Hrl9aCPA5bGRQYFWU5UjGu3Wdtnt3+XznY+0cXna9TTh213um wmOtD8qlNGZea+Bw== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id B86B4A3B83; Wed, 17 Nov 2021 21:02:23 +0000 (UTC) Date: Wed, 17 Nov 2021 22:02:23 +0100 Message-ID: From: Takashi Iwai To: Alex Elder Cc: Takashi Iwai , greybus-dev@lists.linaro.org, Alex Elder , Johan Hovold , linux-kernel@vger.kernel.org Subject: Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around snd_ctl_remove() calls In-Reply-To: <07e228eb-676a-bdb1-c2ec-a96f691f5a18@linaro.org> References: <20211116072027.18466-1-tiwai@suse.de> <07e228eb-676a-bdb1-c2ec-a96f691f5a18@linaro.org> 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 Wed, 17 Nov 2021 20:56:14 +0100, Alex Elder wrote: > > On 11/16/21 1:20 AM, Takashi Iwai wrote: > > snd_ctl_remove() has to be called with card->controls_rwsem held (when > > called after the card instantiation). This patch adds the missing > > rwsem calls around it. > > I see the comment above snd_ctl_remove() that says you must hold > the write lock. And given that, this seems correct to me. > > I understand why you want to take the lock just once, rather > than each time snd_ctl_remove() is called. > > However I believe the acquisition and release of the lock > belongs inside gbaudio_remove_controls(), not in its caller. > > If you disagree, can you please explain why? In general if the function returns an error and has a loop inside, taking a lock in the caller side avoids the forgotten unlock. Takashi > Otherwise, will you please submit version two, taking the > lock inside gbaudio_remove_controls()? > > Thanks. > > -Alex > > > Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules") > > Signed-off-by: Takashi Iwai > > --- > > drivers/staging/greybus/audio_helper.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c > > index 1ed4772d2771..843760675876 100644 > > --- a/drivers/staging/greybus/audio_helper.c > > +++ b/drivers/staging/greybus/audio_helper.c > > @@ -192,7 +192,11 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component, > > unsigned int num_controls) > > { > > struct snd_card *card = component->card->snd_card; > > + int err; > > - return gbaudio_remove_controls(card, component->dev, controls, > > - num_controls, component->name_prefix); > > + down_write(&card->controls_rwsem); > > + err = gbaudio_remove_controls(card, component->dev, controls, > > + num_controls, component->name_prefix); > > + up_write(&card->controls_rwsem); > > + return err; > > } > > >