[PATCH 0/3] ASoC: Use snd_ctl_find_id_mixer() instead of open-coding

[PATCH 0/3] ASoC: Use snd_ctl_find_id_mixer() instead of open-coding

[Richard Fitzgerald]

The first two patches change snd_soc_card_get_kcontrol() to use the
core snd_ctl_find_id_mixer() functionality instead of open-coding its
own list walk.

The last patch adds a KUnit test for this, which was tested on the
original and modified code.

Richard Fitzgerald (3):
  ALSA: control: Introduce snd_ctl_find_id_mixer_locked()
  ASoC: soc-card: Use snd_ctl_find_id_mixer() instead of open-coding
  ASoC: soc-card: Add KUnit test case for snd_soc_card_get_kcontrol

 include/sound/control.h   |  23 +++++
 sound/soc/Kconfig         |   8 ++
 sound/soc/Makefile        |   4 +
 sound/soc/soc-card-test.c | 191 ++++++++++++++++++++++++++++++++++++++
 sound/soc/soc-card.c      |  21 +----
 5 files changed, 230 insertions(+), 17 deletions(-)
 create mode 100644 sound/soc/soc-card-test.c

-- 
2.39.2

[PATCH 1/3] ALSA: control: Introduce snd_ctl_find_id_mixer_locked()

[Richard Fitzgerald]

Adds wrapper function snd_ctl_find_id_mixer_locked(). This is
identical to snd_ctl_find_id_mixer() except that it can be called
from code that is already holding controls_rwsem.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/sound/control.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/sound/control.h b/include/sound/control.h
index 9a4f4f7138da..c1659036c4a7 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -167,6 +167,29 @@ snd_ctl_find_id_mixer(struct snd_card *card, const char *name)
 	return snd_ctl_find_id(card, &id);
 }
 
+/**
+ * snd_ctl_find_id_mixer_locked - find the control instance with the given name string
+ * @card: the card instance
+ * @name: the name string
+ *
+ * Finds the control instance with the given name and
+ * @SNDRV_CTL_ELEM_IFACE_MIXER. Other fields are set to zero.
+ *
+ * This is merely a wrapper to snd_ctl_find_id_locked().
+ * The caller must down card->controls_rwsem before calling this function.
+ *
+ * Return: The pointer of the instance if found, or %NULL if not.
+ */
+static inline struct snd_kcontrol *
+snd_ctl_find_id_mixer_locked(struct snd_card *card, const char *name)
+{
+	struct snd_ctl_elem_id id = {};
+
+	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	strscpy(id.name, name, sizeof(id.name));
+	return snd_ctl_find_id_locked(card, &id);
+}
+
 int snd_ctl_create(struct snd_card *card);
 
 int snd_ctl_register_ioctl(snd_kctl_ioctl_func_t fcn);
-- 
2.39.2

[PATCH 2/3] ASoC: soc-card: Use snd_ctl_find_id_mixer() instead of open-coding

[Richard Fitzgerald]

Use the snd_ctl_find_id_mixer[_locked]() wrapper in
snd_soc_card_get_kcontrol[_locked]() instead of open-coding a custom
list walk of the card controls list.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/soc-card.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/sound/soc/soc-card.c b/sound/soc/soc-card.c
index 8a2f163da6bc..0a3104d4ad23 100644
--- a/sound/soc/soc-card.c
+++ b/sound/soc/soc-card.c
@@ -32,33 +32,20 @@ static inline int _soc_card_ret(struct snd_soc_card *card,
 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;
 
-	list_for_each_entry(kctl, &card->controls, list)
-		if (!strncmp(kctl->id.name, name, sizeof(kctl->id.name)))
-			return kctl;
-	return NULL;
+	return snd_ctl_find_id_mixer_locked(soc_card->snd_card, name);
 }
 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);
+	if (unlikely(!name))
+		return NULL;
 
-	return kctl;
+	return snd_ctl_find_id_mixer(soc_card->snd_card, name);
 }
 EXPORT_SYMBOL_GPL(snd_soc_card_get_kcontrol);
 
-- 
2.39.2

[PATCH 3/3] ASoC: soc-card: Add KUnit test case for snd_soc_card_get_kcontrol

[Richard Fitzgerald]

