linux-kernel - Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

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: Tue, 18 Jun 2024 17:59:36 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Stefan Moring <stefan.moring@...hnolution.nl>
Cc: Adam Butcher <adam@...samine.co.uk>, 
	Benjamin Bigler <benjamin@...ler.one>, Fabio Estevam <festevam@...il.com>, s.hauer@...gutronix.de, 
	linux-kernel@...r.kernel.org, Stefan Bigler <linux@...ler.io>, linux-spi@...r.kernel.org, 
	Thorsten Scherer <T.Scherer@...elmann.de>, broonie@...nel.org, Clark Wang <xiaoning.wang@....com>, 
	linux-imx@....com, kernel@...gutronix.de, Sebastian Reichel <sre@...nel.org>, 
	shawnguo@...nel.org, Carlos Song <carlos.song@....com>, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer
 length

On 18.06.2024 13:58:07, Marc Kleine-Budde wrote:
> Hi Stefan,
> 
> On 28.06.2023 15:20:39, Stefan Moring wrote:
> > In our application we send ~80kB at 10MHz. The total transfer time
> > went from ~80ms to 67ms, so that would be a reduction of 15%.
> > I tested it on an IMX8MM platform.
> 
> I'm currently debugging a problem with spi-imx, HW CS and SPI_CS_WORD on
> torvalds/master. The breakage goes back this patch.
> 
> I'm wondering what is your setup you have optimized with this patch?
> - Are you using HW or GPIO CS?
> - What are bits_per_word?
> - What's the length of the spi_transfer?
> 
> I'm asking because with a 8, 16 or 32 bit-per-word setting, the driver
> should use dynamic_burst on the imx8mm, which will overwrite the burst
> length in spi_imx_push().

This patch, even with all the fixes on top of it (torvalds/master)
breaks the HW CS + SPI_CS_WORD support which was added in 6e95b23a5b2d
("spi: imx: Implement support for CS_WORD").

I think this also breaks the support for bits-per-word != multiple of 8
bits. For these transfers the in-memory wordsizes are powers of two
bytes (e.g. 20 bit samples use 32 bits) [1] and via the burst length
configuration only the bits-per-word number of bits are shifted out.

[1] https://elixir.bootlin.com/linux/v6.9/source/include/linux/spi/spi.h#L144

| 	if (spi_imx->target_mode && is_imx53_ecspi(spi_imx))
| 		ctrl |= (spi_imx->target_burst * 8 - 1)
| 			<< MX51_ECSPI_CTRL_BL_OFFSET;
| 	else {
| 		if (spi_imx->usedma) {
| 			ctrl |= (spi_imx->bits_per_word - 1)
| 				<< MX51_ECSPI_CTRL_BL_OFFSET;
| 		} else {
| 			if (spi_imx->count >= MX51_ECSPI_CTRL_MAX_BURST)
| 				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
| 						<< MX51_ECSPI_CTRL_BL_OFFSET;
| 			else
| 				ctrl |= (spi_imx->count / DIV_ROUND_UP(spi_imx->bits_per_word,
| 						BITS_PER_BYTE) * spi_imx->bits_per_word - 1)
| 						<< MX51_ECSPI_CTRL_BL_OFFSET;
| 		}
| 	}

Consider a message with bits-per-word = 9 consisting of 4 words. It uses
4 word x 2 bytes/word = 8 bytes of memory. This boils down to a burst
length of 36 bits. Which means the spi-imx sends the first 36 bits from
the 64 bits of memory, this is clearly wrong.

This patch (15a6af94a277 ("spi: Increase imx51 ecspi burst length based
on transfer length")) is wrong and the 4 fixes on top of it don't
finally fix it. I can send a series of 5 reverts, or a manually revert
the burst length calculation to the original value in one patch.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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