[PATCH v3] ALSA: add snd_ctl_add_locked()

[PATCH v3] ALSA: add snd_ctl_add_locked()

[Oswald Buddenhagen]

This is in fact more symmetrical to snd_ctl_remove() than snd_ctl_add()
is - the former could be named snd_ctl_remove_locked() just as well.

One may argue that this is going in the wrong direction, as drivers have
no business managing the lock. This may be true in principle, but in
practice the vast majority of controls is created even before the device
was registered, and therefore before any locking is necessary at all.
That means that an even more radical approach of changing snd_ctl_add()
to do no locking, and converting the call sites that actually need
locking to a new function, would better match reality, and would be
somewhat more efficient at that. However, that seems a bit risky and way
too much work.

This will be used to dynamically change the available controls from
another control's put() callback, which is already locked.

One might want to add snd_ctl_replace_locked() for completeness, but I
have no use for it now.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---

applying this upstream would simplify applying the emu10k1 high bit-rate
patchset locally, as it would limit the affected modules to the driver
itself.

v3:
- fixed typo in commit message

v2:
- extended commit message
---
 include/sound/control.h |  1 +
 sound/core/control.c    | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/sound/control.h b/include/sound/control.h
index cc3dcc6cfb0f..d4e210831a38 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -134,6 +134,7 @@ void snd_ctl_notify_one(struct snd_card * card, unsigned int mask, struct snd_kc
 struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new * kcontrolnew, void * private_data);
 void snd_ctl_free_one(struct snd_kcontrol * kcontrol);
 int snd_ctl_add(struct snd_card * card, struct snd_kcontrol * kcontrol);
+int snd_ctl_add_locked(struct snd_card * card, struct snd_kcontrol * kcontrol);
 int snd_ctl_remove(struct snd_card * card, struct snd_kcontrol * kcontrol);
 int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, bool add_on_replace);
 int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id);
diff --git a/sound/core/control.c b/sound/core/control.c
index 8386b53acdcd..1a0bb76dd8e7 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -527,6 +527,27 @@ static int snd_ctl_add_replace(struct snd_card *card,
 	return err;
 }
 
