spi: fsi: Fix spurious timeout
Subsystems (2)
Eddie James
maintainercover spi: fsi: Fix spurious timeout
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(-)
Signature · 1 line
2.27.0
Eddie James
maintainerPATCH 1/2 spi: fsi: Fix spurious timeout
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.
---
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(-)
drivers/spi/spi-fsi.c +6 -6
@@ -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);
Signature · 1 line
2.27.0
Mark Brown
maintainerQuoted 4 lines · Eddie James
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.
Eddie James
maintainerPATCH 2/2 spi: core: Display return code when failing to transfer message
In reply to Eddie JamesAll 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.
--- drivers/spi/spi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/spi/spi.c +2 -1
@@ -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; }
Signature · 1 line
2.27.0
Re: [PATCH 2/2] spi: core: Display return code when failing to transfer message
Hello,
Quoted 14 lines · 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 <[email protected]>--- drivers/spi/spi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/spi/spi.c +0
-0
drivers/spi/spi.c static void __spi_pump_messages(struct spi_controller *ctlr, bool in_kthread) · line 1658
Quoted 6 lines
drivers/spi/spi.c +2
-1@@ -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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Mark Brown
maintainerQuoted 8 lines · 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.[...]
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: ebf2a3521738520e12849b221fea24928b3f61ffAll 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