Message-ID: <b0d2b643-214a-07c5-96b7-4845c54cba72@linaro.org>
Date:   Wed, 17 Nov 2021 15:55:06 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [greybus-dev] [PATCH] staging: greybus: Add missing rwsem around
 snd_ctl_remove() calls
Content-Language: en-US
To:     Takashi Iwai <tiwai@suse.de>
Cc:     greybus-dev@lists.linaro.org, Alex Elder <elder@kernel.org>,
        Johan Hovold <johan@kernel.org>, linux-kernel@vger.kernel.org
References: <20211116072027.18466-1-tiwai@suse.de>
 <07e228eb-676a-bdb1-c2ec-a96f691f5a18@linaro.org>
 <s5h5ysqlcm8.wl-tiwai@suse.de>
From:   Alex Elder <elder@linaro.org>
In-Reply-To: <s5h5ysqlcm8.wl-tiwai@suse.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.

					-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;
>>>    }
>>>
>>