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: <c194004a-2a22-5354-9042-3ce811236319@opensource.cirrus.com>
Date:   Tue, 10 Aug 2021 17:27:45 +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 10/08/2021 16:49, Mark Brown wrote:
> On Tue, Aug 10, 2021 at 04:37:51PM +0100, Richard Fitzgerald wrote:
>> cs42l42_pcm_hw_params() must only configure the PLL if this is the first
>> stream to become active, otherwise it will be overwriting the registers
>> while the PLL is running.
> 
> 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).

hw_params() deals with both playback and capture streams so it makes
sense to me that it should contain logic to ensure it isn't stomping on
the other stream it already set up, rather than have everything it calls
figure out whether it shouldn't have done that.