linux-kernel - Re: [PATCH v2 SPI for-next 2/3] spi: mchp-pci1xxxx: DMA Read support for copying data into SPI Buf

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: <ZcIoWplpBp5A5yjr@finisterre.sirena.org.uk>
Date: Tue, 6 Feb 2024 12:38:50 +0000
From: Mark Brown <broonie@...nel.org>
To: Thangaraj Samynathan <thangaraj.s@...rochip.com>
Cc: linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
	UNGLinuxDriver@...rochip.com, kumaravel.thiagarajan@...rochip.com,
	tharunkumar.pasumarthi@...rochip.com
Subject: Re: [PATCH v2 SPI for-next 2/3] spi: mchp-pci1xxxx: DMA Read support
 for copying data into SPI Buf

On Tue, Feb 06, 2024 at 09:11:17AM +0530, Thangaraj Samynathan wrote:

> pci1xxxx_spi_transfer_with_dma is registered as transfer_one callback
> when DMA can be supported. This function adds DMA read operation which
> copies the data from host cpu buffer to SPI Tx Buffer.
> On DMA Read Completion interrupt, SPI transaction is initiated in isr.
> Helper functions pci1xxxx_spi_setup, pci1xxxx_spi_setup_dma_read and
> pci1xxxx_start_spi_xfer are added for starting spi transfer, setting up
> spi and dma read operation. In the existing implementation, codes are
> replaced with helper wherever applicable.

This description is kind of hard to follow and it seems like the
hardware is quite weirdly designed which doesn't help.  What you appear
to be saying here is that the DMA here is transmit only as far as SPI is
concerned (it's a bit confusing that you say it's read DMA but AFAICT
it's reading from CPU memory and writing to the SPI controller).  This
is a bit off in terms of the core DMA support, the core is assuming that
DMA will be bidirectional and mapping both the TX and RX buffers for DMA
which isn't great if the RX path is PIO.  If that's the case then you
might be better off open coding the mapping of the buffers.

> +static irqreturn_t pci1xxxx_spi_isr_dma(int irq, void *dev)
> +{

> +	if (regval & SPI_INTR) {
> +		rx_buf = p->rx_buf;
> +		memcpy_fromio(rx_buf + p->bytes_recvd, p->parent->reg_base +
> +				      SPI_MST_RSP_BUF_OFFSET(p->hw_inst), p->tx_sgl_len);
> +		p->bytes_recvd += p->tx_sgl_len;
> +
> +		p->tx_sgl = sg_next(p->tx_sgl);

If we're doing DMA why do we need to have a memcpy() here?  That would
tie in with the DMA being transmit only.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)