[PATCH 00/11] ALSA: Make control API taking controls_rwsem consistently

[PATCH 00/11] ALSA: Make control API taking controls_rwsem consistently

[Takashi Iwai]

A few ALSA control API helpers like snd_ctl_rename(), snd_ctl_remove()
and snd_ctl_find_*() suppose the callers taking card->controls_rwsem.
But it's error-prone and fragile.  This patch set tries to change
those API functions to take the card->controls>rwsem internally by
themselves, so that the drivers don't need to take care of lockings.

After applying this patch set, only a couple of places still touch
card->controls_rwsem (which are OK-ish as they need for traversing the
control linked list).


Takashi

===

Takashi Iwai (11):
  ALSA: control: Take card->controls_rwsem in snd_ctl_rename()
  staging: greybus: audio_helper: Use snd_ctl_remove_id()
  ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()
  ALSA: control: Take controls_rwsem lock in snd_ctl_remove()
  ALSA: control: Add lockdep warning to internal functions
  ASoC: sigmadsp: Simplify with snd_ctl_activate_id()
  staging: greybus: Avoid abusing controls_rwsem
  ALSA: control: Make snd_ctl_find_id() argument const
  ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers
  ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()
  ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()

 drivers/staging/greybus/audio_codec.c  |  18 ++--
 drivers/staging/greybus/audio_codec.h  |   1 +
 drivers/staging/greybus/audio_helper.c |  20 +---
 include/sound/control.h                |   6 +-
 sound/core/control.c                   | 126 ++++++++++++++++++++-----
 sound/core/control_compat.c            |   2 +-
 sound/core/control_led.c               |   2 +-
 sound/core/jack.c                      |   2 -
 sound/core/oss/mixer_oss.c             |  10 +-
 sound/core/pcm.c                       |   2 -
 sound/isa/sb/emu8000.c                 |   2 -
 sound/isa/sb/sb16_csp.c                |   2 -
 sound/pci/emu10k1/emufx.c              |   5 -
 sound/pci/hda/hda_codec.c              |   2 -
 sound/soc/atmel/mchp-pdmc.c            |  12 +--
 sound/soc/codecs/sigmadsp.c            |  25 +----
 sound/soc/soc-topology.c               |   3 -
 17 files changed, 129 insertions(+), 111 deletions(-)

-- 
2.35.3

[PATCH 01/11] ALSA: control: Take card->controls_rwsem in snd_ctl_rename()

[Takashi Iwai]

snd_ctl_rename() expects that card->controls_rwsem is held in the
caller side for avoiding possible races, but actually no one really
did that.  It's likely because this operation is done usually only at
the device initialization where no race can happen.  But, it's still
safer to take a lock, so we just take the lock inside snd_ctl_rename()
like most of other API functions do.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 8386b53acdcd..a41d19c46df2 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -769,11 +769,12 @@ EXPORT_SYMBOL(snd_ctl_rename_id);
  *
  * Renames the specified control on the card to the new name.
  *
- * Make sure to take the control write lock - down_write(&card->controls_rwsem).
+ * Note that this function takes card->controls_rwsem lock internally.
  */
 void snd_ctl_rename(struct snd_card *card, struct snd_kcontrol *kctl,
 		    const char *name)
 {
+	down_write(&card->controls_rwsem);
 	remove_hash_entries(card, kctl);
 
 	if (strscpy(kctl->id.name, name, sizeof(kctl->id.name)) < 0)
@@ -781,6 +782,7 @@ void snd_ctl_rename(struct snd_card *card, struct snd_kcontrol *kctl,
 			name, kctl->id.name);
 
 	add_hash_entries(card, kctl);
+	up_write(&card->controls_rwsem);
 }
 EXPORT_SYMBOL(snd_ctl_rename);
 
-- 
2.35.3

[PATCH 02/11] staging: greybus: audio_helper: Use snd_ctl_remove_id()

[Takashi Iwai]

