| Message ID | 20240924134009.116247-2-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 68a16708d2503b6303d67abd43801e2ca40c208d |
| Headers | show |
| Series | [1/2] spi: s3c64xx: fix timeout counters in flush_fifo | expand |
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.
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); 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");
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(-)