Add a new snd-soc-card KUnit test with a simple test case for
snd_soc_card_get_kcontrol() and snd_soc_card_get_kcontrol_locked().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/Kconfig         |   8 ++
 sound/soc/Makefile        |   4 +
 sound/soc/soc-card-test.c | 191 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 sound/soc/soc-card-test.c

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 439fa631c342..a52afb423b46 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -66,6 +66,14 @@ config SND_SOC_TOPOLOGY_KUNIT_TEST
 	  userspace applications such as pulseaudio, to prevent unnecessary
 	  problems.
 
+config SND_SOC_CARD_KUNIT_TEST
+	tristate "KUnit tests for SoC card"
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  If you want to perform tests on ALSA SoC card functions say Y here.
+	  If unsure, say N.
+
 config SND_SOC_UTILS_KUNIT_TEST
 	tristate "KUnit tests for SoC utils"
 	depends on KUNIT
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 8376fdb217ed..f90f5300b36e 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -12,6 +12,10 @@ ifneq ($(CONFIG_SND_SOC_TOPOLOGY_KUNIT_TEST),)
 obj-$(CONFIG_SND_SOC_TOPOLOGY_KUNIT_TEST) += soc-topology-test.o
 endif
 
+ifneq ($(CONFIG_SND_SOC_CARD_KUNIT_TEST),)
+obj-$(CONFIG_SND_SOC_CARD_KUNIT_TEST) += soc-card-test.o
+endif
+
 ifneq ($(CONFIG_SND_SOC_UTILS_KUNIT_TEST),)
 # snd-soc-test-objs := soc-utils-test.o
 obj-$(CONFIG_SND_SOC_UTILS_KUNIT_TEST) += soc-utils-test.o
