linux-kernel - Re: [PATCH] [RFC] dmaengine: add fifo

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: <56aa6f45-cfd8-7f1e-9392-628ceb58093f@ti.com>
Date:   Fri, 7 Jun 2019 16:35:53 +0300
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Jon Hunter <jonathanh@...dia.com>,
        Sameer Pujar <spujar@...dia.com>, Vinod Koul <vkoul@...nel.org>
CC:     <dan.j.williams@...el.com>, <tiwai@...e.com>,
        <dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <sharadg@...dia.com>, <rlokhande@...dia.com>, <dramesh@...dia.com>,
        <mkumard@...dia.com>, linux-tegra <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member



On 07/06/2019 15.58, Jon Hunter wrote:
>> Imho if you can explain it without using 'HACK' in the sentences it
>> might be OK, but it does not feel right.
> 
> I don't perceive this as a hack. Although from looking at the
> description of the src/dst_maxburst these are burst size with regard to
> the device, so maybe it is a stretch.
> 
>> However since your ADMA and ADMIF is highly coupled and it does needs
>> special maxburst information (burst and allocated FIFO depth) I would
>> rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV:
>>
>> ADMA_BURST_SIZE(maxburst)	((maxburst) & 0xff)
>> ADMA_FIFO_SIZE(maxburst)	(((maxburst) >> 8) & 0xffffff)
>>
>> So lower 1 byte is the burst value you want from ADMA
>> the other 3 bytes are the allocated FIFO size for the given ADMAIF channel.
>>
>> Sure, you need a header for this to make sure there is no
>> misunderstanding between the two sides.
> 
> I don't like this because as I mentioned to Dmitry, the ADMA can perform
> memory-to-memory transfers where such encoding would not be applicable.

mem2mem does not really use dma_slave_config, it is for used by
is_slave_direction() == true type of transfers.

But true, if you use ADMA against anything other than ADMAIF then this
might be not right for non cyclic transfers.

> That does not align with the description in the
> include/linux/dmaengine.h either.

True.

>> Or pass the allocated FIFO size via maxburst and then the ADMA driver
>> will pick a 'good/safe' burst value for it.
>>
>> Or new member, but do you need two of them for src/dst? Probably
>> fifo_depth is better word for it, or allocated_fifo_depth.
> 
> Right, so looking at the struct dma_slave_config we have ...
> 
> u32 src_maxburst;
> u32 dst_maxburst;
> u32 src_port_window_size;
> u32 dst_port_window_size;
> 
> Now if we could make these window sizes a union like the following this
> could work ...
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8fcdee1c0cf9..851251263527 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -360,8 +360,14 @@ struct dma_slave_config {
>         enum dma_slave_buswidth dst_addr_width;
>         u32 src_maxburst;
>         u32 dst_maxburst;
> -       u32 src_port_window_size;
> -       u32 dst_port_window_size;
> +       union {
> +               u32 port_window_size;
> +               u32 port_fifo_size;
> +       } src;
> +       union {
> +               u32 port_window_size;
> +               u32 port_fifo_size;
> +       } dst;

What if in the future someone will have a setup where they would need both?

So not sure. Your problems are coming from a split DMA setup where the
two are highly coupled, but sits in a different place and need to be
configured as one device.

I think xilinx_dma is facing with similar issues and they have a custom
API to set parameters which does not fit or is peripheral specific:
include/linux/dma/xilinx_dma.h

Not sure if that is an acceptable solution.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki