Reported 4.4.y-st issue from Flamefire #cip

Reported 4.4.y-st issue from Flamefire #cip

[Chris Paterson]

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

Re: [cip-dev] Reported 4.4.y-st issue from Flamefire #cip

[Pavel Machek]

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

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

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

Re: Reported 4.4.y-st issue from Flamefire #cip

[theflamefire89]

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

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

[-- Attachment #2: Type: text/html, Size: 404 bytes --]

Re: [cip-dev] Reported 4.4.y-st issue from Flamefire #cip

[Pavel Machek]

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

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

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

Re: Reported 4.4.y-st issue from Flamefire #cip

[theflamefire89]

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

> 
> 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

[-- Attachment #2: Type: text/html, Size: 1350 bytes --]