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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1cd66d6d-6a39-41eb-8646-0a6e6f473e8d@collabora.com>
Date:   Tue, 5 Sep 2023 23:05:28 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To:     "Rhodes, David" <drhodes@...nsource.cirrus.com>,
        Charles Keepax <ckeepax@...nsource.cirrus.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 21:11, Rhodes, David wrote:
> 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.

Unfortunately I had to rely on the existing implementation as I don't 
have access to the datasheet.

If I got it right, we should drop all write operations on PWR_CTRL1, 
and simply set the CS35L41_SYNC_EN_MASK bit in PWR_CTRL3.
 
>>
>> 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.

That's a good point, it should be fixed implicitly by replacing the 
read/write operations with a single regmap_update_bits() call, which 
is protected by regmap's internal lock.

> 
> 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.

As a matter of fact I initially considered doing this in the IRQ handler, 
but I also wanted to understand the reasoning behind current solution. 
Therefore I searched the ML archive for any relevant review comments, 
and I came across [1] which raises some concerns regarding the PLL lock 
signal, e.g. lack of documentation regarding trigger time & frequency.

If going with the IRQ handler approach is still the recommended approach,
I will handle it in v2.

Thanks for reviewing,
Cristian

[1] https://lore.kernel.org/all/20230207114855.GC36097@ediswmail.ad.cirrus.com/