[PATCH 0/2] spi: fsi: Fix spurious timeout

[PATCH 0/2] spi: fsi: Fix spurious timeout

[Eddie James]

The driver may return a timeout error even if the status register
indicates that the transfer may proceed. Fix this by restructuring
the polling loop.
Also include a patch to display the error return code when failing
to transfer one message, which would have been very helpful in
debugging this issue.

Eddie James (2):
  spi: fsi: Fix spurious timeout
  spi: core: Display return code when failing to transfer message

 drivers/spi/spi-fsi.c | 12 ++++++------
 drivers/spi/spi.c     |  3 ++-
 2 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.27.0

[PATCH 1/2] spi: fsi: Fix spurious timeout

[Eddie James]

The driver may return a timeout error even if the status register
indicates that the transfer may proceed. Fix this by restructuring
the polling loop.

Fixes: 89b35e3f2851 ("spi: fsi: Implement a timeout for polling status")
Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 I have one concern still, that if the kernel is very busy, it may
 schedule other work for the entire timeout period between assigning
 "end" and checking if timed out in the do/while loop... Is it worth
 worrying about this case?

 drivers/spi/spi-fsi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-fsi.c b/drivers/spi/spi-fsi.c
index d403a7a3021d..72ab066ce552 100644
--- a/drivers/spi/spi-fsi.c
+++ b/drivers/spi/spi-fsi.c
@@ -319,12 +319,12 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx,
 
 			end = jiffies + msecs_to_jiffies(SPI_FSI_STATUS_TIMEOUT_MS);
 			do {
+				if (time_after(jiffies, end))
+					return -ETIMEDOUT;
+
 				rc = fsi_spi_status(ctx, &status, "TX");
 				if (rc)
 					return rc;
-
-				if (time_after(jiffies, end))
-					return -ETIMEDOUT;
 			} while (status & SPI_FSI_STATUS_TDR_FULL);
 
 			sent += nb;
@@ -337,12 +337,12 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx,
 		while (transfer->len > recv) {
 			end = jiffies + msecs_to_jiffies(SPI_FSI_STATUS_TIMEOUT_MS);
 			do {
+				if (time_after(jiffies, end))
+					return -ETIMEDOUT;
+
 				rc = fsi_spi_status(ctx, &status, "RX");
 				if (rc)
 					return rc;
-
-				if (time_after(jiffies, end))
-					return -ETIMEDOUT;
 			} while (!(status & SPI_FSI_STATUS_RDR_FULL));
 
 			rc = fsi_spi_read_reg(ctx, SPI_FSI_DATA_RX, &in);
-- 
2.27.0

[PATCH 2/2] spi: core: Display return code when failing to transfer message

[Eddie James]

All the other calls to the controller driver display the error
return code. The return code is helpful to understand what went
wrong, so include it when failing to transfer one message.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/spi/spi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 481edea77c62..ea09d1b42bf6 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1654,7 +1654,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
 	ret = ctlr->transfer_one_message(ctlr, msg);
 	if (ret) {
 		dev_err(&ctlr->dev,
-			"failed to transfer one message from queue\n");
+			"failed to transfer one message from queue: %d\n",
+			ret);
 		goto out;
 	}
 
-- 
2.27.0

Re: [PATCH 1/2] spi: fsi: Fix spurious timeout

[Mark Brown]

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

On Wed, May 25, 2022 at 11:58:51AM -0500, Eddie James wrote:

>  I have one concern still, that if the kernel is very busy, it may
>  schedule other work for the entire timeout period between assigning
>  "end" and checking if timed out in the do/while loop... Is it worth
>  worrying about this case?

I'm not sure we can entirely avoid there being a gap, but with it being
a busy loop you'd have to be very unlucky to hit that case.  If you come
up with a fix that'd be nice.

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

Re: [PATCH 0/2] spi: fsi: Fix spurious timeout

[Mark Brown]

On Wed, 25 May 2022 11:58:50 -0500, Eddie James wrote:
> The driver may return a timeout error even if the status register
> indicates that the transfer may proceed. Fix this by restructuring
> the polling loop.
> Also include a patch to display the error return code when failing
> to transfer one message, which would have been very helpful in
> debugging this issue.
> 
> [...]

Applied to

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

Thanks!

[1/2] spi: fsi: Fix spurious timeout
      commit: 61bf40ef51aa73f6216b33563271b6acf7ea8d70
[2/2] spi: core: Display return code when failing to transfer message
      commit: ebf2a3521738520e12849b221fea24928b3f61ff

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: [PATCH 2/2] spi: core: Display return code when failing to transfer message

[Uwe Kleine-König]

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

Hello,

On Wed, May 25, 2022 at 11:58:52AM -0500, Eddie James wrote:
> All the other calls to the controller driver display the error
> return code. The return code is helpful to understand what went
> wrong, so include it when failing to transfer one message.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/spi/spi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 481edea77c62..ea09d1b42bf6 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1654,7 +1654,8 @@ static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread)
>  	ret = ctlr->transfer_one_message(ctlr, msg);
>  	if (ret) {
>  		dev_err(&ctlr->dev,
> -			"failed to transfer one message from queue\n");
> +			"failed to transfer one message from queue: %d\n",
> +			ret);

(I know it's too late, just stumbled over this commit in mainline by
chance. So maybe just a suggestion for the next similar change...)

A tad nicer would be to use %pe instead of %d that results in

	mydev mybus: failed to transfer one message from queue: -EIO

instead of

	mydev mybus: failed to transfer one message from queue: -5

and so is more descriptive. (Note you need ERR_PTR(ret) for %pe.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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