linux-kernel - Re: [PATCH v2 2/2] spi: imx: support word delay

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: <1d3623ee-1f16-487c-98cb-ca2647e7239d@ijzerbout.nl>
Date: Fri, 15 Nov 2024 22:53:39 +0100
From: Kees Bakker <kees@...erbout.nl>
To: Jonas Rebmann <jre@...gutronix.de>, Mark Brown <broonie@...nel.org>,
 Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>,
 Fabio Estevam <festevam@...il.com>
Cc: kernel@...gutronix.de, linux-spi@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] spi: imx: support word delay

Op 13-11-2024 om 13:18 schreef Jonas Rebmann:
> Implement support for the word delay feature of i.MX51 (and onwards) via
> the ECSPI interface.
>
> Convert the requested delay to SPI cycles and account for an extra
> inter-word delay inserted by the controller in addition to the requested
> number of cycles, which was observed when testing this patch.
>
> Disable dynamic burst when word delay is set. As the configurable delay
> period in the controller is inserted after bursts, the burst length must
> equal the word length.
>
> Account for word delay in the transfer time estimation for
> polling_limit_us.
>
> Signed-off-by: Jonas Rebmann <jre@...gutronix.de>
> ---
>   drivers/spi/spi-imx.c | 95 +++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 85 insertions(+), 10 deletions(-)
>
> [...]
> +static unsigned int spi_imx_transfer_estimate_time_us(struct spi_transfer *transfer)
> +{
> +	u64 result;
> +
> +	result = DIV_U64_ROUND_CLOSEST((u64)USEC_PER_SEC * transfer->len * BITS_PER_BYTE,
> +				       transfer->effective_speed_hz);
> +	if (transfer->word_delay.value) {
> +		unsigned int word_delay_us;
> +		unsigned int words;
> +
> +		words = DIV_ROUND_UP(transfer->len * BITS_PER_BYTE, transfer->bits_per_word);
> +		word_delay_us = DIV_ROUND_CLOSEST(spi_delay_to_ns(&transfer->word_delay, transfer),
> +						  NSEC_PER_USEC);
> +		result += words * word_delay_us;
If the multiplication can overflow 32 bits to need to force a 64 bits 
multiply.
     result += (u64)words * word_delay_us;

But I'm wondering if `result` needs to be u64.
> +	}
> +
> +	return min(result, U32_MAX);
Do you really expect this much? You're clipping to U32_MAX.
U32_MAX microsecs is already more than an hour.
> +}
> [...]