linux-kernel - Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <58687207-c5e7-2b4e-1ac6-221a85ceb224@sholland.org>
Date:   Mon, 5 Oct 2020 23:46:03 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Chen-Yu Tsai <wens@...e.org>, Maxime Ripard <maxime@...no.tech>
Cc:     Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Ondrej Jirman <megous@...ous.com>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while
 streams are open

On 10/5/20 8:15 AM, Chen-Yu Tsai wrote:
> On Mon, Oct 5, 2020 at 8:01 PM Maxime Ripard <maxime@...no.tech> wrote:
>>
>> On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
>>> The codec's clock input is shared among all AIFs, and shared with other
>>> audio-related hardware in the SoC, including I2S and SPDIF controllers.
>>> To ensure sample rates selected by userspace or by codec2codec DAI links
>>> are maintained, the clock rate must be protected while it is in use.
>>>
>>> Signed-off-by: Samuel Holland <samuel@...lland.org>
>>> ---
>>>  sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++---
>>>  1 file changed, 22 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
>>> index 501af64d43a0..86065bee7cd3 100644
>>> --- a/sound/soc/sunxi/sun8i-codec.c
>>> +++ b/sound/soc/sunxi/sun8i-codec.c
...
>>> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>>>                          (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
>>>
>>>       /* BCLK divider (SYSCLK/BCLK ratio) */
>>>       bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
>>>       regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>>>                          SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
>>>                          bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
>>>
>>> -     if (!aif->open_streams) {
>>> +     /* SYSCLK rate */
>>> +     if (aif->open_streams) {
>>> +             ret = clk_set_rate(scodec->clk_module, sysclk_rate);
>>> +             if (ret < 0)
>>> +                     return ret;
>>> +     } else {
>>> +             ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);
>>
>> It's not really clear to me why we wouldn't want to always protect the
>> clock rate here?
> 
> I believe the intention is to allow a window, i.e. when no audio
> blocks are running,
> when it is possible to switch between sample rate families?

Yes, this is an advantage now. It would not be the case if sun4i-i2s did
something similar. It has the same problem that multiple separate sound
cards compete for one PLL.

> ChenYu

Cheers,
Samuel