linux-kernel - Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to use DMA

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: <20151208060511.GL11966@pengutronix.de>
Date:	Tue, 8 Dec 2015 07:05:12 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Anton Bondarenko <anton.bondarenko.sama@...il.com>
Cc:	broonie@...nel.org, b38343@...escale.com,
	linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	vladimir_zapolskiy@...tor.com, jiada_wang@...tor.com
Subject: Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to
 use DMA

On Tue, Dec 08, 2015 at 01:33:18AM +0100, Anton Bondarenko wrote:
> 
> 
> On 2015-12-07 10:42, Sascha Hauer wrote:
> >On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:
> >>RX DMA tail data handling doesn't work correctly in many cases with current
> >>implementation. It happens because SPI core was setup to generates both RX
> >>and RX TAIL events. And RX TAIL event does not work correctly.
> >>This can be easily verified by sending SPI transaction with size modulus
> >>WML(32 in our case) not equal 0.
> >>
> >>Also removing change introduced in f6ee9b582d2db652497b73c1f117591dfb6d3a90
> >>since this change only fix usecases with transfer size from 33 to 128 bytes
> >>and does not fix 129 bytes etc.
> >>
> >>This is output from transaction with len 138 bytes in loopback mode at 10Mhz:
> >>TX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
> >>TX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
> >>TX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
> >>TX0030: 5f 3c 35 b5 c4 8c dd 6c 11 32 3d e2 b4 b4 59 cf
> >>TX0040: ce 23 3d 27 df a7 f9 96 fc 1e e0 66 2c 0e 7b 8c
> >>TX0050: ca 30 42 8f bc 9f 7b ce d1 b8 b1 87 ec 8a d6 bb
> >>TX0060: 2e 15 63 0e 3c dc a4 3a 7a 06 20 a7 93 1b 34 dd
> >>TX0070: 4c f5 ec 88 96 68 d6 68 a0 09 6f 8e 93 47 c9 41
> >>TX0080: db ac cf 97 89 f3 51 05 79 71
> >>
> >>RX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
> >>RX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
> >>RX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
> >>RX0030: 5f 3c 35 00 00 b5 00 00 00 c4 00 00 8c 00 00 dd
> >>RX0040: 6c 11 32 3d e2 b4 b4 59 cf ce 23 3d 27 df a7 f9
> >>RX0050: 96 fc 1e e0 66 2c 0e 7b 8c ca 30 42 8f 1f 1f bc
> >>RX0060: 9f 7b ce d1 b8 b1 87 ec 8a d6 bb 2e 15 63 0e ed
> >>RX0070: ed 3c 58 58 58 dc 3d 3d a4 6a 6a 3a 52 52 7a 36
> >>RX0080: 06 20 a7 93 1b 34 dd 4c f5 ec
> >>
> >>Zeros at offset 33 and 34 caused by reading empty RX FIFO which not possible
> >>if DMA RX read was triggered by RX event. This mean DMA was triggered
> >>by RX TAIL event.
> >>
> >>Signed-off-by: Anton Bondarenko <anton.bondarenko.sama@...il.com>
> >>---
> >>  drivers/spi/spi-imx.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> >>index fa24637..7a68c62 100644
> >>--- a/drivers/spi/spi-imx.c
> >>+++ b/drivers/spi/spi-imx.c
> >>@@ -204,8 +204,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
> >>  {
> >>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
> >>
> >>-	if (spi_imx->dma_is_inited &&
> >>-	    transfer->len > spi_imx->wml * sizeof(u32))
> >>+	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml &&
> >>+	    (transfer->len % spi_imx->wml) == 0)
> >>  		return true;
> >
> >Must transfer->len really be bigger than spi_imx->wml? I would assume it
> >should be >= instead. And where is the * sizeof(u32) gone? If that's
> >unnecessary I heven't understood why.
> >
> >Sascha
> >
> >
> 
> Agree on '>='. Will be in V6.
> As for missing sizeof(u32).
> According to SoC specification it's possible to operate with 8- or 16 bits
> word directly by setting proper value in word_width field in CFG register. I
> do not know the what exactly is fixed by f6ee9b582d, but I could suspect
> such scenario:
> Some SPI client (spi-nor?) trying to perform SPI transaction with len mod 32
> not equal zero. It could be any value between 33 and 127, excluding 64. But
> since DMA RX tail functionality does not work correctly RX contains some
> garbage.

I just tested it. Back then I had 60byte transfers with the SPI NOR
driver. What I got is:

spi_master spi0: I/O Error in DMA RX
spi_master spi0: failed to transfer one message from queue

I can't follow anymore what led me to the assumption that this issue is
related to byte/wordsize mixup. The SPI NOR driver uses 8bit transfers
so one word is one byte, something the driver handles correctly.

I just tested your series successfully with this usecase. I can redo the
test with v6 once you send it and provide my tested-by tag.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/