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

[Takashi Iwai <tiwai@suse.de>]

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