Use the standard snd_ctl_remove_id() helper function instead of open
code.  This allows us to reduce the manual card->rwsem lock in the
caller side.

Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: greybus-dev@lists.linaro.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/greybus/audio_helper.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 223987616e07..97ce5b9ad7fd 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -149,7 +149,6 @@ static int gbaudio_remove_controls(struct snd_card *card, struct device *dev,
 	for (i = 0; i < num_controls; i++) {
 		const struct snd_kcontrol_new *control = &controls[i];
 		struct snd_ctl_elem_id id;
-		struct snd_kcontrol *kctl;
 
 		if (prefix)
 			snprintf(id.name, sizeof(id.name), "%s %s", prefix,
@@ -161,17 +160,10 @@ static int gbaudio_remove_controls(struct snd_card *card, struct device *dev,
 		id.device = control->device;
 		id.subdevice = control->subdevice;
 		id.index = control->index;
-		kctl = snd_ctl_find_id(card, &id);
-		if (!kctl) {
-			dev_err(dev, "Failed to find %s\n", control->name);
-			continue;
-		}
-		err = snd_ctl_remove(card, kctl);
-		if (err < 0) {
+		err = snd_ctl_remove_id(card, &id);
+		if (err < 0)
 			dev_err(dev, "%d: Failed to remove %s\n", err,
 				control->name);
-			continue;
-		}
 	}
 	return 0;
 }
@@ -181,11 +173,7 @@ int gbaudio_remove_component_controls(struct snd_soc_component *component,
 				      unsigned int num_controls)
 {
 	struct snd_card *card = component->card->snd_card;
-	int err;
 
-	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;
+	return gbaudio_remove_controls(card, component->dev, controls,
+				       num_controls, component->name_prefix);
 }
-- 
2.35.3

Re: [PATCH 02/11] staging: greybus: audio_helper: Use snd_ctl_remove_id()

[Greg Kroah-Hartman]

On Tue, Jul 18, 2023 at 04:12:55PM +0200, Takashi Iwai wrote:
> Use the standard snd_ctl_remove_id() helper function instead of open
> code.  This allows us to reduce the manual card->rwsem lock in the
> caller side.
> 
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: greybus-dev@lists.linaro.org
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/staging/greybus/audio_helper.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 03/11] ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()

[Takashi Iwai]

Use the standard snd_ctl_remove_id() helper instead of open code for
removing a kctl.  This helps for avoiding possible races.

Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/atmel/mchp-pdmc.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c
index c79c73e6791e..1a069f4cdcda 100644
--- a/sound/soc/atmel/mchp-pdmc.c
+++ b/sound/soc/atmel/mchp-pdmc.c
@@ -386,7 +386,6 @@ static int mchp_pdmc_open(struct snd_soc_component *component,
 	for (i = 0; i < ARRAY_SIZE(mchp_pdmc_snd_controls); i++) {
 		const struct snd_kcontrol_new *control = &mchp_pdmc_snd_controls[i];
 		struct snd_ctl_elem_id id;
-		struct snd_kcontrol *kctl;
 		int err;
 
 		if (component->name_prefix)
@@ -400,17 +399,10 @@ static int mchp_pdmc_open(struct snd_soc_component *component,
 		id.device = control->device;
 		id.subdevice = control->subdevice;
 		id.index = control->index;
-		kctl = snd_ctl_find_id(component->card->snd_card, &id);
-		if (!kctl) {
-			dev_err(component->dev, "Failed to find %s\n", control->name);
-			continue;
-		}
-		err = snd_ctl_remove(component->card->snd_card, kctl);
-		if (err < 0) {
+		err = snd_ctl_remove_id(component->card->snd_card, &id);
+		if (err < 0)
 			dev_err(component->dev, "%d: Failed to remove %s\n", err,
 				control->name);
-			continue;
-		}
 	}
 
 	return 0;
-- 
2.35.3

Re: [PATCH 03/11] ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()

[claudiu beznea]

On 18.07.2023 17:12, Takashi Iwai wrote:
> Use the standard snd_ctl_remove_id() helper instead of open code for
> removing a kctl.  This helps for avoiding possible races.
> 
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   sound/soc/atmel/mchp-pdmc.c | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c
> index c79c73e6791e..1a069f4cdcda 100644
> --- a/sound/soc/atmel/mchp-pdmc.c
> +++ b/sound/soc/atmel/mchp-pdmc.c
> @@ -386,7 +386,6 @@ static int mchp_pdmc_open(struct snd_soc_component *component,
>   	for (i = 0; i < ARRAY_SIZE(mchp_pdmc_snd_controls); i++) {
>   		const struct snd_kcontrol_new *control = &mchp_pdmc_snd_controls[i];
>   		struct snd_ctl_elem_id id;
> -		struct snd_kcontrol *kctl;
>   		int err;
>   
>   		if (component->name_prefix)
> @@ -400,17 +399,10 @@ static int mchp_pdmc_open(struct snd_soc_component *component,
>   		id.device = control->device;
>   		id.subdevice = control->subdevice;
>   		id.index = control->index;
> -		kctl = snd_ctl_find_id(component->card->snd_card, &id);
> -		if (!kctl) {
> -			dev_err(component->dev, "Failed to find %s\n", control->name);
> -			continue;
> -		}
> -		err = snd_ctl_remove(component->card->snd_card, kctl);
> -		if (err < 0) {
> +		err = snd_ctl_remove_id(component->card->snd_card, &id);
> +		if (err < 0)
>   			dev_err(component->dev, "%d: Failed to remove %s\n", err,
>   				control->name);
> -			continue;
> -		}
>   	}
>   
>   	return 0;

Re: [PATCH 03/11] ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

On Tue, Jul 18, 2023 at 04:12:56PM +0200, Takashi Iwai wrote:
> Use the standard snd_ctl_remove_id() helper instead of open code for
> removing a kctl.  This helps for avoiding possible races.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/11] ALSA: control: Take controls_rwsem lock in snd_ctl_remove()

[Takashi Iwai]

So far, snd_ctl_remove() requires its caller to take
card->controls_rwsem manually before the call for avoiding possible
races.  However, many callers don't care and miss the locking.

Basically it's cumbersome and error-prone to enforce it to each
caller.  Moreover, card->controls_rwsem is a field that should be used
only by internal or proper helpers, and it's not to be touched at
random external places.

This patch is an attempt to make those calls more consistent: now
snd_ctl_remove() takes the card->controls_rwsem internally, just like
other API functions for kctls.  Since a few callers already take the
controls_rwsem locks, the patch removes those locks at the same time,
too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c      | 27 +++++++++++++++++++++------
 sound/core/jack.c         |  2 --
 sound/core/pcm.c          |  2 --
 sound/isa/sb/emu8000.c    |  2 --
 sound/isa/sb/sb16_csp.c   |  2 --
 sound/pci/emu10k1/emufx.c |  2 --
 sound/pci/hda/hda_codec.c |  2 --
 sound/soc/soc-topology.c  |  3 ---
 8 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index a41d19c46df2..9c933350ec6b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -39,6 +39,9 @@ static LIST_HEAD(snd_control_compat_ioctls);
 #endif
 static struct snd_ctl_layer_ops *snd_ctl_layer;
 
+static int snd_ctl_remove_locked(struct snd_card *card,
+				 struct snd_kcontrol *kcontrol);
+
 static int snd_ctl_open(struct inode *inode, struct file *file)
 {
 	unsigned long flags;
@@ -483,7 +486,7 @@ static int __snd_ctl_add_replace(struct snd_card *card,
 			return -EBUSY;
 		}
 
-		err = snd_ctl_remove(card, old);
+		err = snd_ctl_remove_locked(card, old);
 		if (err < 0)
 			return err;
 	}
@@ -589,20 +592,32 @@ static int __snd_ctl_remove(struct snd_card *card,
 	return 0;
 }
 
+static inline int snd_ctl_remove_locked(struct snd_card *card,
+					struct snd_kcontrol *kcontrol)
+{
+	return __snd_ctl_remove(card, kcontrol, true);
+}
+
 /**
  * snd_ctl_remove - remove the control from the card and release it
  * @card: the card instance
  * @kcontrol: the control instance to remove
  *
  * Removes the control from the card and then releases the instance.
- * You don't need to call snd_ctl_free_one(). You must be in
- * the write lock - down_write(&card->controls_rwsem).
+ * You don't need to call snd_ctl_free_one().
  *
  * Return: 0 if successful, or a negative error code on failure.
+ *
+ * Note that this function takes card->controls_rwsem lock internally.
  */
 int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol)
 {
-	return __snd_ctl_remove(card, kcontrol, true);
+	int ret;
+
+	down_write(&card->controls_rwsem);
+	ret = snd_ctl_remove_locked(card, kcontrol);
+	up_write(&card->controls_rwsem);
+	return ret;
 }
 EXPORT_SYMBOL(snd_ctl_remove);
 
@@ -627,7 +642,7 @@ int snd_ctl_remove_id(struct snd_card *card, struct snd_ctl_elem_id *id)
 		up_write(&card->controls_rwsem);
 		return -ENOENT;
 	}
-	ret = snd_ctl_remove(card, kctl);
+	ret = snd_ctl_remove_locked(card, kctl);
 	up_write(&card->controls_rwsem);
 	return ret;
 }
@@ -665,7 +680,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 			ret = -EBUSY;
 			goto error;
 		}
