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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2026-01-30 3:12 PM, Mark Brown wrote:
> On Fri, Jan 30, 2026 at 02:55:41PM +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.
> 
> Makes sense to me, although I fear it's about as likely that people
> enable this config as that they run mixer-test it's reasonable to be
> helpful.

Fair point. I admit I've noticed the problem in the existing 
sound/soc/intel/* drivers long time ago and.. forgot to address them due 
to lack of any "pings" in the logs.

alsa-utils' tools and alsa-lib do not allow to differentiate between 
success-no-changes and success-changes. Ideally userspace-level tests 
(functional tests) would be allowed to verify that. While in the long 
run updating/extending alsa-lib is probably the way to go, I believe 
having kernel-level check _now_ is not bad.

>> +	struct snd_ctl_elem_value original;
> 
>> +	ret = kctl->get(kctl, &original);
>> +	if (ret)
>> +		return ret;
> 
>> +	retcmp = memcmp(&original.value.bytes.data[0], &control->value.bytes.data[0],
>> +			sizeof(original.value.bytes.data[0]));
>> +	if (retcmp)
>> +		retcmp = 1;
> 
> original was just allocated from the stack so who knows what values it
> had originally, and the get() is only going to write the part of the
> value that has data since the normal get() path has a memset() in the
> core.  Similarly with the new value coming in from userspace there's no
> requirement for userspace to set anything that isn't part of the value
> being written to any particular value.  This means we're liable to get
> spurious mismatches.

You're right, I've based my patch on results from drivers that do init 
the space entirely. That may not be true for every driver in sound/.