[PATCH v6] ALSA: control: Verify put() result when in debug mode

[PATCH v6] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

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-1107    [002] .....   9.553687: snd_ctl_put: success: expected=0, actual=0 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
  amixer-1108    [003] .....   9.498897: snd_ctl_put: success: expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
  amixer-1111    [001] .....   9.882768: snd_ctl_put: success: expected=1, actual=1 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0
  amixer-1114    [002] .....   9.212184: snd_ctl_put: fail: expected=1, actual=0 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---

Changes in v6:
- the 'snd_ctl_put' event is now located in a new 'snd_ctl' trace/events
  category rather than being part of 'snd_pcm'
- provided an example of trace output for the new event within the
  commit message


Changes in v5:
- converged xxx_success() and xxx_fail() macros as suggested by Mark.
  The keyword "success" or "fail" is now denoted in the output of the
  trace event


Changes in v4:

- moved away from pr_xxx() in favor of trace events
- refactired the print format so that it aligns closer to 'amixer
  controls' output
- added 'value_buf' to 'struct snd_card' as suggested - to prevent stack
  overflow in snd_ctl_put_verify()


Changes in v3:

- simplified the memcmp() as suggested by Jaroslav
- added ->access verification for SKIP_CHECK and VOLATILE as suggested
  by Jaroslav and Takashi. For that very purpose I've deviced to use the
  kcontrol-volatile (vd) pointer that already part of
  snd_ctl_elem_write().

- as things are more complex now, replaced snd_ctl_put() macros with
  functions. This aligns with suggestion previously provided by Kuninori

- reordered operations within snd_ctl_put_verify() so that it's more
  intuitive to read, at least in my opinion:
  get/info/put -> info/get/put


Changes in v2:

A number of fixes as suggested by Mark and Takashi. The initial version
did not account for possibility of invalid payload sent from the userspace
and was buggy.
- enlisted ->info() operation and reused existing
  fill_remaining_elem_value() to sanitize the 'new' value provided by
  user
- fixed size provided to memcmp()
- the 'original' value is now initilized with memset()

Readability improvements suggested by Kuninori.
- added conditional #define snd_ctl_put() so that no additional
  if-statements are needed in the actual code.


 include/sound/core.h       |  3 ++
 sound/core/Makefile        |  1 +
 sound/core/control.c       | 76 +++++++++++++++++++++++++++++++++++++-
 sound/core/control_trace.h | 49 ++++++++++++++++++++++++
 sound/core/init.c          |  8 ++++
 5 files changed, 136 insertions(+), 1 deletion(-)
 create mode 100644 sound/core/control_trace.h