-	ret = snd_ctl_remove(card, kctl);
+	ret = snd_ctl_remove_locked(card, kctl);
 error:
 	up_write(&card->controls_rwsem);
 	return ret;
diff --git a/sound/core/jack.c b/sound/core/jack.c
index 03d155ed362b..e0f034e7275c 100644
--- a/sound/core/jack.c
+++ b/sound/core/jack.c
@@ -66,12 +66,10 @@ static int snd_jack_dev_free(struct snd_device *device)
 	struct snd_card *card = device->card;
 	struct snd_jack_kctl *jack_kctl, *tmp_jack_kctl;
 
-	down_write(&card->controls_rwsem);
 	list_for_each_entry_safe(jack_kctl, tmp_jack_kctl, &jack->kctl_list, list) {
 		list_del_init(&jack_kctl->list);
 		snd_ctl_remove(card, jack_kctl->kctl);
 	}
-	up_write(&card->controls_rwsem);
 
 	if (jack->private_free)
 		jack->private_free(jack);
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 9d95e3731123..1c0bb3a07bff 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -814,9 +814,7 @@ static void free_chmap(struct snd_pcm_str *pstr)
 	if (pstr->chmap_kctl) {
 		struct snd_card *card = pstr->pcm->card;
 
-		down_write(&card->controls_rwsem);
 		snd_ctl_remove(card, pstr->chmap_kctl);
-		up_write(&card->controls_rwsem);
 		pstr->chmap_kctl = NULL;
 	}
 }
diff --git a/sound/isa/sb/emu8000.c b/sound/isa/sb/emu8000.c
index e02029677743..a6405772d537 100644
--- a/sound/isa/sb/emu8000.c
+++ b/sound/isa/sb/emu8000.c
@@ -1040,10 +1040,8 @@ snd_emu8000_create_mixer(struct snd_card *card, struct snd_emu8000 *emu)
 
 __error:
 	for (i = 0; i < EMU8000_NUM_CONTROLS; i++) {
-		down_write(&card->controls_rwsem);
 		if (emu->controls[i])
 			snd_ctl_remove(card, emu->controls[i]);
-		up_write(&card->controls_rwsem);
 	}
 	return err;
 }
diff --git a/sound/isa/sb/sb16_csp.c b/sound/isa/sb/sb16_csp.c
index 7ad8c5f7b664..8d8357019719 100644
--- a/sound/isa/sb/sb16_csp.c
+++ b/sound/isa/sb/sb16_csp.c
@@ -1080,7 +1080,6 @@ static void snd_sb_qsound_destroy(struct snd_sb_csp * p)
 
 	card = p->chip->card;	
 	
-	down_write(&card->controls_rwsem);
 	if (p->qsound_switch) {
 		snd_ctl_remove(card, p->qsound_switch);
 		p->qsound_switch = NULL;
@@ -1089,7 +1088,6 @@ static void snd_sb_qsound_destroy(struct snd_sb_csp * p)
 		snd_ctl_remove(card, p->qsound_space);
 		p->qsound_space = NULL;
 	}
-	up_write(&card->controls_rwsem);
 
 	/* cancel pending transfer of QSound parameters */
 	spin_lock_irqsave (&p->q_lock, flags);
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 9904bcfee106..70c8252a92d9 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -977,11 +977,9 @@ static int snd_emu10k1_del_controls(struct snd_emu10k1 *emu,
 				       in_kernel);
 		if (err < 0)
 			return err;
-		down_write(&card->controls_rwsem);
 		ctl = snd_emu10k1_look_for_ctl(emu, &id);
 		if (ctl)
 			snd_ctl_remove(card, ctl->kcontrol);
-		up_write(&card->controls_rwsem);
 	}
 	return 0;
 }
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index bd19f92aeeec..33af707a65ab 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1769,10 +1769,8 @@ void snd_hda_ctls_clear(struct hda_codec *codec)
 	int i;
 	struct hda_nid_item *items = codec->mixers.list;
 
-	down_write(&codec->card->controls_rwsem);
 	for (i = 0; i < codec->mixers.used; i++)
 		snd_ctl_remove(codec->card, items[i].kctl);
-	up_write(&codec->card->controls_rwsem);
 	snd_array_free(&codec->mixers);
 	snd_array_free(&codec->nids);
 }
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 8add361e87c6..03ec3c5adc3a 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -2564,7 +2564,6 @@ EXPORT_SYMBOL_GPL(snd_soc_tplg_component_load);
 /* remove dynamic controls from the component driver */
 int snd_soc_tplg_component_remove(struct snd_soc_component *comp)
 {
-	struct snd_card *card = comp->card->snd_card;
 	struct snd_soc_dobj *dobj, *next_dobj;
 	int pass;
 
@@ -2572,7 +2571,6 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp)
 	for (pass = SOC_TPLG_PASS_END; pass >= SOC_TPLG_PASS_START; pass--) {
 
 		/* remove mixer controls */
-		down_write(&card->controls_rwsem);
 		list_for_each_entry_safe(dobj, next_dobj, &comp->dobj_list,
 			list) {
 
@@ -2607,7 +2605,6 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp)
 				break;
 			}
 		}
