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

[PATCH v5] 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.

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

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..48afb56c5b71
--- /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_pcm
+
+#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 v5] ALSA: control: Verify put() result when in debug mode

[Takashi Iwai]

On Thu, 19 Feb 2026 10:46:14 +0100,
Cezary Rojewski wrote:
> --- /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_pcm

It doesn't look like a right place?


thanks,

Takashi

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

[Takashi Iwai]

On Thu, 19 Feb 2026 11:08:12 +0100,
Takashi Iwai wrote:
> 
> On Thu, 19 Feb 2026 10:46:14 +0100,
> Cezary Rojewski wrote:
> > --- /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_pcm
> 
> It doesn't look like a right place?

Also, it'd be more helpful if you put example outputs in the
description, too.  Then one can have a clue what to look for.


Takashi

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

[Cezary Rojewski]

On 2026-02-19 11:09 AM, Takashi Iwai wrote:
> On Thu, 19 Feb 2026 11:08:12 +0100,
> Takashi Iwai wrote:
>>
>> On Thu, 19 Feb 2026 10:46:14 +0100,
>> Cezary Rojewski wrote:
>>> --- /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_pcm
>>
>> It doesn't look like a right place?
> 
> Also, it'd be more helpful if you put example outputs in the
> description, too.  Then one can have a clue what to look for.

Agreed, an example in the commit message would be nice.

In regard to the "trace system", not a fan of adding a new "system" just 
for the sake of a single event. If we are going that route, I'd rather 
reword "snd_pcm" to "snd" or something similar. From sound-core 
perspective, I believe there should be 1 directory for the events 
declared in sound/core. The event-names are prefixed already, further 
division looks redundant.

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

[Takashi Iwai]

On Thu, 19 Feb 2026 11:15:41 +0100,
Cezary Rojewski wrote:
> 
> On 2026-02-19 11:09 AM, Takashi Iwai wrote:
> > On Thu, 19 Feb 2026 11:08:12 +0100,
> > Takashi Iwai wrote:
> >> 
> >> On Thu, 19 Feb 2026 10:46:14 +0100,
> >> Cezary Rojewski wrote:
> >>> --- /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_pcm
> >> 
> >> It doesn't look like a right place?
> > 
> > Also, it'd be more helpful if you put example outputs in the
> > description, too.  Then one can have a clue what to look for.
> 
> Agreed, an example in the commit message would be nice.
> 
> In regard to the "trace system", not a fan of adding a new "system"
> just for the sake of a single event.

Why not?  We may extend the tracing of controls, too, and it'll make
easier to enable all tracepoints relevant either for PCM or control.

> If we are going that route, I'd
> rather reword "snd_pcm" to "snd" or something similar. From sound-core
> perspective, I believe there should be 1 directory for the events
> declared in sound/core. The event-names are prefixed already, further
> division looks redundant.

That'll be messy under /sys/kernel/debug/tracing/events/*; it'll
become snd/applptr, snd/hwptr, etc.  So, if we want to consolidate
into a single directory, we need to put "pcm_" prefix again to each.

Or maybe better to use subdirectories "snd/pcm" and "snd/ctl" instead.


thanks,

Takashi

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

[Jaroslav Kysela]

On 2/19/26 10:46, Cezary Rojewski wrote:
> +	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)

Trace also the control index. I know that ASoC ignores this field to allow 
simple string concat (easy name stacking), but we have drivers which are using 
those integer index numbers.

			Thanks,
				Jaroslav

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

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

[Cezary Rojewski]

On 2026-02-19 11:36 AM, Jaroslav Kysela wrote:
> On 2/19/26 10:46, Cezary Rojewski wrote:
>> +    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)
> 
> Trace also the control index. I know that ASoC ignores this field to 
> allow simple string concat (easy name stacking), but we have drivers 
> which are using those integer index numbers.

amixer seems not to do so. Isn't ctl's numid sufficient? The reason I'm 
asking is that the message is quite long as is already.

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

[Cezary Rojewski]

On 2026-02-19 11:30 AM, Takashi Iwai wrote:
> On Thu, 19 Feb 2026 11:15:41 +0100,
> Cezary Rojewski wrote:

...

>> In regard to the "trace system", not a fan of adding a new "system"
>> just for the sake of a single event.
> 
> Why not?  We may extend the tracing of controls, too, and it'll make
> easier to enable all tracepoints relevant either for PCM or control.
> 
>> If we are going that route, I'd
>> rather reword "snd_pcm" to "snd" or something similar. From sound-core
>> perspective, I believe there should be 1 directory for the events
>> declared in sound/core. The event-names are prefixed already, further
>> division looks redundant.
> 
> That'll be messy under /sys/kernel/debug/tracing/events/*; it'll
> become snd/applptr, snd/hwptr, etc.  So, if we want to consolidate
> into a single directory, we need to put "pcm_" prefix again to each.
> 
> Or maybe better to use subdirectories "snd/pcm" and "snd/ctl" instead.

Well, I'm taking example from existing directories e.g.: 
trace/events/power or trace/events/sched. TBH, trace/event/asoc fits 
here too - all of them just group events together. sched_kthread_xxx are 
in the same bucket as sched_process_xxx (as are plenty of others). The 
'name' alone is enough of a filter. I believe that until one reaches 
certain number of event-types, there is no reason to dilute the hierarchy.

However, looking at the subject from a high-level perspective, I think 
it isn't in the scope of this patch - I can align to any suggested 
pattern and we can work on the details in a dedicated series.


Kind regards,
Czakre

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

[Jaroslav Kysela]

On 2/19/26 17:54, Cezary Rojewski wrote:
> On 2026-02-19 11:36 AM, Jaroslav Kysela wrote:
>> On 2/19/26 10:46, Cezary Rojewski wrote:
>>> +    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)
>>
>> Trace also the control index. I know that ASoC ignores this field to
>> allow simple string concat (easy name stacking), but we have drivers
>> which are using those integer index numbers.
> 
> amixer seems not to do so. Isn't ctl's numid sufficient? The reason I'm
> asking is that the message is quite long as is already.

Amixer prints index and also device and subdevice numbers when they are not 
zero. See function snd_ctl_ascii_elem_id_get() in [1].

Jaroslav

[1] 
https://github.com/alsa-project/alsa-lib/blob/master/src/control/ctlparse.c#L117


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

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

[Cezary Rojewski]

On 2026-02-24 2:17 PM, Jaroslav Kysela wrote:
> On 2/19/26 17:54, Cezary Rojewski wrote:
>> On 2026-02-19 11:36 AM, Jaroslav Kysela wrote:
>>> On 2/19/26 10:46, Cezary Rojewski wrote:
>>>> +    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)
>>>
>>> Trace also the control index. I know that ASoC ignores this field to
>>> allow simple string concat (easy name stacking), but we have drivers
>>> which are using those integer index numbers.
>>
>> amixer seems not to do so. Isn't ctl's numid sufficient? The reason I'm
>> asking is that the message is quite long as is already.
> 
> Amixer prints index and also device and subdevice numbers when they are 
> not zero. See function snd_ctl_ascii_elem_id_get() in [1].
> 
> Jaroslav
> 
> [1] https://github.com/alsa-project/alsa-lib/blob/master/src/control/ 
> ctlparse.c#L117

Thank you for the explanation. I've never seen such output when working 
with asoc-based drivers. Looking at the code now I know why.