ALSA: control: Verify put() result when in debug mode

The put() operation is expected to return:
1) 0 on success if no changes were made
2) 1 on success if changes were made
3) error code otherwise

Currently 2) is usually ignored when writing control-operations. While
forcing compliance is not an option right now, make it easier for
developers to adhere to the expectations and notice problems by logging
them when CONFIG_SND_CTL_DEBUG is enabled.

Due to large size of struct snd_ctl_elem_value, 'value_buf' is provided
as a reusable buffer for kctl->put() verification. This prevents
exhausting the stack when verifying the operation.

>From user perspective, patch introduces a new trace/events category
'snd_ctl' containing a single 'snd_ctl_put' event type. Log sample:

  amixer-1086    [003] .....    8.035939: snd_ctl_put: success: expected=0, actual=0 for ctl numid=1, iface=MIXER, name='Master Playback Volume', index=0, device=0, subdevice=0, card=0
  amixer-1087    [003] .....    8.938721: snd_ctl_put: success: expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master Playback Volume', index=0, device=0, subdevice=0, card=0
  amixer-1088    [003] .....    9.631470: snd_ctl_put: success: expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master Playback Volume', index=0, device=0, subdevice=0, card=0
  amixer-1089    [000] .....    9.636786: snd_ctl_put: fail: expected=1, actual=0 for ctl numid=5, iface=MIXER, name='Loopback Mute', index=0, device=0, subdevice=0, card=0

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Jaroslav Kysela <perex@perex.cz>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Link: https://patch.msgid.link/20260224205619.584795-1-cezary.rojewski@intel.com

diff --git a/include/sound/core.h b/include/sound/core.h
index 64327e97112220969e03ba52571e2993d59df70e..4093ec82a0a12bfb267cdde6ff4093434bf25476 100644 (file)
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -133,6 +133,9 @@ struct snd_card {
 #ifdef CONFIG_SND_DEBUG
        struct dentry *debugfs_root;    /* debugfs root for card */
 #endif
+#ifdef CONFIG_SND_CTL_DEBUG
+       struct snd_ctl_elem_value *value_buf; /* buffer for kctl->put() verification */
+#endif
 
 #ifdef CONFIG_PM
        unsigned int power_state;       /* power state */

diff --git a/sound/core/Makefile b/sound/core/Makefile
index 31a0623cc89d1273376ade5297ca0cdea3d0f57c..fdd3bb6e81a993da1f3a27ab676cc35fe390286b 100644 (file)
--- a/sound/core/Makefile
+++ b/sound/core/Makefile
@@ -23,6 +23,7 @@ snd-pcm-$(CONFIG_SND_PCM_IEC958) += pcm_iec958.o
 # for trace-points
 CFLAGS_pcm_lib.o := -I$(src)
 CFLAGS_pcm_native.o := -I$(src)
+CFLAGS_control.o := -I$(src)
 
 snd-pcm-dmaengine-y := pcm_dmaengine.o
 

diff --git a/sound/core/control.c b/sound/core/control.c
index 934e84e93838063f3f3ff1f31fe009d9464c47ed..374e703d15a91c6d9ff03dfc3c771724dd9ebc00 100644 (file)
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -19,6 +19,13 @@
 #include <sound/info.h>
 #include <sound/control.h>
 
+#ifdef CONFIG_SND_CTL_DEBUG
+#define CREATE_TRACE_POINTS
+#include "control_trace.h"
+#else
+#define trace_snd_ctl_put(card, kctl, iname, expected, actual)
+#endif
+
 // Max allocation size for user controls.
 static int max_user_ctl_alloc_size = 8 * 1024 * 1024;
 module_param_named(max_user_ctl_alloc_size, max_user_ctl_alloc_size, int, 0444);
@@ -1264,6 +1271,72 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
        return result;
 }
 
