linux-kernel - Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

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:   Mon, 19 Oct 2020 16:17:18 +0300
From:   Alexander Kochetkov <al.kochet@...il.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Mark Brown <broonie@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        linux-spi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: spi-sun6i: implement DMA-based transfer mode

Hi, Maxime! Thanks for reviewing patches!

>> 
>> +static int sun6i_spi_prepare_dma(struct sun6i_spi *sspi,
>> +				 struct spi_transfer *tfr)
>> +{
>> +	struct dma_async_tx_descriptor *rxdesc, *txdesc;
>> +	struct spi_master *master = sspi->master;
>> +
>> +	rxdesc = NULL;
>> +	if (tfr->rx_buf) {
>> +		struct dma_slave_config rxconf = {
>> +			.direction = DMA_DEV_TO_MEM,
>> +			.src_addr = sspi->dma_addr_rx,
>> +			.src_addr_width = 1,
>> +			.src_maxburst = 1,
>> +		};
> 
> That doesn't really look optimal, the controller seems to be able to
> read / write 32 bits at a time from its FIFO and we probably can
> increase the burst length too?

I had doubts if it would work. I didn’t know will DMA work for transfers with lengths not
aligned to 32 bits. For example, if we init DMA with src_addr_width = 1 and
.src_maxburst = 8 will DMA work for transfer with length 11? I expect that DMA fill FIFO
with 16 bytes and SPI transfer only 11 bytes and 5 bytes will leave in TX fifo.  I did the test
and there is no additional data left in the fifo buffer. Also at reception the is no memory
overwrites.

I made test with src_addr_width = 4, src_maxburst = 1 and transfer length 3. Looks
like in that case DMA doesn’t issue 4 bytes transfer.

For test with src_addr_width = 4, src_maxburst = 8 I had to adjust RF_RDY_TRIG_LEVEL_BITS
and TF_ERQ_TRIG_LEVEL_BITS of FIFO_CTL_REG to half of FIFO (32 bytes). With the config
DMA will transfer burst of half of FIFO length during transfer and remaining bytes at the end of
transfer.

>> 
>> @@ -343,7 +436,8 @@ static irqreturn_t sun6i_spi_handler(int irq, void *dev_id)
>> 	/* Transfer complete */
>> 	if (status & SUN6I_INT_CTL_TC) {
>> 		sun6i_spi_write(sspi, SUN6I_INT_STA_REG, SUN6I_INT_CTL_TC);
>> -		sun6i_spi_drain_fifo(sspi);
>> +		if (!sspi->use_dma)
>> +			sun6i_spi_drain_fifo(sspi);
> 
> Is it causing any issue? The FIFO will be drained only if there's
> something remaining in the FIFO, which shouldn't happen with DMA?
> 

No. It’s for make code patch explicit.
Remove the change?

Alexander.