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

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

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

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.


 sound/core/control.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 9c3fd5113a61..18ddae138795 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1264,6 +1264,60 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	return result;
 }
 
+#if IS_ENABLED(CONFIG_SND_CTL_DEBUG)
+static int snd_ctl_put_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
+{
+	struct snd_ctl_elem_value original;
+	struct snd_ctl_elem_info info;
+	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;
+
+	if (retcmp == ret)
+		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
+	else
+		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
+	return ret;
+}
+
+static int snd_ctl_put(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(kctl, control);
+}
+#else
+static inline int snd_ctl_put(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 +1354,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(kctl, control, vd->access);
+
 	if (result < 0) {
 		up_write(&card->controls_rwsem);
 		return result;
-- 
2.34.1

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

[Takashi Iwai]

On Fri, 06 Feb 2026 12:32:50 +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.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

Thanks, it looks better now.

A few remaining concerns:

> ---
> 
> 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.
> 
> 
>  sound/core/control.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 9c3fd5113a61..18ddae138795 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1264,6 +1264,60 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
>  	return result;
>  }
>  
> +#if IS_ENABLED(CONFIG_SND_CTL_DEBUG)
> +static int snd_ctl_put_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
> +{
> +	struct snd_ctl_elem_value original;
> +	struct snd_ctl_elem_info info;

Those will consume too much on stack.
Maybe snd_ctl_elem_info is acceptable, but snd_ctl_elem_value is too
big.  That's why we use a temporary kmalloc() for storing the values
in other places, and I'm afraid you'd need to follow it here, too.

If the performance matters, we may introduce a shared
snd_ctl_elem_value object with some protection, too.  IIRC, this code
path is already protected exclusively with card->control_rwsem write,
it can be per-card basis.

> +	if (retcmp == ret)
> +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
> +	else
> +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);

So this prints out a message at each access even if it succeeds.
I believe this would flood too many messages unnecessarily.  Can it be
better to be with debug level?

Also, this message doesn't show any relevant information about which
card, device and which control caused the error, and that makes
debugging harder.  Put some prefix to identify the problematic
control.


thanks,

Takashi

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

On Fri, Feb 06, 2026 at 01:28:45PM +0100, Takashi Iwai wrote:
> Cezary Rojewski wrote:

> > +	if (retcmp == ret)
> > +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
> > +	else
> > +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);

> So this prints out a message at each access even if it succeeds.
> I believe this would flood too many messages unnecessarily.  Can it be
> better to be with debug level?

> Also, this message doesn't show any relevant information about which
> card, device and which control caused the error, and that makes
> debugging harder.  Put some prefix to identify the problematic
> control.

Might it be worth considering doing this via trace_printk() or possibly
tracepoints?  They're very low overhead and with tracepoints they're
strutured so they're more suitable for very high volume logging.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

On Fri, 06 Feb 2026 15:08:28 +0100,
Mark Brown wrote:
> 
> On Fri, Feb 06, 2026 at 01:28:45PM +0100, Takashi Iwai wrote:
> > Cezary Rojewski wrote:
> 
> > > +	if (retcmp == ret)
> > > +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
> > > +	else
> > > +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
> 
> > So this prints out a message at each access even if it succeeds.
> > I believe this would flood too many messages unnecessarily.  Can it be
> > better to be with debug level?
> 
> > Also, this message doesn't show any relevant information about which
> > card, device and which control caused the error, and that makes
> > debugging harder.  Put some prefix to identify the problematic
> > control.
> 
> Might it be worth considering doing this via trace_printk() or possibly
> tracepoints?  They're very low overhead and with tracepoints they're
> strutured so they're more suitable for very high volume logging.

If we want to record each kcontrol access indeed, then yes, I agree
with tracepoint as the best way to go.  But, IIRC, trace_printk()
leads to a kernel warning, so better to avoid.


Takashi

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Fri, Feb 06, 2026 at 03:33:34PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Might it be worth considering doing this via trace_printk() or possibly
> > tracepoints?  They're very low overhead and with tracepoints they're
> > strutured so they're more suitable for very high volume logging.

> If we want to record each kcontrol access indeed, then yes, I agree
> with tracepoint as the best way to go.  But, IIRC, trace_printk()
> leads to a kernel warning, so better to avoid.

It does cause a kernel warning, but given that this is only going to be
turned on with SND_DEBUG and probably shouldn't be on in production
(given the extra read on each write) that doesn't seem unreasonable?
I'm fairly sure tracepoints by themselves don't cause a warning (they're
there all the time in all my subsystems anyway) so would be more
suitable if the complaint about trace_printk() is an issue.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Cezary Rojewski]

