Re: [PATCH v3 2/3] spi: atmel: Prevent false timeouts on long transfers

From: <Ryan.Wanner@microchip.com>
To: <miquel.raynal@bootlin.com>, <broonie@kernel.org>,
	<linux-spi@vger.kernel.org>
Cc: <samuel@sholland.org>, <wens@csie.org>,
	<jernej.skrabec@gmail.com>, <tudor.ambarus@linaro.org>,
	<thomas.petazzoni@bootlin.com>, <Claudiu.Beznea@microchip.com>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 2/3] spi: atmel: Prevent false timeouts on long transfers
Date: Thu, 22 Jun 2023 16:29:05 +0000	[thread overview]
Message-ID: <035e89d4-03da-e8b3-bbd3-2301548f142d@microchip.com> (raw)
In-Reply-To: <20230622090634.3411468-3-miquel.raynal@bootlin.com>

On 6/22/23 02:06, Miquel Raynal wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> A slow SPI bus clocks at ~20MHz, which means it would transfer about
> 2500 bytes per second with a single data line. Big transfers, like when
> dealing with flashes can easily reach a few MiB. The current DMA timeout
> is set to 1 second, which means any working transfer of about 4MiB will
> always be cancelled.
> 
> With the above derivations, on a slow bus, we can assume every byte will
> take at most 0.4ms. Said otherwise, we could add 4ms to the 1-second
> timeout delay every 10kiB. On a 4MiB transfer, it would bring the
> timeout delay up to 2.6s which still seems rather acceptable for a
> timeout.
> 
> The consequence of this is that long transfers might be allowed, which
> hence requires the need to interrupt the transfer if wanted by the
> user. We can hence switch to the _interruptible variant of
> wait_for_completion. This leads to a little bit more handling to also
> handle the interrupted case but looks really acceptable overall.
> 
> While at it, we drop the useless, noisy and redundant WARN_ON() call.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Ryan Wanner <ryan.wanner@microchip.com>
> ---
>  drivers/spi/spi-atmel.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 943548aab8af..d87be2890597 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -233,7 +233,8 @@
>   */
>  #define DMA_MIN_BYTES  16
> 
> -#define SPI_DMA_TIMEOUT                (msecs_to_jiffies(1000))
> +#define SPI_DMA_MIN_TIMEOUT    (msecs_to_jiffies(1000))
> +#define SPI_DMA_TIMEOUT_PER_10K        (msecs_to_jiffies(4))
> 
>  #define AUTOSUSPEND_TIMEOUT    2000
> 
> @@ -1279,7 +1280,8 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
>         struct atmel_spi_device *asd;
>         int                     timeout;
>         int                     ret;
> -       unsigned long           dma_timeout;
> +       unsigned int            dma_timeout;
> +       long                    ret_timeout;
> 
>         as = spi_controller_get_devdata(host);
> 
> @@ -1333,11 +1335,13 @@ static int atmel_spi_one_transfer(struct spi_controller *host,
>                         atmel_spi_unlock(as);
>                 }
> 
> -               dma_timeout = wait_for_completion_timeout(&as->xfer_completion,
> -                                                         SPI_DMA_TIMEOUT);
> -               if (WARN_ON(dma_timeout == 0)) {
> -                       dev_err(&spi->dev, "spi transfer timeout\n");
> -                       as->done_status = -EIO;
> +               dma_timeout = msecs_to_jiffies(spi_controller_xfer_timeout(host, xfer));
> +               ret_timeout = wait_for_completion_interruptible_timeout(&as->xfer_completion,
> +                                                                       dma_timeout);
> +               if (ret_timeout <= 0) {
> +                       dev_err(&spi->dev, "spi transfer %s\n",
> +                               !ret_timeout ? "timeout" : "canceled");
> +                       as->done_status = ret_timeout < 0 ? ret_timeout : -EIO;
>                 }
> 
>                 if (as->done_status)
> --
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel