[PATCH v2 0/3] ASoC: Use snd_ctl_find_id_mixer() instead of open-coding
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.
Changes in V2: Only change is in the KUnit test (patch #3) to make the const strings more consty.
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 | 184 ++++++++++++++++++++++++++++++++++++++ sound/soc/soc-card.c | 21 +---- 5 files changed, 223 insertions(+), 17 deletions(-) create mode 100644 sound/soc/soc-card-test.c
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);
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; + if (unlikely(!name)) + return NULL;
- down_read(&card->controls_rwsem); - kctl = snd_soc_card_get_kcontrol_locked(soc_card, name); - up_read(&card->controls_rwsem); - - return kctl; + return snd_ctl_find_id_mixer(soc_card->snd_card, name); } EXPORT_SYMBOL_GPL(snd_soc_card_get_kcontrol);
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 | 184 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 196 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..075c52fe82e5 --- /dev/null +++ b/sound/soc/soc-card-test.c @@ -0,0 +1,184 @@ +// 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 struct snd_kcontrol_new test_card_controls[] = { + SOC_SINGLE("Fee", SND_SOC_NOPM, 0, 1, 0), + SOC_SINGLE("Fi", SND_SOC_NOPM, 1, 1, 0), + SOC_SINGLE("Fo", SND_SOC_NOPM, 2, 1, 0), + SOC_SINGLE("Fum", SND_SOC_NOPM, 3, 1, 0), + SOC_SINGLE("Left Fee", SND_SOC_NOPM, 4, 1, 0), + SOC_SINGLE("Right Fee", SND_SOC_NOPM, 5, 1, 0), + SOC_SINGLE("Left Fi", SND_SOC_NOPM, 6, 1, 0), + SOC_SINGLE("Right Fi", SND_SOC_NOPM, 7, 1, 0), + SOC_SINGLE("Left Fo", SND_SOC_NOPM, 8, 1, 0), + SOC_SINGLE("Right Fo", SND_SOC_NOPM, 9, 1, 0), + SOC_SINGLE("Left Fum", SND_SOC_NOPM, 10, 1, 0), + SOC_SINGLE("Right Fum", SND_SOC_NOPM, 11, 1, 0), +}; + +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_card_controls); ++i) { + kc = snd_soc_card_get_kcontrol(card, test_card_controls[i].name); + KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kc, "Failed to find '%s'\n", + test_card_controls[i].name); + 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_card_controls[i].name); + } + + /* 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_card_controls); ++i) { + down_read(&card->snd_card->controls_rwsem); + kc = snd_soc_card_get_kcontrol_locked(card, test_card_controls[i].name); + up_read(&card->snd_card->controls_rwsem); + KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kc, "Failed to find '%s'\n", + test_card_controls[i].name); + 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_card_controls[i].name); + + down_write(&card->snd_card->controls_rwsem); + kcw = snd_soc_card_get_kcontrol_locked(card, test_card_controls[i].name); + up_write(&card->snd_card->controls_rwsem); + KUNIT_EXPECT_NOT_ERR_OR_NULL_MSG(test, kcw, "Failed to find '%s'\n", + test_card_controls[i].name); + + 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");
On Mon, 01 Apr 2024 12:02:07 +0200, Richard Fitzgerald wrote:
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.
Changes in V2: Only change is in the KUnit test (patch #3) to make the const strings more consty.
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
I suppose this goes via Mark's tree. Feel free to take my ack:
Reviewed-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
On Mon, 01 Apr 2024 10:02:07 +0000, Richard Fitzgerald wrote:
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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ALSA: control: Introduce snd_ctl_find_id_mixer_locked() commit: 08ea486a61451189b190c7b89e406b889cf693fa [2/3] ASoC: soc-card: Use snd_ctl_find_id_mixer() instead of open-coding commit: 897cc72b08374c1224a9ded03c82dfc8e41f80c2 [3/3] ASoC: soc-card: Add KUnit test case for snd_soc_card_get_kcontrol commit: ef7784e41db73f3d31ce545227ebba4483479a26
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
participants (3)
-
Mark Brown
-
Richard Fitzgerald
-
Takashi Iwai