On 2026-02-06 3:44 PM, Mark Brown wrote:
> On Fri, Feb 06, 2026 at 03:33:34PM +0100, Takashi Iwai wrote:
>> Mark Brown wrote:
> 
>>> Might it be worth considering doing this via trace_printk() or possibly
>>> tracepoints?  They're very low overhead and with tracepoints they're
>>> strutured so they're more suitable for very high volume logging.
> 
>> If we want to record each kcontrol access indeed, then yes, I agree
>> with tracepoint as the best way to go.  But, IIRC, trace_printk()
>> leads to a kernel warning, so better to avoid.
> 
> It does cause a kernel warning, but given that this is only going to be
> turned on with SND_DEBUG and probably shouldn't be on in production
> (given the extra read on each write) that doesn't seem unreasonable?
> I'm fairly sure tracepoints by themselves don't cause a warning (they're
> there all the time in all my subsystems anyway) so would be more
> suitable if the complaint about trace_printk() is an issue.

Since tracepoints are part of sound/core already (e.g.: pcm_trace.h) 
I'll probably go that route. In regard to warnings, I do not see 
anything and Intel drivers utilize them plenty.

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

[Cezary Rojewski]

On 2026-02-06 1:28 PM, Takashi Iwai wrote:
> On Fri, 06 Feb 2026 12:32:50 +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.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> Thanks, it looks better now.
> 
> A few remaining concerns:
> 
>> ---
>>
>> 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.
>>
>>
>>   sound/core/control.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/core/control.c b/sound/core/control.c
>> index 9c3fd5113a61..18ddae138795 100644
>> --- a/sound/core/control.c
>> +++ b/sound/core/control.c
>> @@ -1264,6 +1264,60 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
>>   	return result;
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_SND_CTL_DEBUG)
>> +static int snd_ctl_put_verify(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *control)
>> +{
>> +	struct snd_ctl_elem_value original;
>> +	struct snd_ctl_elem_info info;
> 
> Those will consume too much on stack.
> Maybe snd_ctl_elem_info is acceptable, but snd_ctl_elem_value is too
> big.  That's why we use a temporary kmalloc() for storing the values
> in other places, and I'm afraid you'd need to follow it here, too.
> 
> If the performance matters, we may introduce a shared
> snd_ctl_elem_value object with some protection, too.  IIRC, this code
> path is already protected exclusively with card->control_rwsem write,
> it can be per-card basis.

TBH, I thought going for ~1K on the stack is fine, as IIRC, the typical 
limit is 4K. This function should not be folded into any of its callers 
either.

Since the idea is to move away from on-the-stack approach and, a global 
instance of struct snd_ctl_elem_value is no-go, guess the per-card 
buffer is the direction. And yes, I'd like to avoid memory allocation 
for every single kctl->put().

> 
>> +	if (retcmp == ret)
>> +		pr_info("kctl->put() returned the expected value of '%d'\n", ret);
>> +	else
>> +		pr_warn("expected kctl->put() to return '%d' but got '%d'\n", ret, retcmp);
> 
> So this prints out a message at each access even if it succeeds.
> I believe this would flood too many messages unnecessarily.  Can it be
> better to be with debug level?
> 
> Also, this message doesn't show any relevant information about which
> card, device and which control caused the error, and that makes
> debugging harder.  Put some prefix to identify the problematic
> control.

I agree, this is a little bit bogus without more context. Will add 
tracepoints as suggested and more descriptive message.

Didn't see the problem as our tests do write to /dev/kmsg or the trace 
equivalent before _and_ after doing operations such as 
control-manipulation and later dump all the dmesgs and traces to the log 
files.


Kind regards,
Czarek

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Fri, Feb 06, 2026 at 06:45:08PM +0100, Cezary Rojewski wrote:

> TBH, I thought going for ~1K on the stack is fine, as IIRC, the typical
> limit is 4K. This function should not be folded into any of its callers
> either.

We trigger build warnings on individual stack frames at a much lower
limit.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Cezary Rojewski]

On 2026-02-06 1:28 PM, Takashi Iwai wrote:
> On Fri, 06 Feb 2026 12:32:50 +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

...

> Also, this message doesn't show any relevant information about which
> card, device and which control caused the error, and that makes
> debugging harder.  Put some prefix to identify the problematic
> control.

Quick peak to what I'm currently logging - output from 'cat trace':


#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
           amixer-1177    [003] .....    97.421022: snd_ctl_put_success: 