diff --git a/sound/soc/soc-card-test.c b/sound/soc/soc-card-test.c
new file mode 100644
index 000000000000..48c71bd1b6b1
--- /dev/null
+++ b/sound/soc/soc-card-test.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2024 Cirrus Logic, Inc. and
+//                    Cirrus Logic International Semiconductor Ltd.
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+#include <linux/module.h>
+#include <sound/control.h>
+#include <sound/soc.h>
+#include <sound/soc-card.h>
+
+struct soc_card_test_priv {
+	struct device *card_dev;
+	struct snd_soc_card *card;
+};
+
+static const char * const test_control_names[] = {
+	"Fee", "Fi", "Fo", "Fum",
+	"Left Fee", "Right Fee", "Left Fi", "Right Fi",
+	"Left Fo", "Right Fo", "Left Fum", "Right Fum",
+};
+
+static const struct snd_kcontrol_new test_card_controls[] = {
+	SOC_SINGLE(test_control_names[0], SND_SOC_NOPM, 0, 1, 0),
+	SOC_SINGLE(test_control_names[1], SND_SOC_NOPM, 1, 1, 0),
+	SOC_SINGLE(test_control_names[2], SND_SOC_NOPM, 2, 1, 0),
+	SOC_SINGLE(test_control_names[3], SND_SOC_NOPM, 3, 1, 0),
+	SOC_SINGLE(test_control_names[4], SND_SOC_NOPM, 4, 1, 0),
+	SOC_SINGLE(test_control_names[5], SND_SOC_NOPM, 5, 1, 0),
+	SOC_SINGLE(test_control_names[6], SND_SOC_NOPM, 6, 1, 0),
+	SOC_SINGLE(test_control_names[7], SND_SOC_NOPM, 7, 1, 0),
+	SOC_SINGLE(test_control_names[8], SND_SOC_NOPM, 8, 1, 0),
+	SOC_SINGLE(test_control_names[9], SND_SOC_NOPM, 9, 1, 0),
+	SOC_SINGLE(test_control_names[10], SND_SOC_NOPM, 10, 1, 0),
+	SOC_SINGLE(test_control_names[11], SND_SOC_NOPM, 11, 1, 0),
+};
+static_assert(ARRAY_SIZE(test_control_names) == ARRAY_SIZE(test_card_controls));
+
+static void test_snd_soc_card_get_kcontrol(struct kunit *test)
+{
+	struct soc_card_test_priv *priv = test->priv;
+	struct snd_soc_card *card = priv->card;
+	struct snd_kcontrol *kc;
+	struct soc_mixer_control *mc;
+	int i, ret;
+
+	ret = snd_soc_add_card_controls(card, test_card_controls, ARRAY_SIZE(test_card_controls));
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/* Look up every control */
+	for (i = 0; i < ARRAY_SIZE(test_control_names); ++i) {
+		kc = snd_soc_card_get_kcontrol(card, test_control_names[i]);
+		KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kc, "Failed to find '%s'\n",
+						 test_control_names[i]);
+		if (!kc)
+			continue;
+
+		/* Test that it is the correct control */
+		mc = (struct soc_mixer_control *)kc->private_value;
+		KUNIT_EXPECT_EQ_MSG(test, mc->shift, i, "For '%s'\n", test_control_names[i]);
+	}
+
+	/* Test some names that should not be found */
+	kc = snd_soc_card_get_kcontrol(card, "None");
+	KUNIT_EXPECT_NULL(test, kc);
+
+	kc = snd_soc_card_get_kcontrol(card, "Left None");
+	KUNIT_EXPECT_NULL(test, kc);
+
+	kc = snd_soc_card_get_kcontrol(card, "Left");
+	KUNIT_EXPECT_NULL(test, kc);
+
+	kc = snd_soc_card_get_kcontrol(card, NULL);
+	KUNIT_EXPECT_NULL(test, kc);
+}
+
+static void test_snd_soc_card_get_kcontrol_locked(struct kunit *test)
+{
+	struct soc_card_test_priv *priv = test->priv;
+	struct snd_soc_card *card = priv->card;
+	struct snd_kcontrol *kc, *kcw;
+	struct soc_mixer_control *mc;
+	int i, ret;
+
+	ret = snd_soc_add_card_controls(card, test_card_controls, ARRAY_SIZE(test_card_controls));
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/* Look up every control */
+	for (i = 0; i < ARRAY_SIZE(test_control_names); ++i) {
+		down_read(&card->snd_card->controls_rwsem);
+		kc = snd_soc_card_get_kcontrol_locked(card, test_control_names[i]);
+		up_read(&card->snd_card->controls_rwsem);
+		KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kc, "Failed to find '%s'\n",
+						 test_control_names[i]);
+		if (!kc)
+			continue;
+
+		/* Test that it is the correct control */
+		mc = (struct soc_mixer_control *)kc->private_value;
+		KUNIT_EXPECT_EQ_MSG(test, mc->shift, i, "For '%s'\n", test_control_names[i]);
+
+		down_write(&card->snd_card->controls_rwsem);
+		kcw = snd_soc_card_get_kcontrol_locked(card, test_control_names[i]);
+		up_write(&card->snd_card->controls_rwsem);
+		KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kcw, "Failed to find '%s'\n",
+						 test_control_names[i]);
+
+		KUNIT_EXPECT_PTR_EQ(test, kc, kcw);
+	}
+
+	/* Test some names that should not be found */
+	down_read(&card->snd_card->controls_rwsem);
+	kc = snd_soc_card_get_kcontrol_locked(card, "None");
+	up_read(&card->snd_card->controls_rwsem);
+	KUNIT_EXPECT_NULL(test, kc);
+
+	down_read(&card->snd_card->controls_rwsem);
+	kc = snd_soc_card_get_kcontrol_locked(card, "Left None");
+	up_read(&card->snd_card->controls_rwsem);
+	KUNIT_EXPECT_NULL(test, kc);
+
+	down_read(&card->snd_card->controls_rwsem);
+	kc = snd_soc_card_get_kcontrol_locked(card, "Left");
+	up_read(&card->snd_card->controls_rwsem);
+	KUNIT_EXPECT_NULL(test, kc);
+
+	down_read(&card->snd_card->controls_rwsem);
+	kc = snd_soc_card_get_kcontrol_locked(card, NULL);
+	up_read(&card->snd_card->controls_rwsem);
+	KUNIT_EXPECT_NULL(test, kc);
+}
+
+static int soc_card_test_case_init(struct kunit *test)
+{
+	struct soc_card_test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	test->priv = priv;
+
+	priv->card_dev = kunit_device_register(test, "sound-soc-card-test");
+	priv->card_dev = get_device(priv->card_dev);
+	if (!priv->card_dev)
+		return -ENODEV;
+
+	priv->card = kunit_kzalloc(test, sizeof(*priv->card), GFP_KERNEL);
+	if (!priv->card)
+		return -ENOMEM;
+
+	priv->card->name = "soc-card-test";
+	priv->card->dev = priv->card_dev;
+	priv->card->owner = THIS_MODULE;
+
+	ret = snd_soc_register_card(priv->card);
+	if (!ret)
+		return ret;
+
+	return 0;
+}
+
+static void soc_card_test_case_exit(struct kunit *test)
+{
+	struct soc_card_test_priv *priv = test->priv;
+
+	if (priv->card)
+		snd_soc_unregister_card(priv->card);
+
+	if (priv->card_dev)
+		put_device(priv->card_dev);
+}
+
+static struct kunit_case soc_card_test_cases[] = {
+	KUNIT_CASE(test_snd_soc_card_get_kcontrol),
+	KUNIT_CASE(test_snd_soc_card_get_kcontrol_locked),
+	{}
+};
+
+static struct kunit_suite soc_card_test_suite = {
+	.name = "soc-card",
+	.test_cases = soc_card_test_cases,
+	.init = soc_card_test_case_init,
+	.exit = soc_card_test_case_exit,
+};
+
+kunit_test_suites(&soc_card_test_suite);
+
+MODULE_DESCRIPTION("ASoC soc-card KUnit test");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH 3/3] ASoC: soc-card: Add KUnit test case for snd_soc_card_get_kcontrol

