linux-kernel - Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai

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] [day] [month] [year] [list]

Message-ID: <5bb2ec07-a6ae-b49c-f10b-d588d95c7da0@maciej.szmigiero.name>
Date:   Tue, 21 Nov 2017 12:27:15 +0100
From:   "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To:     Nicolin Chen <nicoleotsuka@...il.com>
Cc:     Timur Tabi <timur@...i.org>, Xiubo Li <Xiubo.Lee@...il.com>,
        alsa-devel@...a-project.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Fabio Estevam <fabio.estevam@....com>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt()
 just once in AC'97 mode

On 21.11.2017 02:14, Nicolin Chen wrote:
> On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote:
>> On 21.11.2017 01:00, Nicolin Chen wrote:
>>> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
>> (..)
>>>> We need to make sure, however, that only proper channel slots are enabled
>>>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>>>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>>>> an AC'97 link is started but before the CODEC is configured by its driver.
>>>
>>> I don't really understand this part. Why do we need to *make sure*
>>> and set SACCDIS and SACCEN again since they're initialized already> 
>>> Could you please elaborate a bit more?
>>
>> SACCDIS and SACCEN aren't just normal registers.
>> They are a way to disable or enable bits in SACCST register - writing
>> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
>> writing a '1' bit at some position in SACCEN sets the relevant bit in
>> SACCST.
>>
>> But a bit in SACCST can also be set (and so an AC'97 channel slot
>> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
>> enough if this bit was set in SLOTREQ just once, bits in SACCST are
>> 'sticky').
> 
> This makes sense now. It could be mentioned a bit in the commit log
> as well.

Will do.

>> If an extra slot gets enabled that's a disaster for playback because some
>> of normal left or right channel samples are instead redirected to this
>> extra slot.
>>
>> Unfortunately, the VT1613 codec on UDOO board (which is the only user of
>> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
>> set extra bits in its SLOTREQ request - I've confirmed this on two boards
>> bought a few months apart directly from UDOO shop.
>>
>> A workaround implemented in fsl-asoc-card of setting an appropriate
>> CODEC register so that slots 3/4 (the normal stereo playback slots) are
>> used for S/PDIF seems to mostly fix this issue but since this codec is so
>> untrustworthy then:
> 
> If this move is also a workaround, probably it'd be better to have
> an fsl_ssi_ac97_xxx_war() function with some comments/descriptions,
> and include it in the config(). Since it doesn't hurt to set these
> registers again, I am personally fine with that. But it needs to be
> clear -- otherwise, people may try to remove it later with a change
> like: Removing redundant SACCEN/SACCDIS settings since they're done
> during probe().
> 

That's a good point, will do.

Maciej