expected=0, actual=0 for ctl "Master Playback Volume" of numid=1 from 
dev="bdw_rt286", card="broadwell-rt286"

           amixer-1181    [003] .....    99.065279: snd_ctl_put_success: 
expected=1, actual=1 for ctl "Master Playback Volume" of numid=1 from 
dev="bdw_rt286", card="broadwell-rt286"

           amixer-1185    [003] .....   144.007205: snd_ctl_put_fail: 
expected=1, actual=0 for ctl "Loopback Mute" of numid=5 from 
dev="bdw_rt286", card="broadwell-rt286"


The format for trace_snd_ctl_put event class:

         TP_printk("expected=%d, actual=%d for ctl \"%s\" of numid=%d 
from dev=\"%s\", card=\"%s\"\n",
                   __entry->expected, __entry->actual,
                   __get_str(kname), __entry->numid,
                   __get_str(dname), __get_str(cname))


kname = kctl->id.name
dname = dev_name(card->dev)
cname = shortname or longname, shortname prio

Is there something still to be added?

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

[Jaroslav Kysela]

On 2/9/26 15:43, Cezary Rojewski wrote:
> On 2026-02-06 1:28 PM, Takashi Iwai wrote:
>> On Fri, 06 Feb 2026 12:32:50 +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
> 
> ...
> 
>> Also, this message doesn't show any relevant information about which
>> card, device and which control caused the error, and that makes
>> debugging harder.  Put some prefix to identify the problematic
>> control.
> 
> Quick peak to what I'm currently logging - output from 'cat trace':
> 

...

          amixer-1185    [003] .....   144.007205: snd_ctl_put_fail:
> expected=1, actual=0 for ctl "Loopback Mute" of numid=5 from
> dev="bdw_rt286", card="broadwell-rt286"
> 
> 
> The format for trace_snd_ctl_put event class:
> 
>           TP_printk("expected=%d, actual=%d for ctl \"%s\" of numid=%d
> from dev=\"%s\", card=\"%s\"\n",
>                     __entry->expected, __entry->actual,
>                     __get_str(kname), __entry->numid,
>                     __get_str(dname), __get_str(cname))
> 
> 
> kname = kctl->id.name
> dname = dev_name(card->dev)
> cname = shortname or longname, shortname prio

I would trace just card number (the driver/names can be obtained with other 
tools), but add control index and control interface number. See 'amixer 
controls' output for hints.

					Jaroslav

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

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

[Cezary Rojewski]

On 2026-02-09 4:14 PM, Jaroslav Kysela wrote:
> On 2/9/26 15:43, Cezary Rojewski wrote:
>> On 2026-02-06 1:28 PM, Takashi Iwai wrote:
>>> On Fri, 06 Feb 2026 12:32:50 +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

...

>>> Also, this message doesn't show any relevant information about which
>>> card, device and which control caused the error, and that makes
>>> debugging harder.  Put some prefix to identify the problematic
>>> control.
>>
>> Quick peak to what I'm currently logging - output from 'cat trace':
>>
> 
> ...
> 
>           amixer-1185    [003] .....   144.007205: snd_ctl_put_fail:
>> expected=1, actual=0 for ctl "Loopback Mute" of numid=5 from
>> dev="bdw_rt286", card="broadwell-rt286"
>>
>>
>> The format for trace_snd_ctl_put event class:
>>
>>           TP_printk("expected=%d, actual=%d for ctl \"%s\" of numid=%d
>> from dev=\"%s\", card=\"%s\"\n",
>>                     __entry->expected, __entry->actual,
>>                     __get_str(kname), __entry->numid,
>>                     __get_str(dname), __get_str(cname))
>>
>>
>> kname = kctl->id.name
>> dname = dev_name(card->dev)
>> cname = shortname or longname, shortname prio
> 
> I would trace just card number (the driver/names can be obtained with 
> other tools), but add control index and control interface number. See 
> 'amixer controls' output for hints.

I like the idea of aligning closer to the output of ALSA's standard 
apps. For the rest of reviewers, pasting exemplary output from my test 
device:

	test@BDW-I2S-1:~$ amixer -c0 controls
	numid=12,iface=CARD,name='Headphone Jack'
	numid=11,iface=CARD,name='Mic Jack'
	numid=1,iface=MIXER,name='Master Playback Volume'
	(and the list goes on)

Not sure about "but add control index" part though. I'm already dumping 
kctl->id.numid and, kctl->id.index is nowhere to be found in 'amixer -c0 
controls'. At least not on my end.

Example after the change:

	snd_ctl_put_fail: expected=1, actual=0 for ctl numid=5, iface=MIXER, 
name="Loopback Mute", card id=0

Looks good?