[PATCH] ASoC: DAPM: Cover regression by kctl change notification fix

The recent fix for DAPM to correct the kctl change notification by the commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") caused other regressions since it changed the behavior of snd_soc_dapm_set_pin() that is called from several API functions. Formerly it returned always 0 for success, but now it returns 0 or 1.
This patch addresses it, restoring the old behavior of snd_soc_dapm_set_pin() while keeping the fix in snd_soc_dapm_put_pin_switch().
Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") Reported-by: Yu-Hsuan Hsu yuhsuan@chromium.org Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/soc-dapm.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 2892b0aba151..b06c5682445c 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -2559,8 +2559,13 @@ static struct snd_soc_dapm_widget *dapm_find_widget( return NULL; }
-static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm, - const char *pin, int status) +/* + * set the DAPM pin status: + * returns 1 when the value has been updated, 0 when unchanged, or a negative + * error code; called from kcontrol put callback + */ +static int __snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm, + const char *pin, int status) { struct snd_soc_dapm_widget *w = dapm_find_widget(dapm, pin, true); int ret = 0; @@ -2586,6 +2591,18 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm, return ret; }
+/* + * similar as __snd_soc_dapm_set_pin(), but returns 0 when successful; + * called from several API functions below + */ +static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm, + const char *pin, int status) +{ + int ret = __snd_soc_dapm_set_pin(dapm, pin, status); + + return ret < 0 ? ret : 0; +} + /** * snd_soc_dapm_sync_unlocked - scan and power dapm paths * @dapm: DAPM context @@ -3589,10 +3606,10 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol, const char *pin = (const char *)kcontrol->private_value; int ret;
- if (ucontrol->value.integer.value[0]) - ret = snd_soc_dapm_enable_pin(&card->dapm, pin); - else - ret = snd_soc_dapm_disable_pin(&card->dapm, pin); + mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME); + ret = __snd_soc_dapm_set_pin(&card->dapm, pin, + !!ucontrol->value.integer.value[0]); + mutex_unlock(&card->dapm_mutex);
snd_soc_dapm_sync(&card->dapm); return ret;

On 2021-11-05 10:09 AM, Takashi Iwai wrote:
The recent fix for DAPM to correct the kctl change notification by the commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") caused other regressions since it changed the behavior of snd_soc_dapm_set_pin() that is called from several API functions. Formerly it returned always 0 for success, but now it returns 0 or 1.
This patch addresses it, restoring the old behavior of snd_soc_dapm_set_pin() while keeping the fix in snd_soc_dapm_put_pin_switch().
Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") Reported-by: Yu-Hsuan Hsu yuhsuan@chromium.org Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
Hello,
From my research I've found very few places which actually check the returned value of functions mentioned by you. And thus we could use this opportunity to adjust the kcontrol-put behavior according to documentation for all users without adding any additional functions which are part of this patch.
Board: sound/soc/intel/boards/kbl_da7219_max98927.c
seems to be the offending user. We could update its code instead, leaving ASoC unchanged. What do you think?
Regards Czarek

On Fri, 05 Nov 2021 12:30:48 +0100, Cezary Rojewski wrote:
On 2021-11-05 10:09 AM, Takashi Iwai wrote:
The recent fix for DAPM to correct the kctl change notification by the commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") caused other regressions since it changed the behavior of snd_soc_dapm_set_pin() that is called from several API functions. Formerly it returned always 0 for success, but now it returns 0 or 1.
This patch addresses it, restoring the old behavior of snd_soc_dapm_set_pin() while keeping the fix in snd_soc_dapm_put_pin_switch().
Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") Reported-by: Yu-Hsuan Hsu yuhsuan@chromium.org Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
Hello,
From my research I've found very few places which actually check the returned value of functions mentioned by you. And thus we could use this opportunity to adjust the kcontrol-put behavior according to documentation for all users without adding any additional functions which are part of this patch.
Board: sound/soc/intel/boards/kbl_da7219_max98927.c
seems to be the offending user. We could update its code instead, leaving ASoC unchanged. What do you think?
Well, if we're going to that direction, we have to update the documentation and properly mention about the positive return value. So the changes in ASoC core would be needed nevertheless.
I find it good to keep the old existing behavior; those API functions are only for enabling/disabling, so returning 0 or a negative error is more natural than tri-state, less complex for callers.
thanks,
Takashi

On 2021-11-05 12:36 PM, Takashi Iwai wrote:
On Fri, 05 Nov 2021 12:30:48 +0100, Cezary Rojewski wrote:
On 2021-11-05 10:09 AM, Takashi Iwai wrote:
The recent fix for DAPM to correct the kctl change notification by the commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") caused other regressions since it changed the behavior of snd_soc_dapm_set_pin() that is called from several API functions. Formerly it returned always 0 for success, but now it returns 0 or 1.
This patch addresses it, restoring the old behavior of snd_soc_dapm_set_pin() while keeping the fix in snd_soc_dapm_put_pin_switch().
Fixes: 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") Reported-by: Yu-Hsuan Hsu yuhsuan@chromium.org Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
Hello,
From my research I've found very few places which actually check the returned value of functions mentioned by you. And thus we could use this opportunity to adjust the kcontrol-put behavior according to documentation for all users without adding any additional functions which are part of this patch.
Board: sound/soc/intel/boards/kbl_da7219_max98927.c
seems to be the offending user. We could update its code instead, leaving ASoC unchanged. What do you think?
Well, if we're going to that direction, we have to update the documentation and properly mention about the positive return value. So the changes in ASoC core would be needed nevertheless.
I find it good to keep the old existing behavior; those API functions are only for enabling/disabling, so returning 0 or a negative error is more natural than tri-state, less complex for callers.
Hmm, indeed ASoC API doc would need an update, ASoC DAPM api != kcontrol api after all. My impression of original patch was the desire to align the behaviour of snd_soc_dapm_enable/disable_pin() and similar functions with kcontrol-put equivalents - in terms of the returned value. Perhaps that's not the case.
While it's preferred the keep the existing behaviour, if we were to align DAPM functions with kcontrol-put ones, fact that vast majority of users ignore the returned value, helps us out.
Regards, Czarek

On Fri, 5 Nov 2021 10:09:25 +0100, Takashi Iwai wrote:
The recent fix for DAPM to correct the kctl change notification by the commit 5af82c81b2c4 ("ASoC: DAPM: Fix missing kctl change notifications") caused other regressions since it changed the behavior of snd_soc_dapm_set_pin() that is called from several API functions. Formerly it returned always 0 for success, but now it returns 0 or 1.
This patch addresses it, restoring the old behavior of snd_soc_dapm_set_pin() while keeping the fix in snd_soc_dapm_put_pin_switch().
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus
Thanks!
[1/1] ASoC: DAPM: Cover regression by kctl change notification fix commit: 827b0913a9d9d07a0c3e559dbb20ca4d6d285a54
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Cezary Rojewski
-
Mark Brown
-
Takashi Iwai