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 <[email protected]>
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.
---
include/sound/soc-card.h | 2 ++
sound/soc/codecs/cs35l45.c | 2 +-
sound/soc/codecs/cs35l56.c | 2 +-
sound/soc/fsl/fsl_xcvr.c | 12 +++++++++++-
sound/soc/soc-card.c | 24 ++++++++++++++++++++++--
5 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc-card.h b/include/sound/soc-card.h
index ecc02e955279..1f4c39922d82 100644
--- a/include/sound/soc-card.h
+++ b/include/sound/soc-card.h
@@ -30,6 +30,8 @@ static inline void snd_soc_card_mutex_unlock(struct snd_soc_card *card)
struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card,
const char *name);
+struct snd_kcontrol *snd_soc_card_get_kcontrol_locked(struct snd_soc_card *soc_card,
+ const char *name);
int snd_soc_card_jack_new(struct snd_soc_card *card, const char *id, int type,
struct snd_soc_jack *jack);
int snd_soc_card_jack_new_pins(struct snd_soc_card *card, const char *id,
diff --git a/sound/soc/codecs/cs35l45.c b/sound/soc/codecs/cs35l45.c
index 44c221745c3b..2392c6effed8 100644
--- a/sound/soc/codecs/cs35l45.c
+++ b/sound/soc/codecs/cs35l45.c
@@ -184,7 +184,7 @@ static int cs35l45_activate_ctl(struct snd_soc_component *component,
else
snprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s", ctl_name);
- kcontrol = snd_soc_card_get_kcontrol(component->card, name);
+ kcontrol = snd_soc_card_get_kcontrol_locked(component->card, name);
if (!kcontrol) {
dev_err(component->dev, "Can't find kcontrol %s\n", name);
return -EINVAL;
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 2c1313e34cce..6dd0319bc843 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -114,7 +114,7 @@ static int cs35l56_sync_asp1_mixer_widgets_with_firmware(struct cs35l56_private
name = full_name;
}
- kcontrol = snd_soc_card_get_kcontrol(dapm->card, name);
+ kcontrol = snd_soc_card_get_kcontrol_locked(dapm->card, name);
if (!kcontrol) {
dev_warn(cs35l56->base.dev, "Could not find control %s\n", name);
continue;
diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
index f0fb33d719c2..c46f64557a7f 100644
--- a/sound/soc/fsl/fsl_xcvr.c
+++ b/sound/soc/fsl/fsl_xcvr.c
@@ -174,7 +174,9 @@ static int fsl_xcvr_activate_ctl(struct snd_soc_dai *dai, const char *name,
struct snd_kcontrol *kctl;
bool enabled;
- kctl = snd_soc_card_get_kcontrol(card, name);
+ lockdep_assert_held(&card->snd_card->controls_rwsem);
+
+ kctl = snd_soc_card_get_kcontrol_locked(card, name);
if (kctl == NULL)
return -ENOENT;
@@ -576,10 +578,14 @@ static int fsl_xcvr_startup(struct snd_pcm_substream *substream,
xcvr->streams |= BIT(substream->stream);
if (!xcvr->soc_data->spdif_only) {
+ struct snd_soc_card *card = dai->component->card;
+
/* Disable XCVR controls if there is stream started */
+ down_read(&card->snd_card->controls_rwsem);
fsl_xcvr_activate_ctl(dai, fsl_xcvr_mode_kctl.name, false);
fsl_xcvr_activate_ctl(dai, fsl_xcvr_arc_mode_kctl.name, false);
fsl_xcvr_activate_ctl(dai, fsl_xcvr_earc_capds_kctl.name, false);
+ up_read(&card->snd_card->controls_rwsem);
}
return 0;
@@ -598,11 +604,15 @@ static void fsl_xcvr_shutdown(struct snd_pcm_substream *substream,
/* Enable XCVR controls if there is no stream started */
if (!xcvr->streams) {
if (!xcvr->soc_data->spdif_only) {
+ struct snd_soc_card *card = dai->component->card;
+
+ down_read(&card->snd_card->controls_rwsem);
fsl_xcvr_activate_ctl(dai, fsl_xcvr_mode_kctl.name, true);
fsl_xcvr_activate_ctl(dai, fsl_xcvr_arc_mode_kctl.name,
(xcvr->mode == FSL_XCVR_MODE_ARC));
fsl_xcvr_activate_ctl(dai, fsl_xcvr_earc_capds_kctl.name,
(xcvr->mode == FSL_XCVR_MODE_EARC));
+ up_read(&card->snd_card->controls_rwsem);
}
ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0,
FSL_XCVR_IRQ_EARC_ALL, 0);
diff --git a/sound/soc/soc-card.c b/sound/soc/soc-card.c
index 285ab4c9c716..8a2f163da6bc 100644
--- a/sound/soc/soc-card.c
+++ b/sound/soc/soc-card.c
@@ -5,6 +5,9 @@
// Copyright (C) 2019 Renesas Electronics Corp.
// Kuninori Morimoto <[email protected]>
//
+
+#include <linux/lockdep.h>
+#include <linux/rwsem.h>
#include <sound/soc.h>
#include <sound/jack.h>
@@ -26,12 +29,15 @@ static inline int _soc_card_ret(struct snd_soc_card *card,
return ret;
}
-struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card,
- const char *name)
+struct snd_kcontrol *snd_soc_card_get_kcontrol_locked(struct snd_soc_card *soc_card,
+ const char *name)
{
struct snd_card *card = soc_card->snd_card;
struct snd_kcontrol *kctl;
+ /* must be held read or write */
+ lockdep_assert_held(&card->controls_rwsem);
+
if (unlikely(!name))
return NULL;
@@ -40,6 +46,20 @@ struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card,
return kctl;
return NULL;
}
+EXPORT_SYMBOL_GPL(snd_soc_card_get_kcontrol_locked);
+
+struct snd_kcontrol *snd_soc_card_get_kcontrol(struct snd_soc_card *soc_card,
+ const char *name)
+{
+ struct snd_card *card = soc_card->snd_card;
+ struct snd_kcontrol *kctl;
+
+ down_read(&card->controls_rwsem);
+ kctl = snd_soc_card_get_kcontrol_locked(soc_card, name);
+ up_read(&card->controls_rwsem);
+
+ return kctl;
+}
EXPORT_SYMBOL_GPL(snd_soc_card_get_kcontrol);
static int jack_new(struct snd_soc_card *card, const char *id, int type,
--
2.30.2
On Wed, 21 Feb 2024 12:37:10 +0000, 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).
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: soc-card: Fix missing locking in snd_soc_card_get_kcontrol()
commit: eba2eb2495f47690400331c722868902784e59de
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
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 <[email protected]>
> 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.
thanks,
Takashi
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 <[email protected]>
>> 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