linux-kernel - Re: [PATCH 1/1] axi-i2s: set period size register

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:   Wed, 29 Aug 2018 09:15:56 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Lars-Peter Clausen <lars@...afoo.de>, alsa-devel@...a-project.org
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-kernel@...r.kernel.org,
        Michal Simek <michal.simek@...inx.com>
Subject: Re: [PATCH 1/1] axi-i2s: set period size register

Hi,

On 27/08/2018 18:51, Lars-Peter Clausen wrote:
> On 08/27/2018 06:22 PM, Luca Ceresoli wrote:
>> Hi,
>>
>> thanks for your feedback.
>>
>> [Adding Michal Simek (Xilinx maintainer) in Cc]
>>
>> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>>> The default value of the PERIOD_LEN register is 0 and results in
>>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>>
>>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>>> is active.
>>>>
>>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>>> known. This way TLAST is emitted once per period, and the DMA raises
>>>> interrupts correctly.
>>>
>>> The patch looks OK. But I'd prefer not to merge it if possible.
>>>
>>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>>> to be to unreliable and we've abandoned support for it. One of the more
>>> critical issues was that you can't abort a DMA transfer. That means when
>>> audio capture is stopped the DMA will halt, but not complete the current
>>> transfer. Then when the next audio capture start the DMA will continue with
>>> the previous transfer. The observed effect of this was that the system would
>>> just crash randomly (Presumably due to memory corruption).
>>
>> Strange. I have done many capture experiments with arecord and didn't
>> run into such bad issues. I only have a much less serious problem
>> (garbage or old samples in the first few buffers), but no crashes.
>>
>> Michal, are you aware of these problems?
>>
>>> Have you considered using the ADI AXI-DMAC? That should work just fine.
>>
>> Not until today, because AXI-DMA is working here.
>>
>> I'd like to better understand what's going on before changing an IP that
>> is working. Do you have additional details about your setup? How do you
>> run your tests?
> 
> This was 4-5 years ago. A AXI-DMA with both TX and RX connected to the
> AXI-I2S.
> 
> It might be that back then I didn't have buffer prealloc enabled, so a
> new DMA buffer gets allocated for each transfer. Then you end up with
> use after free and the DMA overwriting freed (and maybe reused) memory.
> 
> It was bad enough that it was a lot easier to add PL330 support to the
> I2S peripheral. Not using Xilinx DMA for anything anymore has saved me
> from a lot of headache.

I kind of understand you. However since AXI-DMA it is working for me
with this patch, chances are that bug has been fixed in the meanwhile.

-- 
Luca