-		up_write(&card->controls_rwsem);
 	}
 
 	/* let caller know if FW can be freed when no objects are left */
-- 
2.35.3

[PATCH 05/11] ALSA: control: Add lockdep warning to internal functions

[Takashi Iwai]

To assure the proper locking, add the lockdep check to
__snd_ctl_remove(), __snd_ctl_add_replace() and other internal
functions to handle user controls.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 9c933350ec6b..8aaa2a84a670 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -469,6 +469,8 @@ static int __snd_ctl_add_replace(struct snd_card *card,
 	struct snd_kcontrol *old;
 	int err;
 
+	lockdep_assert_held_write(&card->controls_rwsem);
+
 	id = kcontrol->id;
 	if (id.index > UINT_MAX - kcontrol->count)
 		return -EINVAL;
@@ -578,6 +580,8 @@ static int __snd_ctl_remove(struct snd_card *card,
 {
 	unsigned int idx;
 
+	lockdep_assert_held_write(&card->controls_rwsem);
+
 	if (snd_BUG_ON(!card || !kcontrol))
 		return -EINVAL;
 	list_del(&kcontrol->list);
@@ -1524,6 +1528,8 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf,
 	int i;
 	int change;
 
+	lockdep_assert_held_write(&ue->card->controls_rwsem);
+
 	if (size > 1024 * 128)	/* sane value */
 		return -EINVAL;
 
@@ -1600,6 +1606,8 @@ static int snd_ctl_elem_init_enum_names(struct user_element *ue)
 	unsigned int i;
 	const uintptr_t user_ptrval = ue->info.value.enumerated.names_ptr;
 
+	lockdep_assert_held_write(&ue->card->controls_rwsem);
+
 	buf_len = ue->info.value.enumerated.names_length;
 	if (buf_len > 64 * 1024)
 		return -EINVAL;
@@ -1904,6 +1912,8 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 	struct snd_ctl_elem_id id;
 	struct snd_kcontrol_volatile *vd;
 
+	lockdep_assert_held(&file->card->controls_rwsem);
+
 	if (copy_from_user(&header, buf, sizeof(header)))
 		return -EFAULT;
 
-- 
2.35.3

[PATCH 06/11] ASoC: sigmadsp: Simplify with snd_ctl_activate_id()

[Takashi Iwai]

Use the standard snd_ctl_activate_id() helper instead of an open code
for code simplification.

Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: "Nuno Sá" <nuno.sa@analog.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/sigmadsp.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c
index 3047a6fbb380..b93c078a8040 100644
--- a/sound/soc/codecs/sigmadsp.c
+++ b/sound/soc/codecs/sigmadsp.c
@@ -669,36 +669,19 @@ static void sigmadsp_activate_ctrl(struct sigmadsp *sigmadsp,
 	struct sigmadsp_control *ctrl, unsigned int samplerate_mask)
 {
 	struct snd_card *card = sigmadsp->component->card->snd_card;
-	struct snd_kcontrol_volatile *vd;
-	struct snd_ctl_elem_id id;
 	bool active;
-	bool changed = false;
+	int changed;
 
 	active = sigmadsp_samplerate_valid(ctrl->samplerates, samplerate_mask);
-
-	down_write(&card->controls_rwsem);
-	if (!ctrl->kcontrol) {
-		up_write(&card->controls_rwsem);
+	if (!ctrl->kcontrol)
 		return;
-	}
-
-	id = ctrl->kcontrol->id;
-	vd = &ctrl->kcontrol->vd[0];
-	if (active == (bool)(vd->access & SNDRV_CTL_ELEM_ACCESS_INACTIVE)) {
-		vd->access ^= SNDRV_CTL_ELEM_ACCESS_INACTIVE;
-		changed = true;
-	}
-	up_write(&card->controls_rwsem);
-
-	if (active && changed) {
+	changed = snd_ctl_activate_id(card, &ctrl->kcontrol->id, active);
+	if (active && changed > 0) {
 		mutex_lock(&sigmadsp->lock);
 		if (ctrl->cached)
 			sigmadsp_ctrl_write(sigmadsp, ctrl, ctrl->cache);
 		mutex_unlock(&sigmadsp->lock);
 	}
-
-	if (changed)
-		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, &id);
 }
 
 /**
-- 
2.35.3

Re: [PATCH 06/11] ASoC: sigmadsp: Simplify with snd_ctl_activate_id()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 204 bytes --]

On Tue, Jul 18, 2023 at 04:12:59PM +0200, Takashi Iwai wrote:
> Use the standard snd_ctl_activate_id() helper instead of an open code
> for code simplification.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/11] staging: greybus: Avoid abusing controls_rwsem

[Takashi Iwai]

The controls_rwsem of snd_card object is rather an internal lock, and
not really meant to be used by others for its data protection.

This patch addresses it by replacing the controls_rwsem usages with
the own (new) mutex.

Note that the up_write() and down_write() calls around
gbaudio_remove_component_controls() are simply dropped without
replacement.  These temporary up/down were a workaround since
gbaudio_remove_component_controls() itself took the rwsem.  Now it was
also gone, we can clean up the workaround, too.

Cc: Vaibhav Agarwal <vaibhav.sr@gmail.com>
Cc: Mark Greer <mgreer@animalcreek.com>
Cc: Johan Hovold <johan@kernel.org>
Cc: Alex Elder <elder@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: greybus-dev@lists.linaro.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/staging/greybus/audio_codec.c | 18 +++++++-----------
 drivers/staging/greybus/audio_codec.h |  1 +
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index 72ace74ea605..2f05e761fb9a 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -807,7 +807,6 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
 {
 	int ret;
 	struct snd_soc_component *comp;
-	struct snd_card *card;
 	struct gbaudio_jack *jack = NULL;
 
 	if (!gbcodec) {
@@ -816,21 +815,20 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
 	}
 
 	comp = gbcodec->component;
-	card = comp->card->snd_card;
 
-	down_write(&card->controls_rwsem);
+	mutex_lock(&gbcodec->register_mutex);
 
 	if (module->num_dais) {
 		dev_err(gbcodec->dev,
 			"%d:DAIs not supported via gbcodec driver\n",
 			module->num_dais);
-		up_write(&card->controls_rwsem);
+		mutex_unlock(&gbcodec->register_mutex);
 		return -EINVAL;
 	}
 
 	ret = gbaudio_init_jack(module, comp->card);
 	if (ret) {
-		up_write(&card->controls_rwsem);
+		mutex_unlock(&gbcodec->register_mutex);
 		return ret;
 	}
 
@@ -867,7 +865,7 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
 		ret = snd_soc_dapm_new_widgets(comp->card);
 	dev_dbg(comp->dev, "Registered %s module\n", module->name);
 
-	up_write(&card->controls_rwsem);
+	mutex_unlock(&gbcodec->register_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(gbaudio_register_module);
@@ -935,13 +933,12 @@ static void gbaudio_codec_cleanup(struct gbaudio_module_info *module)
 void gbaudio_unregister_module(struct gbaudio_module_info *module)
 {
 	struct snd_soc_component *comp = gbcodec->component;
-	struct snd_card *card = comp->card->snd_card;
 	struct gbaudio_jack *jack, *n;
 	int mask;
 
 	dev_dbg(comp->dev, "Unregister %s module\n", module->name);
 
-	down_write(&card->controls_rwsem);
+	mutex_lock(&gbcodec->register_mutex);
 	mutex_lock(&gbcodec->lock);
 	gbaudio_codec_cleanup(module);
 	list_del(&module->list);
@@ -978,10 +975,8 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module)
 		dev_dbg(comp->dev, "Removing %d controls\n",
 			module->num_controls);
 		/* release control semaphore */
-		up_write(&card->controls_rwsem);
 		gbaudio_remove_component_controls(comp, module->controls,
 						  module->num_controls);
-		down_write(&card->controls_rwsem);
 	}
 	if (module->dapm_widgets) {
 		dev_dbg(comp->dev, "Removing %d widgets\n",
@@ -992,7 +987,7 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module)
 
 	dev_dbg(comp->dev, "Unregistered %s module\n", module->name);
 
-	up_write(&card->controls_rwsem);
+	mutex_unlock(&gbcodec->register_mutex);
 }
 EXPORT_SYMBOL(gbaudio_unregister_module);
 
@@ -1012,6 +1007,7 @@ static int gbcodec_probe(struct snd_soc_component *comp)
 	info->dev = comp->dev;
 	INIT_LIST_HEAD(&info->module_list);
 	mutex_init(&info->lock);
+	mutex_init(&info->register_mutex);
 	INIT_LIST_HEAD(&info->dai_list);
 
 	/* init dai_list used to maintain runtime stream info */
diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
index ce15e800e607..f3f7a7ec6be4 100644
--- a/drivers/staging/greybus/audio_codec.h
+++ b/drivers/staging/greybus/audio_codec.h
@@ -71,6 +71,7 @@ struct gbaudio_codec_info {
 	/* to maintain runtime stream params for each DAI */
 	struct list_head dai_list;
 	struct mutex lock;
+	struct mutex register_mutex;
 };
 
 struct gbaudio_widget {
-- 
2.35.3

Re: [PATCH 07/11] staging: greybus: Avoid abusing controls_rwsem

[Greg Kroah-Hartman]

On Tue, Jul 18, 2023 at 04:13:00PM +0200, Takashi Iwai wrote:
> The controls_rwsem of snd_card object is rather an internal lock, and
> not really meant to be used by others for its data protection.
> 
> This patch addresses it by replacing the controls_rwsem usages with
> the own (new) mutex.
> 
> Note that the up_write() and down_write() calls around
> gbaudio_remove_component_controls() are simply dropped without
> replacement.  These temporary up/down were a workaround since
> gbaudio_remove_component_controls() itself took the rwsem.  Now it was
> also gone, we can clean up the workaround, too.
> 
> Cc: Vaibhav Agarwal <vaibhav.sr@gmail.com>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: greybus-dev@lists.linaro.org
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/staging/greybus/audio_codec.c | 18 +++++++-----------
>  drivers/staging/greybus/audio_codec.h |  1 +
>  2 files changed, 8 insertions(+), 11 deletions(-)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 08/11] ALSA: control: Make snd_ctl_find_id() argument const

[Takashi Iwai]

The id object passed to snd_ctl_find_id() is only read, and we can
mark it with const gracefully.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/control.h | 2 +-
 sound/core/control.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index cc3dcc6cfb0f..e61b749bf204 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -141,7 +141,7 @@ int snd_ctl_rename_id(struct snd_card * card, struct snd_ctl_elem_id *src_id, st
 void snd_ctl_rename(struct snd_card *card, struct snd_kcontrol *kctl, const char *name);
 int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, int active);
 struct snd_kcontrol *snd_ctl_find_numid(struct snd_card * card, unsigned int numid);
-struct snd_kcontrol *snd_ctl_find_id(struct snd_card * card, struct snd_ctl_elem_id *id);
+struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card, const struct snd_ctl_elem_id *id);
 
 int snd_ctl_create(struct snd_card *card);
 
diff --git a/sound/core/control.c b/sound/core/control.c
index 8aaa2a84a670..180e5768a10f 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -858,7 +858,7 @@ EXPORT_SYMBOL(snd_ctl_find_numid);
  *
  */
 struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
-				     struct snd_ctl_elem_id *id)
+				     const struct snd_ctl_elem_id *id)
 {
 	struct snd_kcontrol *kctl;
 
-- 
2.35.3

[PATCH 09/11] ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers

[Takashi Iwai]

For reducing the unnecessary use of controls_rwsem in the drivers,
this patch adds a new variant for snd_ctl_find_*() helpers:
snd_ctl_find_id_locked() and snd_ctl_find_numid_locked() look for a
kctl element inside the card->controls_rwsem -- that is, doing the
very same as what snd_ctl_find_id() and snd_ctl_find_numid() did until
now.  snd_ctl_find_id() and snd_ctl_find_numid() remain same,
i.e. still unlocked version, but they will be switched to locked
version once after all callers are replaced.

The patch also replaces the calls of snd_ctl_find_id() and
snd_ctl_find_numid() in a few places; all of those are places where we
know that the functions are called properly with controls_rwsem held.
All others are without rwsem (although they should have been).

After this patch, we'll turn on the locking in snd_ctl_find_id() and
snd_ctl_find_numid() to be more race-free.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/control.h     |  4 ++-
 sound/core/control.c        | 69 +++++++++++++++++++++++++++----------
 sound/core/control_compat.c |  2 +-
 sound/core/control_led.c    |  2 +-
 sound/core/oss/mixer_oss.c  | 10 +++---
 sound/pci/emu10k1/emufx.c   |  2 +-
 6 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index e61b749bf204..42e8dbb22d8e 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -140,7 +140,9 @@ int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id);
 int snd_ctl_rename_id(struct snd_card * card, struct snd_ctl_elem_id *src_id, struct snd_ctl_elem_id *dst_id);
 void snd_ctl_rename(struct snd_card *card, struct snd_kcontrol *kctl, const char *name);
 int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, int active);
