Re: [PATCH] ASoC: soc-card: Fix missing locking in snd_soc_card_get_kcontrol()

2024-03-06 Thread Richard Fitzgerald

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




[PATCH] ASoC: soc-card: Fix missing locking in snd_soc_card_get_kcontrol()

2024-02-21 Thread Richard Fitzgerald
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 
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);