linux-kernel - Re: [PATCH] spi: spi-imx: imx51: revert burst length calculation back to bits_per

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: Wed, 19 Jun 2024 11:40:04 +0200
From: Thorsten Scherer <T.Scherer@...elmann.de>
To: Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Mark Brown <broonie@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, Stefan Moring <stefan.moring@...hnolution.nl>, 
	Benjamin Bigler <benjamin@...ler.one>, Carlos Song <carlos.song@....com>, 
	Clark Wang <xiaoning.wang@....com>, Adam Butcher <adam@...samine.co.uk>, linux-spi@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Stefan Bigler <linux@...ler.io>, 
	Sebastian Reichel <sre@...nel.org>
Subject: Re: [PATCH] spi: spi-imx: imx51: revert burst length calculation
 back to bits_per_word

Hello,

On Tue, Jun 18, 2024 at 07:34:18PM +0200, Marc Kleine-Budde wrote:
> The patch 15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> on transfer length") increased the burst length calculation in
> mx51_ecspi_prepare_transfer() to be based on the transfer length.
> 
> This breaks HW CS + SPI_CS_WORD support which was added in
> 6e95b23a5b2d ("spi: imx: Implement support for CS_WORD") and transfers
> with bits-per-word != 8, 16, 32.
> 
> SPI_CS_WORD means the CS should be toggled after each word. The
> implementation in the imx-spi driver relies on the fact that the HW CS
> is toggled automatically by the controller after each burst length
> number of bits. Setting the burst length to the number of bits of the
> _whole_ message breaks this use case.
> 
> Further the patch 15a6af94a277 ("spi: Increase imx51 ecspi burst
> length based on transfer length") claims to optimize the transfers.
> But even without this patch, on modern spi-imx controllers with
> "dynamic_burst = true" (imx51, imx6 and newer), the transfers are
> already optimized, i.e. the burst length is dynamically adjusted in
> spi_imx_push() to avoid the pause between the SPI bursts. This has
> been confirmed by a scope measurement on an imx6d.
> 
> Subsequent Patches tried to fix these and other problems:
> 
> - 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
> - e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
> - c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
> - cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")
> 
> but the HW CS + SPI_CS_WORD use case is still broken.
> 
> To fix the problems revert the burst size calculation in
> mx51_ecspi_prepare_transfer() back to the original form, before
> 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on
> transfer length") was applied.
> 
> Cc: Stefan Moring <stefan.moring@...hnolution.nl>
> Cc: Stefan Bigler <linux@...ler.io>
> Cc: Clark Wang <xiaoning.wang@....com>
> Cc: Carlos Song <carlos.song@....com>
> Cc: Sebastian Reichel <sre@...nel.org>
> Cc: Thorsten Scherer <T.Scherer@...elmann.de>
> Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")
> Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
> Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
> Fixes: c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
> Fixes: cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")
> Link: https://lore.kernel.org/all/20240618-oxpecker-of-ideal-mastery-db59f8-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>

This patch fixes an issue on our i.MX8MP board (test based on v6.9 +
small-custom-patchstack + this-patch) where querying a ti,ads7953
(spi->mode |= SPI_CS_WORD) returned either 0 or 4095 .

Tested-by: Thorsten Scherer <t.scherer@...elmann.de>

Thank you, Marc.

> ---
>  drivers/spi/spi-imx.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index f4006c82f867..33164ebdb583 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -660,18 +660,8 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *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;
> -		}
> +		ctrl |= (spi_imx->bits_per_word - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	}
>  
>  	/* set clock speed */
> 
> ---
> base-commit: 6ba59ff4227927d3a8530fc2973b80e94b54d58f
> change-id: 20240618-spi-imx-fix-bustlength-9704b2fd9095
> 
> Best regards,
> -- 
> Marc Kleine-Budde <mkl@...gutronix.de>
> 
> 

Best regards
Thorsten