MIME-Version: 1.0
References: <20211116072027.18466-1-tiwai@suse.de> <07e228eb-676a-bdb1-c2ec-a96f691f5a18@linaro.org>
 <s5h5ysqlcm8.wl-tiwai@suse.de> <b0d2b643-214a-07c5-96b7-4845c54cba72@linaro.org>
In-Reply-To: <b0d2b643-214a-07c5-96b7-4845c54cba72@linaro.org>
From:   Vaibhav Agarwal <vaibhav.sr@gmail.com>
Date:   Thu, 18 Nov 2021 09:02:40 +0530
Message-ID: <CAAs364-thiqACMRwT++JMxkMd7qa_stFApjK-4SmrEJetX6oyw@mail.gmail.com>
Subject: Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around
 snd_ctl_remove() calls
To:     Alex Elder <elder@linaro.org>
Cc:     Takashi Iwai <tiwai@suse.de>,
        "moderated list:GREYBUS SUBSYSTEM" <greybus-dev@lists.linaro.org>,
        Alex Elder <elder@kernel.org>, Johan Hovold <johan@kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 18, 2021 at 3:25 AM Alex Elder <elder@linaro.org> wrote:
>
> On 11/17/21 3:02 PM, Takashi Iwai wrote:
> > 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.
>
> But taking the lock in the called function makes the
> caller not need to take the lock (which would be even
> more valuable if there were more than one caller).
>
> I prefer having the lock acquisition in the called
> function.  Please send version 2, as I suggested.


Hi Takashi,

Thanks for sharing this patch. In reference to the suggestion from Alex,
do you think replacing snd_ctl_find_id(), snd_ctl_remove() with
snd_ctl_remove_id() inside gbaudio_remove_controls() would be an even
better choice without worrying about locks?

--
vaibhav

 >
>
>                                         -Alex
>
> > 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 <tiwai@suse.de>
> >>> ---
> >>>    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;
> >>>    }
> >>>
> >>
>
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev