Received-SPF: pass (google.com: domain of linux-kernel+bounces-93929-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <fedf6e86-ca14-4236-85c0-64205c63d7f9@opensource.cirrus.com>
Date: Wed, 6 Mar 2024 12:54:43 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ASoC: soc-card: Fix missing locking in
 snd_soc_card_get_kcontrol()
To: Takashi Iwai <tiwai@suse.de>
CC: <broonie@kernel.org>, <shengjiu.wang@gmail.com>, <Xiubo.Lee@gmail.com>,
        <linux-sound@vger.kernel.org>, <alsa-devel@alsa-project.org>,
        <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>,
        <patches@opensource.cirrus.com>
References: <20240221123710.690224-1-rf@opensource.cirrus.com>
 <878r33hf81.wl-tiwai@suse.de>
Content-Language: en-US
From: Richard Fitzgerald <rf@opensource.cirrus.com>
In-Reply-To: <878r33hf81.wl-tiwai@suse.de>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 29/2/24 08:00, Takashi Iwai wrote:
> On Wed, 21 Feb 2024 13:37:10 +0100,
> Richard Fitzgerald wrote:
>>
>> snd_soc_card_get_kcontrol() must be holding a read lock on
>> card->controls_rwsem while walking the controls list.
>>
>> Compare with snd_ctl_find_numid().
>>
>> The existing function is renamed snd_soc_card_get_kcontrol_locked()
>> so that it can be called from contexts that are already holding
>> card->controls_rwsem (for example, control get/put functions).
>>
>> There are few direct or indirect callers of
>> snd_soc_card_get_kcontrol(), and most are safe. Three require
>> changes, which have been included in this patch:
>>
>> codecs/cs35l45.c:
>>    cs35l45_activate_ctl() is called from a control put() function so
>>    is changed to call snd_soc_card_get_kcontrol_locked().
>>
>> codecs/cs35l56.c:
>>    cs35l56_sync_asp1_mixer_widgets_with_firmware() is called from
>>    control get()/put() functions so is changed to call
>>    snd_soc_card_get_kcontrol_locked().
>>
>> fsl/fsl_xcvr.c:
>>    fsl_xcvr_activate_ctl() is called from three places, one of which
>>    already holds card->controls_rwsem:
>>    1. fsl_xcvr_mode_put(), a control put function, which will
>>       already be holding card->controls_rwsem.
>>    2. fsl_xcvr_startup(), a DAI startup function.
>>    3. fsl_xcvr_shutdown(), a DAI shutdown function.
>>
>>    To fix this, fsl_xcvr_activate_ctl() has been changed to call
>>    snd_soc_card_get_kcontrol_locked() so that it is safe to call
>>    directly from fsl_xcvr_mode_put().
>>    The fsl_xcvr_startup() and fsl_xcvr_shutdown() functions have been
>>    changed to take a read lock on card->controls_rsem() around calls
>>    to fsl_xcvr_activate_ctl(). While this is not very elegant, it
>>    keeps the change small, to avoid this patch creating a large
>>    collateral churn in fsl/fsl_xcvr.c.
>>
>> Analysis of other callers of snd_soc_card_get_kcontrol() is that
>> they do not need any changes, they are not holding card->controls_rwsem
>> when they call snd_soc_card_get_kcontrol().
>>
>> Direct callers of snd_soc_card_get_kcontrol():
>>    fsl/fsl_spdif.c: fsl_spdif_dai_probe() - DAI probe function
>>    fsl/fsl_micfil.c: voice_detected_fn() - IRQ handler
>>
>> Indirect callers via soc_component_notify_control():
>>    codecs/cs42l43: cs42l43_mic_shutter() - IRQ handler
>>    codecs/cs42l43: cs42l43_spk_shutter() - IRQ handler
>>    codecs/ak4118.c: ak4118_irq_handler() - IRQ handler
>>    codecs/wm_adsp.c: wm_adsp_write_ctl() - not currently used
>>
>> Indirect callers via snd_soc_limit_volume():
>>    qcom/sc8280xp.c: sc8280xp_snd_init() - DAIlink init function
>>    ti/rx51.c: rx51_aic34_init() - DAI init function
>>
>> I don't have hardware to test the fsl/*, qcom/sc828xp.c, ti/rx51.c
>> and ak4118.c changes.
>>
>> Backport note:
>> The fsl/, qcom/, cs35l45, cs35l56 and cs42l43 callers were added
>> since the Fixes commit so won't all be present on older kernels.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> Fixes: 209c6cdfd283 ("ASoC: soc-card: move snd_soc_card_get_kcontrol() to soc-card")
>> ---
>> It would be great if people could test the fsl/, qcom/, ti/rx51 and ak4418
>> drivers.
> 
> This fix itself looks correct, and I merged Mark's PR now.
> 
> But in general, it'd be better to use snd_ctl_find_id() and
> snd_ctl_find_id_unlocked() if possible.  Those standard APIs can use
> the fast Xarray lookup, and especially for the case like many ASoC
> drivers that expose hundreds of kcontrols, the performance gain
> becomes significant.
> 
> I see that there is no snd_ctl_find_mixer_id_unlocked() variant, but
> it should be trivial to add.
> 
> 

Yes, I'll have a look at that. I was thinking that it would be good
to have all the code to find controls in one place, instead of a special
case for ASoC. But I decided to make the bugfix with minimum changes.

> thanks,
> 
> Takashi