linux-kernel - Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 5 Sep 2023 13:11:55 -0500
From:   "Rhodes, David" <drhodes@...nsource.cirrus.com>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
CC:     James Schulman <james.schulman@...rus.com>,
        David Rhodes <david.rhodes@...rus.com>,
        Richard Fitzgerald <rf@...nsource.cirrus.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "Liam Girdwood" <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        "Stefan Binding" <sbinding@...nsource.cirrus.com>,
        <alsa-devel@...a-project.org>, <patches@...nsource.cirrus.com>,
        <linux-kernel@...r.kernel.org>, <kernel@...labora.com>
Subject: Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

On 9/5/23 5:29 AM, Charles Keepax wrote:
> On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
>> Enabling the active/passive shared boosts involves writing the MDSYNC UP
>> register sequence, which cannot be performed before receiving the PLL
>> lock signal.
>>

> 
> Thanks for looking at this apologies this was missed in the
> initial review of the patch.
> 

Thanks Cristian, I agree with the intent of your patch.
We do not expect that clocks are always available before the DAPM PMU 
event and shared boost should still be configured if they are not.

>> +int cs35l41_mdsync_up(struct regmap *regmap)
>> +{
>> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
>> +		{CS35L41_PWR_CTRL3, 0},
>> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
>> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
>> +	};

I don't know why PWR_CTRL1 is included in the up sequence here.
This toggles GLOBAL_EN, which will cause the PLL to unlock and lock 
again. Doing this defeats the purpose of setting SYNC_EN in a separate 
operation, which is to only do so when the amp is powered on and has 
locked the PLL. GLOBAL_EN is set by the mdsync_down_seq, so all that is 
needed when the PLL is locked is to set SYNC_EN.

> 
> Is this now safe? By pulling this out into a worker thread, it is
> no longer under the DAPM lock, which makes me worry this can race
> with the other uses of PWR_CTRL3 which could theoretically change
> state between when you read the reg and when you write it.
> 

The Class-H DAPM widget also uses the PWR_CTRL3 register.

> 
> One question I might also have would be does a worker thread make
> more sense or would it be simpler to do the mdsync power up
> directly in response to the PLL lock IRQ?
> 

I  agree with implementing this in the PLL lock IRQ.
As I described above, all that would need to be done is to set SYNC_EN 
in the PLL Lock IRQ handler.

Thanks,
David