linux-kernel - Re: [PATCH 04/12] ASoC: cs42l42: Don't reconfigure the PLL while it is running

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: <bae1afcb-d983-c17c-d932-3afac16df501@opensource.cirrus.com>
Date:   Wed, 11 Aug 2021 13:21:24 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     Mark Brown <broonie@...nel.org>
CC:     <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 04/12] ASoC: cs42l42: Don't reconfigure the PLL while it
 is running

On 11/08/2021 12:56, Mark Brown wrote:
> On Tue, Aug 10, 2021 at 05:27:45PM +0100, Richard Fitzgerald wrote:
>> On 10/08/2021 16:49, Mark Brown wrote:
> 
>>> Shouldn't the PLL code be noticing problematic attempts to reconfigure
>>> the PLL while it's active rather than the individual callers?
> 
>> It's wrong for a hw_params() for one stream to try to configure the PLL
>> when the other stream has already called hw_params(), configured the PLL
>> and started it. E.g. if you started a PLAYBACK, configured and
>> started everything, then got another hw_params() for the CAPTURE.
> 
>> cs42l42_pll_config() could check whether it is already running and skip
>> configuration in that case, but that seems to me a rather opaque
>> implementation. In my opinion this doesn't really fall into the case of
>> ignoring-bad-stuff-to-be-helpful (like free() accepting a NULL).
> 
> This doesn't treat the situation as an error though, it just ignores it,
> and there's nothing to stop _pll_config() generating a warning if that
> makes sense.
> 

It isn't an error. hw_params() will be called for both substreams
(PLAYBACK and CAPTURE) and if one is already running we mustn't
reconfigure the things we already configured. The DAI is marked
symmetric so both substreams will always produce the same I2C BCLK.

As in:

hw_params() substream=PLAYBACK
     configure PLL
prepare() substream=PLAYBACK
     PLL is started
hw_params() substream=CAPTURE
     PLAYBACK substream already running so don't rewrite PLL registers

Some of the PLL configurations start with a "safe" configuration and
then switch over to the running configuration once the PLL is stable.
Calling pll_config() again would rewrite back to the startup config,
which would change the clock output.

It's ok if neither stream is started, since the PLL isn't started. This
is needed anyway because it is legal for hw_params() to be called
several times to change parameters without starting a stream.