linux-kernel - Re: [PATCH] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S

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]

Date:   Tue, 21 Sep 2021 08:52:23 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Péter Ujfalusi <peter.ujfalusi@...il.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Kirill Marinushkin <kmarinushkin@...dec.com>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S_2
 registers

On 2021-09-21 06:20, Péter Ujfalusi wrote:
> Hi Peter.
> 
> On 20/09/2021 22:37, Peter Rosin wrote:
> 
>> Nope, MODE1/2 are wired for I2C -> FMT is just the last I2C address
>> bit. So, nothing to do with I2S. And I can't see how that would explain
>> the same problem with the I2S_2 register?
> 
> true, but worth the question ;)

Right :-)

>>>> This fix is not 100% correct. The datasheet of at least the pcm5142
>>>> states that four bits (0xcc) in the I2S_1 register are "RSV"
>>>> ("Reserved. Do not access.") and no hint is given as to what theHi
>>>> initial values are supposed to be. So, specifying defaults for
>>>> these bits is wrong. But perhaps better than a broken driver?
>>>
>>> The default of 0x02 (AFMT = 00b - I2S, ALEN = 10b - 24bits) is correct
>>> for I2S_1 and the 0 is the default of I2S_2.
>>>
>>> The failure happens when updating the AFMT (bit4-5) and when regmap is
>>> doing a i2c read since the default is not specified.
>>>
>>> It would be interesting to see what it is reading... Out of interest:
>>> can you mar the I2S_1 and I2S_2 as volatile and read / print the value
>>> just before the afmt and alen update?
>>
>> My first attempt was to mark the register volatile, and then read and
>> compare if the update was needed at all. But marking volatile wasn't
>> enough.
> 
> If the value is not provided in the defaults then the first read is reading out to the chip anyways.

Yeah, but why is accessing I2S_1/2 returning -EBUSY when accessing e.g.
the PCM512x_MASTER_MODE register is not?

>> I also tried to set both a default and mark as volatile,
> 
> Volatile always skips the cache, default would be ignored.
> 
>> but it seems every read fails with -16 (EBUSY). I don't get why, to me
>> it almost feels like a regmap issue of some sort (probably the regmap
>> config is bad in some way), but I'm not fluent in regmap...
> 
> Or most likely the chip is not powered at pcm512x_set_fmt() time?

The chip is always powered, at least externally. Are you hinting that
relevant parts of the chip may be powered down internally when
pcm512x_set_fmt() executes and that this is the -EBUSY cause? Why does
that only happen to me in that case?

>> So, since I can't read, I can't get to the initial values of the four
>> reserved bits. So, I winged them as zero.
> 
> The value of the reserved bits are don't care.
> 
> Can you try the attached patch w/o without the defaults for i2s_1/2?
> Not even compile tested...

[added a couple of underscores]

I get this early during boot/probe
[    1.506291] pcm512x 0-004c: pcm512x_set_fmt: failed to read I2S_1: -16
[    1.512905] pcm512x 0-004c: pcm512x_set_fmt: failed to read PLL_EN: -16
[    1.519517] pcm512x 0-004c: Failed to set data format: -16
[    1.525045] pcm512x 0-004c: Failed to set data offset: -16

and then this later, triggered from userspace when an app opens
the device
[   14.620344] pcm512x 0-004c: pcm512x_hw_params: I2S_1: 0x2

So, reading *can* work.

Cheers,
Peter