+static int snd_ctl_add_replace_locked(struct snd_card *card,
+				      struct snd_kcontrol *kcontrol,
+				      enum snd_ctl_add_mode mode)
+{
+	int err = -EINVAL;
+
+	if (! kcontrol)
+		return err;
+	if (snd_BUG_ON(!card || !kcontrol->info))
+		goto error;
+
+	err = __snd_ctl_add_replace(card, kcontrol, mode);
+	if (err < 0)
+		goto error;
+	return 0;
+
+ error:
+	snd_ctl_free_one(kcontrol);
+	return err;
+}
+
 /**
  * snd_ctl_add - add the control instance to the card
  * @card: the card instance
@@ -547,6 +568,16 @@ int snd_ctl_add(struct snd_card *card, struct snd_kcontrol *kcontrol)
 }
 EXPORT_SYMBOL(snd_ctl_add);
 
+/**
+ * snd_ctl_add_locked - same as snd_ctl_add(), but card->controls_rwsem
+ * is expected to be already locked if necessary.
+ */
+int snd_ctl_add_locked(struct snd_card *card, struct snd_kcontrol *kcontrol)
+{
+	return snd_ctl_add_replace_locked(card, kcontrol, CTL_ADD_EXCLUSIVE);
+}
+EXPORT_SYMBOL(snd_ctl_add_locked);
+
 /**
  * snd_ctl_replace - replace the control instance of the card
  * @card: the card instance
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v3] ALSA: add snd_ctl_add_locked()

[Takashi Iwai]

On Mon, 17 Jul 2023 12:20:36 +0200,
Oswald Buddenhagen wrote:
> 
> This is in fact more symmetrical to snd_ctl_remove() than snd_ctl_add()
> is - the former could be named snd_ctl_remove_locked() just as well.

FYI, the situation about snd_ctl_remove() will change.  I already have
patch set to make snd_ctl_remove() locking by itself, as a part of API
cleanup / consistency improvements.  You can find it in
test/snd_ctl_remove-lock-fix branch of sound.git tree.


Takashi

[PATCH v4] ALSA: add snd_ctl_add_locked() & export snd_ctl_remove_locked()

[Oswald Buddenhagen]

This will be used to dynamically change the available controls from
another control's put() callback, which is already locked.

One might want to add snd_ctl_replace_locked() for completeness, but I
have no use for it now.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---

applying this upstream would simplify applying the emu10k1 high bit-rate
patchset locally, as it would limit the affected modules to the driver
itself.

v4:
- adjust to recent locking changes
- mark exports as internal

v3:
- fixed typo in commit message

v2:
- extended commit message
---
 include/sound/control.h |  2 ++
 sound/core/control.c    | 43 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index 42e8dbb22d8e..6b85df6aba96 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -133,7 +133,9 @@ void snd_ctl_notify_one(struct snd_card * card, unsigned int mask, struct snd_kc
 
 struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new * kcontrolnew, void * private_data);
 void snd_ctl_free_one(struct snd_kcontrol * kcontrol);
+int snd_ctl_add_locked(struct snd_card *card, struct snd_kcontrol *kcontrol);
 int snd_ctl_add(struct snd_card * card, struct snd_kcontrol * kcontrol);
+int snd_ctl_remove_locked(struct snd_card *card, struct snd_kcontrol *kcontrol);
 int snd_ctl_remove(struct snd_card * card, struct snd_kcontrol * kcontrol);
 int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, bool add_on_replace);
 int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id);
diff --git a/sound/core/control.c b/sound/core/control.c
index e13e9d6b3b89..d21d7f773772 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -39,9 +39,6 @@ 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;
@@ -509,6 +506,27 @@ static int __snd_ctl_add_replace(struct snd_card *card,
 	return 0;
 }
 
+static int snd_ctl_add_replace_locked(struct snd_card *card,
+				      struct snd_kcontrol *kcontrol,
+				      enum snd_ctl_add_mode mode)
+{
+	int err = -EINVAL;
+
+	if (! kcontrol)
+		return err;
+	if (snd_BUG_ON(!card || !kcontrol->info))
+		goto error;
+
+	err = __snd_ctl_add_replace(card, kcontrol, mode);
+	if (err < 0)
+		goto error;
+	return 0;
+
+ error:
+	snd_ctl_free_one(kcontrol);
+	return err;
+}
+
 static int snd_ctl_add_replace(struct snd_card *card,
 			       struct snd_kcontrol *kcontrol,
 			       enum snd_ctl_add_mode mode)
@@ -532,6 +550,16 @@ static int snd_ctl_add_replace(struct snd_card *card,
 	return err;
 }
 
+/**
+ * snd_ctl_add_locked - same as snd_ctl_add(), but card->controls_rwsem
+ * is expected to be already locked if necessary.
+ */
+int snd_ctl_add_locked(struct snd_card *card, struct snd_kcontrol *kcontrol)
+{
+	return snd_ctl_add_replace_locked(card, kcontrol, CTL_ADD_EXCLUSIVE);
+}
+EXPORT_SYMBOL_GPL(snd_ctl_add_locked);
+
 /**
  * snd_ctl_add - add the control instance to the card
  * @card: the card instance
@@ -596,11 +624,16 @@ 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)
+/**
+ * snd_ctl_remove_locked - same as snd_ctl_remove(), but card->controls_rwsem
+ * is expected to be already locked if necessary.
+ */
+int snd_ctl_remove_locked(struct snd_card *card,
+			  struct snd_kcontrol *kcontrol)
 {
 	return __snd_ctl_remove(card, kcontrol, true);
 }
+EXPORT_SYMBOL_GPL(snd_ctl_remove_locked);
 
 /**
  * snd_ctl_remove - remove the control from the card and release it
-- 
2.40.0.152.g15d061e6df