On Thu, May 19, 2022 at 02:56:34AM +0300, Tan Nayır wrote:
For a control defined like this: -- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume", WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) -- This is what the snd_soc_info_volsw_sx reports: $mc->platform_max:40, $mc->max:40, $mc->min:-84, $uinfo->value.integer.max:40, $uinfo->value.integer.min:0
OK, so anything setting a value outside of 0..40 was buggy. Note that we've not changed the info() code at all here, snd_soc_info_volsw() subtracts min and then snd_soc_volsw_sx() adds it back on so what we end up with is whatever max was set to reported as the maximum to userspace, with the userpace minimum hard coded to zero meaning the range the control has is 0..max.
So the min and max fields inside the $mc are the same in snd_soc_put_volsw_sx so this means that the code without my patch has an incorrect check.
The check is enforcing the constraint we advertised to userspace, which should be all that any well written userpace application has accessed (though I appreciate that due to lack of bounds checking in the ALSA core it's been possible to do so).
Is the $mc->platform_max supposed to be set to the number of steps as opposed to the maximum value?
It is hard to understand why one would set platform_max in the above situation other than to limit to -44, however there *is* a lot of confusion in the code since in the generic function it gets substituted in like a register value.
Also the snd_soc_put_volsw_sx still checks the value from userspace which has a range of 0 to 124 against the maximum of the signed range which is from -84 to 40 regardless of the patches below.
65c7d020fbee8 ("ASoC: Update the Max value of integer controls.") https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/65c7d020fbee8070f3...
That looks confused since it makes the interpretation of platform_max depend on if the control has a negative bottom for the range which isn't going to help with maintainability...
0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value") https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af4...
...which I guess is why that bit of the change is reverted in this commit, though that then has two different interpretations of platform_max depending on if the control is an integer control for some reason I can't fathom. These two would need to be squashed together for upstreaming, but note that these controls were added by and are used by non-Qualcomm people (see 34198710f55b5 ASoC: Add info callback for SX_TLV controls), and note the comment in there about the max being set to the number of levels rather than a value, so I'm concerned about other users here, the code doesn't look as self consistent as it should be.
I think these controls need a separate, clearly written, info() callback rather than trying to bodge on the side of the main one. That would help a lot with working out if the put() is consistent with it. We probably also need an audit of all the existing users to try to figure out what they think they're doing and what, if anything, it's consistent with. Your patch is clearly not consistent with the info() callback as it stands if nothing else.