-struct snd_kcontrol *snd_ctl_find_numid(struct snd_card * card, unsigned int numid);
+struct snd_kcontrol *snd_ctl_find_numid_locked(struct snd_card *card, unsigned int numid);
+struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numid);
+struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card, const struct snd_ctl_elem_id *id);
 struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card, const struct snd_ctl_elem_id *id);
 
 int snd_ctl_create(struct snd_card *card);
diff --git a/sound/core/control.c b/sound/core/control.c
index 180e5768a10f..30741293708d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -475,7 +475,7 @@ static int __snd_ctl_add_replace(struct snd_card *card,
 	if (id.index > UINT_MAX - kcontrol->count)
 		return -EINVAL;
 
-	old = snd_ctl_find_id(card, &id);
+	old = snd_ctl_find_id_locked(card, &id);
 	if (!old) {
 		if (mode == CTL_REPLACE)
 			return -EINVAL;
@@ -641,7 +641,7 @@ int snd_ctl_remove_id(struct snd_card *card, struct snd_ctl_elem_id *id)
 	int ret;
 
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, id);
+	kctl = snd_ctl_find_id_locked(card, id);
 	if (kctl == NULL) {
 		up_write(&card->controls_rwsem);
 		return -ENOENT;
@@ -670,7 +670,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file,
 	int idx, ret;
 
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, id);
+	kctl = snd_ctl_find_id_locked(card, id);
 	if (kctl == NULL) {
 		ret = -ENOENT;
 		goto error;
@@ -711,7 +711,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
 	int ret;
 
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, id);
+	kctl = snd_ctl_find_id_locked(card, id);
 	if (kctl == NULL) {
 		ret = -ENOENT;
 		goto unlock;
@@ -765,7 +765,7 @@ int snd_ctl_rename_id(struct snd_card *card, struct snd_ctl_elem_id *src_id,
 	int saved_numid;
 
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, src_id);
+	kctl = snd_ctl_find_id_locked(card, src_id);
 	if (kctl == NULL) {
 		up_write(&card->controls_rwsem);
 		return -ENOENT;
@@ -820,7 +820,7 @@ snd_ctl_find_numid_slow(struct snd_card *card, unsigned int numid)
 #endif /* !CONFIG_SND_CTL_FAST_LOOKUP */
 
 /**
- * snd_ctl_find_numid - find the control instance with the given number-id
+ * snd_ctl_find_numid_locked - find the control instance with the given number-id
  * @card: the card instance
  * @numid: the number-id to search
  *
@@ -830,9 +830,9 @@ snd_ctl_find_numid_slow(struct snd_card *card, unsigned int numid)
  * (if the race condition can happen).
  *
  * Return: The pointer of the instance if found, or %NULL if not.
- *
  */
-struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numid)
+struct snd_kcontrol *
+snd_ctl_find_numid_locked(struct snd_card *card, unsigned int numid)
 {
 	if (snd_BUG_ON(!card || !numid))
 		return NULL;
@@ -842,10 +842,26 @@ struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numi
 	return snd_ctl_find_numid_slow(card, numid);
 #endif
 }
+EXPORT_SYMBOL(snd_ctl_find_numid_locked);
+
+/**
+ * snd_ctl_find_numid - find the control instance with the given number-id
+ * @card: the card instance
+ * @numid: the number-id to search
+ *
+ * Finds the control instance with the given number-id from the card.
+ *
+ * Return: The pointer of the instance if found, or %NULL if not.
+ */
+struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card,
+					unsigned int numid)
+{
+	return snd_ctl_find_numid_locked(card, numid);
+}
 EXPORT_SYMBOL(snd_ctl_find_numid);
 
 /**
- * snd_ctl_find_id - find the control instance with the given id
+ * snd_ctl_find_id_locked - find the control instance with the given id
  * @card: the card instance
  * @id: the id to search
  *
@@ -855,17 +871,16 @@ EXPORT_SYMBOL(snd_ctl_find_numid);
  * (if the race condition can happen).
  *
  * Return: The pointer of the instance if found, or %NULL if not.
- *
  */
-struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
-				     const struct snd_ctl_elem_id *id)
+struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card,
+					    const struct snd_ctl_elem_id *id)
 {
 	struct snd_kcontrol *kctl;
 
 	if (snd_BUG_ON(!card || !id))
 		return NULL;
 	if (id->numid != 0)
-		return snd_ctl_find_numid(card, id->numid);
+		return snd_ctl_find_numid_locked(card, id->numid);
 #ifdef CONFIG_SND_CTL_FAST_LOOKUP
 	kctl = xa_load(&card->ctl_hash, get_ctl_id_hash(id));
 	if (kctl && elem_id_matches(kctl, id))
@@ -880,6 +895,22 @@ struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
 
 	return NULL;
 }
