two (possibly bug) updates for spi-s3c644

two (possibly bug) updates for spi-s3c644

[Ben Dooks]

When doing some investigation into customer issues, I ran across a
possble bug in the fifo flush code in spi-s3c64xx.c

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

[Ben Dooks]

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(-)

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");
-- 
2.37.2.352.g3c44437643

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

[Andi Shyti]

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
> 

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

[Mark Brown]

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

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

[Andi Shyti]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Ben Dooks]

The code that checks for loops in the s3c6xx_flush_fifo() checks
for loops being non-zero as a timeout, however the code /could/
finish with loops being zero and the fifo being flushed...

Also, it would be useful to know what is left in the fifo for this
error case, so update the checks to see what is left, and then also
print the number of entries.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6ab416a33966..7b244e1fd58a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
 		val = readl(regs + S3C64XX_SPI_STATUS);
 	} while (TX_FIFO_LVL(val, sdd) && --loops);
 
-	if (loops == 0)
-		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
+	if (TX_FIFO_LVL(val, sdd))
+		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd));
 
 	/* Flush RxFIFO*/
 	loops = msecs_to_loops(1);
@@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
 			break;
 	} while (--loops);
 
-	if (loops == 0)
-		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
+	if (RX_FIFO_LVL(val, sdd))
+		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd));
 
 	val = readl(regs + S3C64XX_SPI_CH_CFG);
 	val &= ~S3C64XX_SPI_CH_SW_RST;
-- 
2.37.2.352.g3c44437643

Re: [PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Andi Shyti]

On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote:
> The code that checks for loops in the s3c6xx_flush_fifo() checks
> for loops being non-zero as a timeout, however the code /could/
> finish with loops being zero and the fifo being flushed...

what is the possibility of this case?

> Also, it would be useful to know what is left in the fifo for this
> error case, so update the checks to see what is left, and then also
> print the number of entries.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/spi/spi-s3c64xx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 6ab416a33966..7b244e1fd58a 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  		val = readl(regs + S3C64XX_SPI_STATUS);
>  	} while (TX_FIFO_LVL(val, sdd) && --loops);
>  
> -	if (loops == 0)
> -		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
> +	if (TX_FIFO_LVL(val, sdd))
> +		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd));
>  
>  	/* Flush RxFIFO*/
>  	loops = msecs_to_loops(1);
> @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>  			break;
>  	} while (--loops);
>  
> -	if (loops == 0)
> -		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
> +	if (RX_FIFO_LVL(val, sdd))
> +		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd));

This change doesn't super excite me, but it's fine. Please add a
comment explaining the case when loops is '0' and the FIFO is
flushed.

With the comment given, you can have my r-b.

Thanks,
Andi

>  	val = readl(regs + S3C64XX_SPI_CH_CFG);
>  	val &= ~S3C64XX_SPI_CH_SW_RST;
> -- 
> 2.37.2.352.g3c44437643
> 

Re: [PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Ben Dooks]

On 30/09/2024 23:51, Andi Shyti wrote:
> On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote:
>> The code that checks for loops in the s3c6xx_flush_fifo() checks
>> for loops being non-zero as a timeout, however the code /could/
>> finish with loops being zero and the fifo being flushed...
> 
> what is the possibility of this case?

Not sure, currently we're trying to debug a customer's setup where
we're seeing some weird issues with SPI. This was found during a
look into the code awaiting hardware access.

The flush count was simply an inspection and it seemed like a good
idea to fix the initial issue and then if there was an issue to
print something more useful than a simple error message.

