Re: [PATCH 1/2] spi: s3c64xx: fix timeout counters in flush_fifo

[Andi Shyti <andi.shyti@xxxxxxxxxx>]

Hi Ben,

On Tue, Sep 24, 2024 at 02:40:08PM GMT, Ben Dooks wrote:
> In the s3c64xx_flush_fifo() code, the loops counter is post-decremented
> in the do { } while(test && loops--) condition. This means the loops is
> left at the unsigned equivalent of -1 if the loop times out. The test
> after will never pass as if tests for loops == 0.
> 
> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>

Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
Cc: <stable@xxxxxxxxxxxxxxx> # v2.6.33+

> ---
>  drivers/spi/spi-s3c64xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 833c58c88e40..6ab416a33966 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -245,7 +245,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  	loops = msecs_to_loops(1);
>  	do {
>  		val = readl(regs + S3C64XX_SPI_STATUS);
> -	} while (TX_FIFO_LVL(val, sdd) && loops--);
> +	} while (TX_FIFO_LVL(val, sdd) && --loops);

Do you think a better fix would be to have a "long loops" as I
don't think we need such a big data type for basically 4 * HZ.

And this becomes (loops >= 0);

The same below.

BTW, it's good you sent these patches separately as this needs to
be ported to the stable kernels.

In any case,

Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx>

Thanks,
Andi

>  
>  	if (loops == 0)
>  		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
> @@ -258,7 +258,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  			readl(regs + S3C64XX_SPI_RX_DATA);
>  		else
>  			break;
> -	} while (loops--);
> +	} while (--loops);
>  
>  	if (loops == 0)
>  		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
> -- 
> 2.37.2.352.g3c44437643
>