SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver
Hi all,
I am using a dev board with a Cirrus Logic cs24l51 codec.
This used to work fine prior to kernel version 5.x, however after 5.x it is not possible to set certain values for ALSA controls from userspace.
I believe this is related to the input validation that is mentioned in this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/, and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
For the cs24l51, all the controls that fail are using the SOC_DOUBLE_R_SX_TLV macro.
I have traced this to the checks in snd_soc_put_volsw_sx, specifically the (val > max - min) check:
pr_warn("Max: %d, Min: %d, Value: %d", max, min, val); pr_warn("platform_max: %d", mc->platform_max); if (mc->platform_max && val > mc->platform_max) { return -EINVAL; } if (val > max - min){ pr_warn("(val > max - min) check failed"); return -EINVAL; } if (val < 0) return -EINVAL;
According to the datasheet of the CS24L51, section 6.13, page 61, the PCMMIXX_VOL registers accept the following range of values:
Binary Code / Volume Setting 001 1000 / +12.0 dB ··· ··· 000 0000 / 0 dB 111 1111 / -0.5 dB 111 1110 / -1.0 dB ··· ··· 001 1001 / -51.5 dB
Minimum value is 0x19 (001 1001) corresponding to -51.5 dB, then there are 127 possible gain settings from -51.5 dB to +12.0 dB, in 0.5 dB steps.
This is declared in the driver as follows:
SOC_DOUBLE_R_SX_TLV("PCM Playback Volume", CS42L51_PCMA_VOL, CS42L51_PCMB_VOL, 0, 0x19, 0x7F, adc_pcm_tlv),
0x19 = min value 0x7F = number of gain settings
This seems to be correct according to the semantics of the SOC_DOUBLE_R_SX_TLV macro as described in commit 34198710f55b5f359f43e67d9a08fe5aadfbca1b ("ASoC: Add info callback for SX_TLV controls").
However, the (val > max - min) check in snd_soc_put_volsw_sx fails in the above example because val = 127, max - min = 127 - 25 = 102.
So I am not sure how this should be fixed. Is the SX_TLV macro being used incorrectly here? Is the check in snd_soc_put_volsw_sx wrong?
Any pointers are welcome.
Thanks,
(If possible, please CC me in any replies)
Guillermo Rodriguez Garcia guille.rodriguez@gmail.com
On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
Hi all,
I am using a dev board with a Cirrus Logic cs24l51 codec.
This used to work fine prior to kernel version 5.x, however after 5.x it is not possible to set certain values for ALSA controls from userspace.
I believe this is related to the input validation that is mentioned in this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/, and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
For the cs24l51, all the controls that fail are using the SOC_DOUBLE_R_SX_TLV macro.
I have traced this to the checks in snd_soc_put_volsw_sx, specifically the (val > max - min) check:
Can you try these two patches:
https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@...
It looks like they somehow got lost around 5.18, I have been meaning to chase up what happened to them.
Thanks, Charles
El jue, 24 nov 2022 a las 12:13, Charles Keepax (ckeepax@opensource.cirrus.com) escribió:
On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
Hi all,
I am using a dev board with a Cirrus Logic cs24l51 codec.
This used to work fine prior to kernel version 5.x, however after 5.x it is not possible to set certain values for ALSA controls from userspace.
I believe this is related to the input validation that is mentioned in this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/, and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
For the cs24l51, all the controls that fail are using the SOC_DOUBLE_R_SX_TLV macro.
I have traced this to the checks in snd_soc_put_volsw_sx, specifically the (val > max - min) check:
Can you try these two patches:
https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@...
Thanks. In my tests, these patches seem to fix the problem for some values, but not for all of them:
$ amixer cset name='Analog Playback Volume' '208','208' numid=3,iface=MIXER,name='Analog Playback Volume' ; type=INTEGER,access=rw---R--,values=2,min=0,max=228,step=0 : values=208,208 | dBscale-min=-102.00dB,step=0.50dB,mute=0
$ amixer cset name='Analog Playback Volume' '180','180' amixer: Control default element write error: Invalid argument
Looking at the code I'd say that patch 1/2 is correct however I have doubts about patch 2/2:
val_mask = mask << rshift; val2 = (ucontrol->value.integer.value[1] + min) & mask; + + if (mc->platform_max && val2 > mc->platform_max) + return -EINVAL; + if (val2 > max) + return -EINVAL; + val2 = val2 << rshift;
err = snd_soc_component_update_bits(component, reg2, val_mask,
The checks for max and platform_max are done on val2, but val2 is already the result of adding the minimum value ('min') and applying the mask. Shouldn't the checks be done on ucontrol->value.integer.value[1] instead?
Thanks,
Guillermo Rodriguez Garcia guille.rodriguez@gmail.com
On Thu, Nov 24, 2022 at 02:54:15PM +0100, Guillermo Rodriguez Garcia wrote:
El jue, 24 nov 2022 a las 12:13, Charles Keepax (ckeepax@opensource.cirrus.com) escribió:
On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
Hi all,
I am using a dev board with a Cirrus Logic cs24l51 codec.
This used to work fine prior to kernel version 5.x, however after 5.x it is not possible to set certain values for ALSA controls from userspace.
I believe this is related to the input validation that is mentioned in this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/, and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
For the cs24l51, all the controls that fail are using the SOC_DOUBLE_R_SX_TLV macro.
I have traced this to the checks in snd_soc_put_volsw_sx, specifically the (val > max - min) check:
Can you try these two patches:
https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@...
Thanks. In my tests, these patches seem to fix the problem for some values, but not for all of them:
$ amixer cset name='Analog Playback Volume' '208','208' numid=3,iface=MIXER,name='Analog Playback Volume' ; type=INTEGER,access=rw---R--,values=2,min=0,max=228,step=0 : values=208,208 | dBscale-min=-102.00dB,step=0.50dB,mute=0
$ amixer cset name='Analog Playback Volume' '180','180' amixer: Control default element write error: Invalid argument
Looking at the code I'd say that patch 1/2 is correct however I have doubts about patch 2/2:
val_mask = mask << rshift; val2 = (ucontrol->value.integer.value[1] + min) & mask;
if (mc->platform_max && val2 > mc->platform_max)return -EINVAL;if (val2 > max)return -EINVAL;val2 = val2 << rshift; err = snd_soc_component_update_bits(component, reg2, val_mask,The checks for max and platform_max are done on val2, but val2 is already the result of adding the minimum value ('min') and applying the mask. Shouldn't the checks be done on ucontrol->value.integer.value[1] instead?
Yeah they definitely should, I have resent the two patches including that fixup, lets see what Mark says. You are CCed on them so be great if you could give them a test too.
Thanks, Charles
On Fri, Nov 25, 2022 at 03:50:23PM +0000, Charles Keepax wrote:
Yeah they definitely should, I have resent the two patches including that fixup, lets see what Mark says. You are CCed on them so be great if you could give them a test too.
Please send the incremental fix (which you didn't mention in the cover leter...) as an incremental fix.
participants (3)
-
Charles Keepax -
Guillermo Rodriguez Garcia -
Mark Brown