Hello kernel team,

An has been reported an issue with 4.4.y-st on IRC.
Forwarding it here so all can see.

<quote>
Hi guys. I can't make it to the meeting(s) but wanted to let you know that a recent commit to the 4.4.y-st branch (and potentially others) introduced an annoying bug. Commit link: https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/commit/sound/soc/soc-ops.c?h=linux-4.4.y-st&id=7a78bde1faa42e0057350378baf518a04b3bae58

The issue is that "val > mc->platform_max" check which would need to be adjusted by "min" as done for a similar function in https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/commit/sound/soc/soc-ops.c?h=linux-4.4.y-st&id=e41846dbe6f49d305c1a8fc229c5deabd66e3e24

Others already did that (e.g. https://github.com/baunilla/android_kernel_xiaomi_rosy/commit/969b9d366c1e9564e173aea325ec544dcd7804ff) but I've only found a mention in the kernel ML but seemingly it wasn't resolved

This bug (in short) e.g. leads to wired headsets being unusable on phones using this kernel. Due to the to-eager check the mic volume isn't changed leading to a feedback loop.
Hope that helps.
</quote>

Thank you Flamefire for reporting!

Kind regards, Chris

Hi!

<quote>
Hi guys. I can't make it to the meeting(s) but wanted to let you know that a recent commit to the 4.4.y-st branch (and potentially others) introduced an annoying bug. Commit link: https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/commit/sound/soc/soc-ops.c?h=linux-4.4.y-st&id=7a78bde1faa42e0057350378baf518a04b3bae58

The issue is that "val > mc->platform_max" check which would need to be adjusted by "min" as done for a similar function in https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/commit/sound/soc/soc-ops.c?h=linux-4.4.y-st&id=e41846dbe6f49d305c1a8fc229c5deabd66e3e24

Others already did that (e.g. https://github.com/baunilla/android_kernel_xiaomi_rosy/commit/969b9d366c1e9564e173aea325ec544dcd7804ff) but I've only found a mention in the kernel ML but seemingly it wasn't resolved

This bug (in short) e.g. leads to wired headsets being unusable on phones using this kernel. Due to the to-eager check the mic volume isn't changed leading to a feedback loop.
Hope that helps.

Thanks for the report.

Unfortunately, mainline seems to be different here. Looking at the
code, is min < 0 in your case?

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi,

> Unfortunately, mainline seems to be different here. Looking at the
> code, is min < 0 in your case?

yes "min" is negative. IIRC min and max are centered around zero. And as linked above a similar function has the adjustment and others independently came up with the same fix. I also verified that it fixes my issue.

Best Regards,

Alex

Hi!

Unfortunately, mainline seems to be different here. Looking at the
code, is min < 0 in your case?

yes "min" is negative. IIRC min and max are centered around zero. And as linked above a similar function has the adjustment and others independently came up with the same fix. I also verified that it fixes my issue.

Upstream said this code is correct and affected drivers should be
fixed, instead, so we don't plan to do anything here.

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Upstream said this code is correct and affected drivers should be
fixed, instead,

First this is a practical issue: There are proprietary drivers which likely will never get fixed.
And second I do not agree that this code is correct as it doesn't make sense to me:

- From snd_soc_put_volsw_sx:

if (mc->platform_max && val > mc->platform_max)
	return -EINVAL;
if (val > max - min)
	return -EINVAL;

And from snd_soc_info_volsw called by snd_soc_info_volsw_sx

mc->platform_max = mc->max;

So if platform_max gets always set to max why would you compare `val` directly against platform_max but subtract min from max?

Or did upstream say that using a negative "min" is invalid? Example where this is used: https://github.com/sonyxperiadev/kernel/blob/5f3e8612a373f035fae72471dcbc37ec2110adde/sound/soc/codecs/wcd9335.c#L2255-L2256

I also find the following puzzling (in snd_soc_limit_volume):

if (max <= mc->max) {
	mc->platform_max = max;

i.e. a check against "max" setting "platform_max"?

Anyway I appreciate you taking care of this and hope the above input helps in guiding a decision.

Best Regards,

Alex