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

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@codethink.co.uk>
---
 drivers/spi/spi-s3c64xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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@codethink.co.uk>

Fixes: 230d42d422e7 ("spi: Add s3c64xx SPI Controller driver")
Cc: <stable@vger.kernel.org> # 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@kernel.org>

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
>

On Tue, 24 Sep 2024 14:40:08 +0100, 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.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: s3c64xx: fix timeout counters in flush_fifo
      commit: 68a16708d2503b6303d67abd43801e2ca40c208d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Hi Mark,

On Wed, Oct 02, 2024 at 01:38:57AM GMT, Mark Brown wrote:
> On Tue, 24 Sep 2024 14:40:08 +0100, 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.
> > 
> > 
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
> 
> Thanks!
> 
> [1/2] spi: s3c64xx: fix timeout counters in flush_fifo
>       commit: 68a16708d2503b6303d67abd43801e2ca40c208d

This still had some pending comments, besides I think it also
needed the Fixes tag.

Andi

On Wed, Oct 02, 2024 at 08:07:32AM +0200, Andi Shyti wrote:
> On Wed, Oct 02, 2024 at 01:38:57AM GMT, Mark Brown wrote:

> > [1/2] spi: s3c64xx: fix timeout counters in flush_fifo
> >       commit: 68a16708d2503b6303d67abd43801e2ca40c208d

> This still had some pending comments, besides I think it also

There were some suggestions for stylistic changes in the code but
nothing that can't be done incrementally (and which expand the scope of
the change).

> needed the Fixes tag.

I'd raise the fixes issue with Konstaintin, that's b4 not picking things
up.  I don't generally worry too much about stable tagging given how
eager the AUTOSEL stuff is.