[kernel test robot]

Hi Richard,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-sound/for-next]
[also build test ERROR on tiwai-sound/for-next tiwai-sound/for-linus linus/master v6.9-rc1 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Fitzgerald/ALSA-control-Introduce-snd_ctl_find_id_mixer_locked/20240325-210026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20240325125650.213913-4-rf%40opensource.cirrus.com
patch subject: [PATCH 3/3] ASoC: soc-card: Add KUnit test case for snd_soc_card_get_kcontrol
config: powerpc-randconfig-r081-20240328 (https://download.01.org/0day-ci/archive/20240328/202403281952.Sz5iE5Tm-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240328/202403281952.Sz5iE5Tm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403281952.Sz5iE5Tm-lkp@intel.com/

All errors (new ones prefixed by >>):

>> sound/soc/soc-card-test.c:24:13: error: initializer element is not a compile-time constant
           SOC_SINGLE(test_control_names[0], SND_SOC_NOPM, 0, 1, 0),
                      ^~~~~~~~~~~~~~~~~~~~~
   include/sound/soc.h:61:48: note: expanded from macro 'SOC_SINGLE'
   {       .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
                                                        ^~~~~
   1 error generated.


vim +24 sound/soc/soc-card-test.c

    22	
    23	static const struct snd_kcontrol_new test_card_controls[] = {
  > 24		SOC_SINGLE(test_control_names[0], SND_SOC_NOPM, 0, 1, 0),
    25		SOC_SINGLE(test_control_names[1], SND_SOC_NOPM, 1, 1, 0),
    26		SOC_SINGLE(test_control_names[2], SND_SOC_NOPM, 2, 1, 0),
    27		SOC_SINGLE(test_control_names[3], SND_SOC_NOPM, 3, 1, 0),
    28		SOC_SINGLE(test_control_names[4], SND_SOC_NOPM, 4, 1, 0),
    29		SOC_SINGLE(test_control_names[5], SND_SOC_NOPM, 5, 1, 0),
    30		SOC_SINGLE(test_control_names[6], SND_SOC_NOPM, 6, 1, 0),
    31		SOC_SINGLE(test_control_names[7], SND_SOC_NOPM, 7, 1, 0),
    32		SOC_SINGLE(test_control_names[8], SND_SOC_NOPM, 8, 1, 0),
    33		SOC_SINGLE(test_control_names[9], SND_SOC_NOPM, 9, 1, 0),
    34		SOC_SINGLE(test_control_names[10], SND_SOC_NOPM, 10, 1, 0),
    35		SOC_SINGLE(test_control_names[11], SND_SOC_NOPM, 11, 1, 0),
    36	};
    37	static_assert(ARRAY_SIZE(test_control_names) == ARRAY_SIZE(test_card_controls));
    38	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki