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/[email protected]/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
[email protected]
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/[email protected]/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/[email protected]/
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
(<[email protected]>) 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/[email protected]/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/[email protected]/
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
[email protected]
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
> (<[email protected]>) 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/[email protected]/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/[email protected]/
>
> 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.