+EXPORT_SYMBOL(snd_ctl_find_id_locked);
+
+/**
+ * snd_ctl_find_id - find the control instance with the given id
+ * @card: the card instance
+ * @id: the id to search
+ *
+ * Finds the control instance with the given id from the card.
+ *
+ * Return: The pointer of the instance if found, or %NULL if not.
+ */
+struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
+				     const struct snd_ctl_elem_id *id)
+{
+	return snd_ctl_find_id_locked(card, id);
+}
 EXPORT_SYMBOL(snd_ctl_find_id);
 
 static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl,
@@ -1194,7 +1225,7 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl,
 	int result;
 
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, &info->id);
+	kctl = snd_ctl_find_id_locked(card, &info->id);
 	if (kctl == NULL)
 		result = -ENOENT;
 	else
@@ -1233,7 +1264,7 @@ static int snd_ctl_elem_read(struct snd_card *card,
 	int ret;
 
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, &control->id);
+	kctl = snd_ctl_find_id_locked(card, &control->id);
 	if (kctl == NULL) {
 		ret = -ENOENT;
 		goto unlock;
@@ -1310,7 +1341,7 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	int result;
 
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, &control->id);
+	kctl = snd_ctl_find_id_locked(card, &control->id);
 	if (kctl == NULL) {
 		up_write(&card->controls_rwsem);
 		return -ENOENT;
@@ -1391,7 +1422,7 @@ static int snd_ctl_elem_lock(struct snd_ctl_file *file,
 	if (copy_from_user(&id, _id, sizeof(id)))
 		return -EFAULT;
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, &id);
+	kctl = snd_ctl_find_id_locked(card, &id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
@@ -1419,7 +1450,7 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file,
 	if (copy_from_user(&id, _id, sizeof(id)))
 		return -EFAULT;
 	down_write(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, &id);
+	kctl = snd_ctl_find_id_locked(card, &id);
 	if (kctl == NULL) {
 		result = -ENOENT;
 	} else {
@@ -1927,7 +1958,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 	container_size = header.length;
 	container = buf->tlv;
 
-	kctl = snd_ctl_find_numid(file->card, header.numid);
+	kctl = snd_ctl_find_numid_locked(file->card, header.numid);
 	if (kctl == NULL)
 		return -ENOENT;
 
diff --git a/sound/core/control_compat.c b/sound/core/control_compat.c
index 9cae5d74335c..0e8b1bfb040e 100644
--- a/sound/core/control_compat.c
+++ b/sound/core/control_compat.c
@@ -173,7 +173,7 @@ static int get_ctl_type(struct snd_card *card, struct snd_ctl_elem_id *id,
 	int err;
 
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_id(card, id);
+	kctl = snd_ctl_find_id_locked(card, id);
 	if (! kctl) {
 		up_read(&card->controls_rwsem);
 		return -ENOENT;
diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index ee77547bf8dc..67fc2a1dcf7a 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -251,7 +251,7 @@ static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
 	card = snd_card_ref(card_number);
 	if (card) {
 		down_write(&card->controls_rwsem);
-		kctl = snd_ctl_find_id(card, id);
+		kctl = snd_ctl_find_id_locked(card, id);
 		if (kctl) {
 			ioff = snd_ctl_get_ioff(kctl, id);
 			vd = &kctl->vd[ioff];
diff --git a/sound/core/oss/mixer_oss.c b/sound/core/oss/mixer_oss.c
index 9620115cfdc0..dae2da380835 100644
--- a/sound/core/oss/mixer_oss.c
+++ b/sound/core/oss/mixer_oss.c
@@ -524,7 +524,7 @@ static struct snd_kcontrol *snd_mixer_oss_test_id(struct snd_mixer_oss *mixer, c
 	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	strscpy(id.name, name, sizeof(id.name));
 	id.index = index;
-	return snd_ctl_find_id(card, &id);
+	return snd_ctl_find_id_locked(card, &id);
 }
 
 static void snd_mixer_oss_get_volume1_vol(struct snd_mixer_oss_file *fmixer,
@@ -540,7 +540,7 @@ static void snd_mixer_oss_get_volume1_vol(struct snd_mixer_oss_file *fmixer,
 	if (numid == ID_UNKNOWN)
 		return;
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_numid(card, numid);
+	kctl = snd_ctl_find_numid_locked(card, numid);
 	if (!kctl) {
 		up_read(&card->controls_rwsem);
 		return;
@@ -579,7 +579,7 @@ static void snd_mixer_oss_get_volume1_sw(struct snd_mixer_oss_file *fmixer,
 	if (numid == ID_UNKNOWN)
 		return;
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_numid(card, numid);
+	kctl = snd_ctl_find_numid_locked(card, numid);
 	if (!kctl) {
 		up_read(&card->controls_rwsem);
 		return;
@@ -645,7 +645,7 @@ static void snd_mixer_oss_put_volume1_vol(struct snd_mixer_oss_file *fmixer,
 	if (numid == ID_UNKNOWN)
 		return;
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_numid(card, numid);
+	kctl = snd_ctl_find_numid_locked(card, numid);
 	if (!kctl) {
 		up_read(&card->controls_rwsem);
 		return;
@@ -688,7 +688,7 @@ static void snd_mixer_oss_put_volume1_sw(struct snd_mixer_oss_file *fmixer,
 	if (numid == ID_UNKNOWN)
 		return;
 	down_read(&card->controls_rwsem);
-	kctl = snd_ctl_find_numid(card, numid);
+	kctl = snd_ctl_find_numid_locked(card, numid);
 	if (!kctl) {
 		up_read(&card->controls_rwsem);
 		return;
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 70c8252a92d9..318a064f2cec 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -800,7 +800,7 @@ static int snd_emu10k1_verify_controls(struct snd_emu10k1 *emu,
 			continue;
 		gctl_id = (struct snd_ctl_elem_id *)&gctl->id;
 		down_read(&emu->card->controls_rwsem);
-		if (snd_ctl_find_id(emu->card, gctl_id)) {
+		if (snd_ctl_find_id_locked(emu->card, gctl_id)) {
 			up_read(&emu->card->controls_rwsem);
 			err = -EEXIST;
 			goto __error;
-- 
2.35.3

[PATCH 10/11] ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()

[Takashi Iwai]

Now all needed callers have been replaced with *_locked() versions,
let's turn on the locking in snd_ctl_find_id() and
snd_ctl_find_numid().

This patch also adds the lockdep assertions for debugging, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 30741293708d..e13e9d6b3b89 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -836,6 +836,7 @@ snd_ctl_find_numid_locked(struct snd_card *card, unsigned int numid)
 {
 	if (snd_BUG_ON(!card || !numid))
 		return NULL;
+	lockdep_assert_held(&card->controls_rwsem);
 #ifdef CONFIG_SND_CTL_FAST_LOOKUP
 	return xa_load(&card->ctl_numids, numid);
 #else
@@ -852,11 +853,18 @@ EXPORT_SYMBOL(snd_ctl_find_numid_locked);
  * Finds the control instance with the given number-id from the card.
  *
  * Return: The pointer of the instance if found, or %NULL if not.
+ *
+ * Note that this function takes card->controls_rwsem lock internally.
  */
 struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card,
 					unsigned int numid)
 {
-	return snd_ctl_find_numid_locked(card, numid);
+	struct snd_kcontrol *kctl;
+
+	down_read(&card->controls_rwsem);
+	kctl = snd_ctl_find_numid_locked(card, numid);
+	up_read(&card->controls_rwsem);
+	return kctl;
 }
 EXPORT_SYMBOL(snd_ctl_find_numid);
 
@@ -879,6 +887,7 @@ struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card,
 
 	if (snd_BUG_ON(!card || !id))
 		return NULL;
+	lockdep_assert_held(&card->controls_rwsem);
 	if (id->numid != 0)
 		return snd_ctl_find_numid_locked(card, id->numid);
 #ifdef CONFIG_SND_CTL_FAST_LOOKUP
@@ -905,11 +914,18 @@ EXPORT_SYMBOL(snd_ctl_find_id_locked);
  * Finds the control instance with the given id from the card.
  *
  * Return: The pointer of the instance if found, or %NULL if not.
+ *
+ * Note that this function takes card->controls_rwsem lock internally.
  */
 struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
 				     const struct snd_ctl_elem_id *id)
 {
-	return snd_ctl_find_id_locked(card, id);
+	struct snd_kcontrol *kctl;
+
+	down_read(&card->controls_rwsem);
+	kctl = snd_ctl_find_id_locked(card, id);
+	up_read(&card->controls_rwsem);
+	return kctl;
 }
 EXPORT_SYMBOL(snd_ctl_find_id);
 
-- 
2.35.3

Re: [PATCH 10/11] ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()

[Oswald Buddenhagen]

On Tue, Jul 18, 2023 at 04:13:03PM +0200, Takashi Iwai wrote:
>Now all needed callers have been replaced with *_locked() versions,
>
i don't understand why you made that a separate patch, given that there 
is no separate conversion step in between. i'd just squash it into the 
parent, and also absorb the followup commit.

regards

Re: [PATCH 10/11] ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()

[Takashi Iwai]

On Wed, 19 Jul 2023 12:20:18 +0200,
Oswald Buddenhagen wrote:
> 
> On Tue, Jul 18, 2023 at 04:13:03PM +0200, Takashi Iwai wrote:
> > Now all needed callers have been replaced with *_locked() versions,
> > 
> i don't understand why you made that a separate patch, given that
> there is no separate conversion step in between. i'd just squash it
> into the parent, and also absorb the followup commit.

The logic is that the first patch does the API function rename and
replaces the existing calls in 1:1 manner, while the second patch
replaces the open code in emu10k1.


Takashi

[PATCH 11/11] ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()

[Takashi Iwai]

Now that snd_ctl_find_id() takes the locking itself, we can get rid of
the messy locking in the caller side in snd_emu10k1_verify_controls().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/emu10k1/emufx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 318a064f2cec..f114bda25eea 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -799,13 +799,10 @@ static int snd_emu10k1_verify_controls(struct snd_emu10k1 *emu,
 		if (snd_emu10k1_look_for_ctl(emu, &gctl->id))
 			continue;
 		gctl_id = (struct snd_ctl_elem_id *)&gctl->id;
-		down_read(&emu->card->controls_rwsem);
-		if (snd_ctl_find_id_locked(emu->card, gctl_id)) {
-			up_read(&emu->card->controls_rwsem);
+		if (snd_ctl_find_id(emu->card, gctl_id)) {
 			err = -EEXIST;
 			goto __error;
 		}
-		up_read(&emu->card->controls_rwsem);
 		if (gctl_id->iface != SNDRV_CTL_ELEM_IFACE_MIXER &&
 		    gctl_id->iface != SNDRV_CTL_ELEM_IFACE_PCM) {
 			err = -EINVAL;
-- 
2.35.3

Re: [PATCH 00/11] ALSA: Make control API taking controls_rwsem consistently

[Takashi Iwai]

On Tue, 18 Jul 2023 16:12:53 +0200,
Takashi Iwai wrote:
> 
> A few ALSA control API helpers like snd_ctl_rename(), snd_ctl_remove()
> and snd_ctl_find_*() suppose the callers taking card->controls_rwsem.
> But it's error-prone and fragile.  This patch set tries to change
> those API functions to take the card->controls>rwsem internally by
> themselves, so that the drivers don't need to take care of lockings.
> 
> After applying this patch set, only a couple of places still touch
> card->controls_rwsem (which are OK-ish as they need for traversing the
> control linked list).
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (11):
>   ALSA: control: Take card->controls_rwsem in snd_ctl_rename()
>   staging: greybus: audio_helper: Use snd_ctl_remove_id()
>   ASoC: atmel: mchp-pdmc: Use snd_ctl_remove_id()
>   ALSA: control: Take controls_rwsem lock in snd_ctl_remove()
>   ALSA: control: Add lockdep warning to internal functions
>   ASoC: sigmadsp: Simplify with snd_ctl_activate_id()
>   staging: greybus: Avoid abusing controls_rwsem
>   ALSA: control: Make snd_ctl_find_id() argument const
>   ALSA: control: Introduce unlocked version for snd_ctl_find_*() helpers
>   ALSA: control: Take lock in snd_ctl_find_id() and snd_ctl_find_numid()
>   ALSA: emu10k1: Go back and simplify with snd_ctl_find_id()

Now all merged.  The branch is based on 6.5-rc2, and tagged as
ctl-lock-fixes-6.6 so that other branch can merge this if needed.


Takashi