| Message ID | 20240221123710.690224-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eba2eb2495f47690400331c722868902784e59de |
| Headers | show |
| Series | ASoC: soc-card: Fix missing locking in snd_soc_card_get_kcontrol() | expand |
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 <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. 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 <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
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 <kuninori.morimoto.gx@renesas.com> // + +#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,
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. --- 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(-)