diff --git a/include/sound/core.h b/include/sound/core.h
index 64327e971122..4093ec82a0a1 100644
--- 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 31a0623cc89d..fdd3bb6e81a9 100644
--- 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 9c3fd5113a61..5cf6702f550d 100644
--- 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(card, kctl, iname, 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
index 000000000000..78b584d3b5ea
--- /dev/null
+++ b/sound/core/control_trace.h
@@ -0,0 +1,49 @@
+/* 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 <sound/control.h>
+#include <sound/core.h>
+
+TRACE_EVENT(snd_ctl_put,
+
+	TP_PROTO(struct snd_card *card, struct snd_kcontrol *kctl, const char *iname,
+		 int expected, int actual),
+
+	TP_ARGS(card, kctl, iname, expected, actual),
+
+	TP_STRUCT__entry(
+		__field(unsigned int,	card_id)
+		__field(unsigned int,	numid)
+		__string(kname,		kctl->id.name)
+		__string(iname,		iname)
+		__field(int,		expected)
+		__field(int,		actual)
+	),
+
+	TP_fast_assign(
+		__entry->card_id = card->number;
+		__entry->numid = kctl->id.numid;
+		__assign_str(kname);
+		__assign_str(iname);
+		__entry->expected = expected;
+		__entry->actual = actual;
+	),
+
+	TP_printk("%s: expected=%d, actual=%d for ctl numid=%d, iface=%s, name='%s', card id=%d\n",
+		  __entry->expected == __entry->actual ? "success" : "fail",
+		  __entry->expected, __entry->actual, __entry->numid,
+		  __get_str(iname), __get_str(kname), __entry->card_id)
+);
+
+#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 c372b3228785..15868dd50f6a 100644
--- 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;
 
@@ -537,6 +542,9 @@ void snd_card_disconnect(struct snd_card *card)
 		synchronize_irq(card->sync_irq);
 
 	snd_info_card_disconnect(card);
+#ifdef CONFIG_SND_CTL_DEBUG
+	kfree(card->value_buf);
+#endif
 #ifdef CONFIG_SND_DEBUG
 	debugfs_remove(card->debugfs_root);
 	card->debugfs_root = NULL;
-- 
2.34.1

Re: [PATCH v6] ALSA: control: Verify put() result when in debug mode

[Takashi Iwai]

On Mon, 23 Feb 2026 15:49:37 +0100,
Cezary Rojewski wrote:
> 
> 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-1107    [002] .....   9.553687: snd_ctl_put: success: expected=0, actual=0 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
>   amixer-1108    [003] .....   9.498897: snd_ctl_put: success: expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
>   amixer-1111    [001] .....   9.882768: snd_ctl_put: success: expected=1, actual=1 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0
>   amixer-1114    [002] .....   9.212184: snd_ctl_put: fail: expected=1, actual=0 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

Looks almost good, just a minor nitpicking:

> --- a/sound/core/init.c
> +++ b/sound/core/init.c
(snip)
> @@ -537,6 +542,9 @@ void snd_card_disconnect(struct snd_card *card)
>  		synchronize_irq(card->sync_irq);
>  
>  	snd_info_card_disconnect(card);
> +#ifdef CONFIG_SND_CTL_DEBUG
> +	kfree(card->value_buf);
> +#endif
>  #ifdef CONFIG_SND_DEBUG
>  	debugfs_remove(card->debugfs_root);
>  	card->debugfs_root = NULL;

IMO, it's better to be put in snd_card_do_free() which is the place to
release the actual resources.


thanks,

Takashi

Re: [PATCH v6] ALSA: control: Verify put() result when in debug mode

[Jaroslav Kysela]

On 2/23/26 15:49, Cezary Rojewski wrote:
> 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-1107    [002] .....   9.553687: snd_ctl_put: success: expected=0, actual=0 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
>    amixer-1108    [003] .....   9.498897: snd_ctl_put: success: expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master Playback Volume', card id=0
>    amixer-1111    [001] .....   9.882768: snd_ctl_put: success: expected=1, actual=1 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0
>    amixer-1114    [002] .....   9.212184: snd_ctl_put: fail: expected=1, actual=0 for ctl numid=5, iface=MIXER, name='Loopback Mute', card id=0

Index (and maybe device/subdevice) numbers are missing to get full control 
identification to follow amixer. Also, "card id" is usually referred to 
automatically or manually assigned ASCII card identifier/alias (like USB-audio 
or PCH). I would just use 'card=<number>' in TP_printk.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v6] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-02-24 2:21 PM, Jaroslav Kysela wrote:
> On 2/23/26 15:49, Cezary Rojewski wrote:
>> 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-1107    [002] .....   9.553687: snd_ctl_put: success: 
>> expected=0, actual=0 for ctl numid=1, iface=MIXER, name='Master 
>> Playback Volume', card id=0
>>    amixer-1108    [003] .....   9.498897: snd_ctl_put: success: 
>> expected=1, actual=1 for ctl numid=1, iface=MIXER, name='Master 
>> Playback Volume', card id=0
>>    amixer-1111    [001] .....   9.882768: snd_ctl_put: success: 
>> expected=1, actual=1 for ctl numid=5, iface=MIXER, name='Loopback 
>> Mute', card id=0
>>    amixer-1114    [002] .....   9.212184: snd_ctl_put: fail: 
>> expected=1, actual=0 for ctl numid=5, iface=MIXER, name='Loopback 
>> Mute', card id=0
> 
> Index (and maybe device/subdevice) numbers are missing to get full 
> control identification to follow amixer. Also, "card id" is usually 
> referred to automatically or manually assigned ASCII card identifier/ 
> alias (like USB-audio or PCH). I would just use 'card=<number>' in 
> TP_printk.


If the message length isn't a problem, then I'm going to add all of them 
in v7.

Re: [PATCH v6] ALSA: control: Verify put() result when in debug mode

[Cezary Rojewski]

On 2026-02-24 1:09 PM, Takashi Iwai wrote:
> On Mon, 23 Feb 2026 15:49:37 +0100,
> Cezary Rojewski wrote:

...

>> --- a/sound/core/init.c
>> +++ b/sound/core/init.c
> (snip)
>> @@ -537,6 +542,9 @@ void snd_card_disconnect(struct snd_card *card)
>>   		synchronize_irq(card->sync_irq);
>>   
>>   	snd_info_card_disconnect(card);
>> +#ifdef CONFIG_SND_CTL_DEBUG
>> +	kfree(card->value_buf);
>> +#endif
>>   #ifdef CONFIG_SND_DEBUG
>>   	debugfs_remove(card->debugfs_root);
>>   	card->debugfs_root = NULL;
> 
> IMO, it's better to be put in snd_card_do_free() which is the place to
> release the actual resources.

No problem, a question though - I've written the above to be cohesive as 
CONFIG_SND_DEBUG-part isn't part of snd_card_do_free() and what I'm 
adding is definitely also of debug-related capability. IIRC 
debugfs_remove() may invoke kfree() operations. Why isn't it part of 
snd_card_do_free() too?


Kind regards,
Czarek

Re: [PATCH v6] ALSA: control: Verify put() result when in debug mode

[Takashi Iwai]

On Tue, 24 Feb 2026 16:46:55 +0100,
Cezary Rojewski wrote:
> 
> On 2026-02-24 1:09 PM, Takashi Iwai wrote:
> > On Mon, 23 Feb 2026 15:49:37 +0100,
> > Cezary Rojewski wrote:
> 
> ...
> 
> >> --- a/sound/core/init.c
> >> +++ b/sound/core/init.c
> > (snip)
> >> @@ -537,6 +542,9 @@ void snd_card_disconnect(struct snd_card *card)
> >>   		synchronize_irq(card->sync_irq);
> >>     	snd_info_card_disconnect(card);
> >> +#ifdef CONFIG_SND_CTL_DEBUG
> >> +	kfree(card->value_buf);
> >> +#endif
> >>   #ifdef CONFIG_SND_DEBUG
> >>   	debugfs_remove(card->debugfs_root);
> >>   	card->debugfs_root = NULL;
> > 
> > IMO, it's better to be put in snd_card_do_free() which is the place to
> > release the actual resources.
> 
> No problem, a question though - I've written the above to be cohesive
> as CONFIG_SND_DEBUG-part isn't part of snd_card_do_free() and what I'm
> adding is definitely also of debug-related capability. IIRC
> debugfs_remove() may invoke kfree() operations. Why isn't it part of
> snd_card_do_free() too?

The *_disconnect() is a stage to stop the operation and disconnect the
interfaces from user-space, so debugfs_remove() is invoked there.
You can do kfree() if all things are detached, but in general we do
the release in a later phase, as some objects may be still alive.

snd_card_do_free() is the actual destructor for the resources, where
all things can be safely freed.


Takashi