>> Also, it would be useful to know what is left in the fifo for this
>> error case, so update the checks to see what is left, and then also
>> print the number of entries.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/spi/spi-s3c64xx.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 6ab416a33966..7b244e1fd58a 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>>   		val = readl(regs + S3C64XX_SPI_STATUS);
>>   	} while (TX_FIFO_LVL(val, sdd) && --loops);
>>   
>> -	if (loops == 0)
>> -		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
>> +	if (TX_FIFO_LVL(val, sdd))
>> +		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd));
>>   
>>   	/* Flush RxFIFO*/
>>   	loops = msecs_to_loops(1);
>> @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
>>   			break;
>>   	} while (--loops);
>>   
>> -	if (loops == 0)
>> -		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
>> +	if (RX_FIFO_LVL(val, sdd))
>> +		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd));
> 
> This change doesn't super excite me, but it's fine. Please add a
> comment explaining the case when loops is '0' and the FIFO is
> flushed.
> 
> With the comment given, you can have my r-b.

Ok, will look at sending a second version later this week.

> 
> Thanks,
> Andi
> 
>>   	val = readl(regs + S3C64XX_SPI_CH_CFG);
>>   	val &= ~S3C64XX_SPI_CH_SW_RST;
>> -- 
>> 2.37.2.352.g3c44437643
>>
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH 2/2] spi: s3c64xx: update flush_fifo timeout code

[Andi Shyti]

Hi Ben,

On Tue, Oct 01, 2024 at 08:01:48AM GMT, Ben Dooks wrote:
> On 30/09/2024 23:51, Andi Shyti wrote:
> > On Tue, Sep 24, 2024 at 02:40:09PM GMT, Ben Dooks wrote:
> > > The code that checks for loops in the s3c6xx_flush_fifo() checks
> > > for loops being non-zero as a timeout, however the code /could/
> > > finish with loops being zero and the fifo being flushed...
> > 
> > what is the possibility of this case?
> 
> Not sure, currently we're trying to debug a customer's setup where
> we're seeing some weird issues with SPI. This was found during a
> look into the code awaiting hardware access.
> 
> The flush count was simply an inspection and it seemed like a good
> idea to fix the initial issue and then if there was an issue to
> print something more useful than a simple error message.
> 
> > > Also, it would be useful to know what is left in the fifo for this
> > > error case, so update the checks to see what is left, and then also
> > > print the number of entries.
> > > 
> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > > ---
> > >   drivers/spi/spi-s3c64xx.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> > > index 6ab416a33966..7b244e1fd58a 100644
> > > --- a/drivers/spi/spi-s3c64xx.c
> > > +++ b/drivers/spi/spi-s3c64xx.c
> > > @@ -247,8 +247,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> > >   		val = readl(regs + S3C64XX_SPI_STATUS);
> > >   	} while (TX_FIFO_LVL(val, sdd) && --loops);
> > > -	if (loops == 0)
> > > -		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
> > > +	if (TX_FIFO_LVL(val, sdd))
> > > +		dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO (%d left)\n", TX_FIFO_LVL(val, sdd));
> > >   	/* Flush RxFIFO*/
> > >   	loops = msecs_to_loops(1);
> > > @@ -260,8 +260,8 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> > >   			break;
> > >   	} while (--loops);
> > > -	if (loops == 0)
> > > -		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO\n");
> > > +	if (RX_FIFO_LVL(val, sdd))
> > > +		dev_warn(&sdd->pdev->dev, "Timed out flushing RX FIFO (%d left)\n", RX_FIFO_LVL(val, sdd));
> > 
> > This change doesn't super excite me, but it's fine. Please add a
> > comment explaining the case when loops is '0' and the FIFO is
> > flushed.
> > 
> > With the comment given, you can have my r-b.
> 
> Ok, will look at sending a second version later this week.

I actually think that these two patches can be squashed into a
single one, I don't see much reason for having it double.

Even better, I think the first patch is not needed at all with
this new one. Right?

Andi

> > 
> > Thanks,
> > Andi
> > 
> > >   	val = readl(regs + S3C64XX_SPI_CH_CFG);
> > >   	val &= ~S3C64XX_SPI_CH_SW_RST;
> > > -- 
> > > 2.37.2.352.g3c44437643
> > > 
> > 
> 
> 
> -- 
> Ben Dooks				http://www.codethink.co.uk/
> Senior Engineer				Codethink - Providing Genius
> 
> https://www.codethink.co.uk/privacy.html