+#if IS_ENABLED(CONFIG_SND_CTL_DEBUG)
+
+static const char *const snd_ctl_elem_iface_names[] = {
+       [SNDRV_CTL_ELEM_IFACE_CARD]             = "CARD",
+       [SNDRV_CTL_ELEM_IFACE_HWDEP]            = "HWDEP",
+       [SNDRV_CTL_ELEM_IFACE_MIXER]            = "MIXER",
+       [SNDRV_CTL_ELEM_IFACE_PCM]              = "PCM",
+       [SNDRV_CTL_ELEM_IFACE_RAWMIDI]          = "RAWMIDI",
+       [SNDRV_CTL_ELEM_IFACE_TIMER]            = "TIMER",
+       [SNDRV_CTL_ELEM_IFACE_SEQUENCER]        = "SEQUENCER",
+};
+
+static int snd_ctl_put_verify(struct snd_card *card, struct snd_kcontrol *kctl,
+                             struct snd_ctl_elem_value *control)
+{
+       struct snd_ctl_elem_value *original = card->value_buf;
+       struct snd_ctl_elem_info info;
+       const char *iname;
+       int ret, retcmp;
+
+       memset(original, 0, sizeof(*original));
+       memset(&info, 0, sizeof(info));
+
+       ret = kctl->info(kctl, &info);
+       if (ret)
+               return ret;
+
+       ret = kctl->get(kctl, original);
+       if (ret)
+               return ret;
+
+       ret = kctl->put(kctl, control);
+       if (ret < 0)
+               return ret;
+
+       /* Sanitize the new value (control->value) before comparing. */
+       fill_remaining_elem_value(control, &info, 0);
+
+       /* With known state for both new and original, do the comparison. */
+       retcmp = memcmp(&original->value, &control->value, sizeof(original->value));
+       if (retcmp)
+               retcmp = 1;
+
+       iname = snd_ctl_elem_iface_names[kctl->id.iface];
+       trace_snd_ctl_put(&kctl->id, iname, card->number, ret, retcmp);
+
+       return ret;
+}
+
+static int snd_ctl_put(struct snd_card *card, struct snd_kcontrol *kctl,
+                      struct snd_ctl_elem_value *control, unsigned int access)
+{
+       if ((access & SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK) ||
+           (access & SNDRV_CTL_ELEM_ACCESS_VOLATILE))
+               return kctl->put(kctl, control);
+
+       return snd_ctl_put_verify(card, kctl, control);
+}
+#else
+static inline int snd_ctl_put(struct snd_card *card, struct snd_kcontrol *kctl,
+                             struct snd_ctl_elem_value *control, unsigned int access)
+{
+       return kctl->put(kctl, control);
+}
+#endif
+
 static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
                              struct snd_ctl_elem_value *control)
 {
@@ -1300,7 +1373,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
                                                           false);
        }
        if (!result)
-               result = kctl->put(kctl, control);
+               result = snd_ctl_put(card, kctl, control, vd->access);
+
        if (result < 0) {
                up_write(&card->controls_rwsem);
                return result;

diff --git a/sound/core/control_trace.h b/sound/core/control_trace.h
new file mode 100644 (file)
index 0000000..d30e654
--- /dev/null
+++ b/sound/core/control_trace.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM snd_ctl
+
+#if !defined(_TRACE_SND_CTL_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_SND_CTL_H
+
+#include <linux/tracepoint.h>
+#include <uapi/sound/asound.h>
+
+TRACE_EVENT(snd_ctl_put,
+
+       TP_PROTO(struct snd_ctl_elem_id *id, const char *iname, unsigned int card,
+                int expected, int actual),
+
+       TP_ARGS(id, iname, card, expected, actual),
+
+       TP_STRUCT__entry(
+               __field(unsigned int,   numid)
+               __string(iname,         iname)
+               __string(kname,         id->name)
+               __field(unsigned int,   index)
+               __field(unsigned int,   device)
+               __field(unsigned int,   subdevice)
+               __field(unsigned int,   card)
+               __field(int,            expected)
+               __field(int,            actual)
+       ),
+
+       TP_fast_assign(
+               __entry->numid = id->numid;
+               __assign_str(iname);
+               __assign_str(kname);
+               __entry->index = id->index;
+               __entry->device = id->device;
+               __entry->subdevice = id->subdevice;
+               __entry->card = card;
+               __entry->expected = expected;
+               __entry->actual = actual;
+       ),
+
+       TP_printk("%s: expected=%d, actual=%d for ctl numid=%d, iface=%s, name='%s', index=%d, device=%d, subdevice=%d, card=%d\n",
+                 __entry->expected == __entry->actual ? "success" : "fail",
+                 __entry->expected, __entry->actual, __entry->numid,
+                 __get_str(iname), __get_str(kname), __entry->index,
+                 __entry->device, __entry->subdevice, __entry->card)
+);
+
+#endif /* _TRACE_SND_CTL_H */
+
+/* This part must be outside protection */
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#define TRACE_INCLUDE_FILE control_trace
+#include <trace/define_trace.h>

diff --git a/sound/core/init.c b/sound/core/init.c
index 2f1bd9cbdbedfbfa275a4279ea174dc226418ce3..0c316189e94769c055ae5165c403f207dff8ba52 100644 (file)
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -362,6 +362,11 @@ static int snd_card_init(struct snd_card *card, struct device *parent,
 #ifdef CONFIG_SND_DEBUG
        card->debugfs_root = debugfs_create_dir(dev_name(&card->card_dev),
                                                sound_debugfs_root);
+#endif
+#ifdef CONFIG_SND_CTL_DEBUG
+       card->value_buf = kmalloc(sizeof(*card->value_buf), GFP_KERNEL);
+       if (!card->value_buf)
+               return -ENOMEM;
 #endif
        return 0;
 
@@ -587,6 +592,9 @@ static int snd_card_do_free(struct snd_card *card)
        snd_device_free_all(card);
        if (card->private_free)
                card->private_free(card);
+#ifdef CONFIG_SND_CTL_DEBUG
+       kfree(card->value_buf);
+#endif
        if (snd_info_card_free(card) < 0) {
                dev_warn(card->dev, "unable to free card info\n");
                /* Not fatal error */