Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Nov 2022 15:50:23 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com>
CC:     <linux-kernel@vger.kernel.org>,
        Tan =?utf-8?B?TmF5xLFy?= <tannayir@gmail.com>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Banajit Goswami <bgoswami@codeaurora.org>,
        Richard Fitzgerald <rf@opensource.cirrus.com>,
        Lucas Tanure <tanureal@opensource.cirrus.com>,
        David Rhodes <david.rhodes@cirrus.com>,
        James Schulman <james.schulman@cirrus.com>,
        <patches@opensource.cirrus.com>, <alsa-devel@alsa-project.org>,
        Mark Brown <broonie@kernel.org>
Subject: Re: SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver
Message-ID: <20221125155023.GF105268@ediswmail.ad.cirrus.com>
References: <CABDcavYdsk-O4x3oPX4i4+T5wsoZV26_kpEq6JvpD8A_cAGHxg@mail.gmail.com>
 <20221124111336.GE105268@ediswmail.ad.cirrus.com>
 <CABDcavbRryENG58LO7+gbJeKbBBPP1uG1Xc00yXUBNzspWpWoA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CABDcavbRryENG58LO7+gbJeKbBBPP1uG1Xc00yXUBNzspWpWoA@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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@kernel.org/
> 
> 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