[PATCH] spi: a3700: fix hang caused by a3700_spi_transfer_one_fifo()

[PATCH] spi: a3700: fix hang caused by a3700_spi_transfer_one_fifo()

[Daisuke Yamane]

transfer_one() must call spi_finalize_current_transfer() before
returning to inform current transfer has finished. Otherwise spi driver
doesn't issue next transfer, and hang.
However a3700_spi_transfer_one_fifo() doesn't call it if waiting for
"wfifo empty" or "xfer ready" has timed out.
Thus, this patch corrects error handling of them.

Signed-off-by: Daisuke Yamane <yamane07ynct@gmail.com>
---
 drivers/spi/spi-armada-3700.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-armada-3700.c b/drivers/spi/spi-armada-3700.c
index fcde419e480c..1eb2c64386c3 100644
--- a/drivers/spi/spi-armada-3700.c
+++ b/drivers/spi/spi-armada-3700.c
@@ -698,13 +698,15 @@ static int a3700_spi_transfer_one_fifo(struct spi_master *master,
 			if (!a3700_spi_transfer_wait(spi,
 						     A3700_SPI_WFIFO_EMPTY)) {
 				dev_err(&spi->dev, "wait wfifo empty timed out\n");
-				return -ETIMEDOUT;
+				ret = -ETIMEDOUT;
+				goto error;
 			}
 		}
 
 		if (!a3700_spi_transfer_wait(spi, A3700_SPI_XFER_RDY)) {
 			dev_err(&spi->dev, "wait xfer ready timed out\n");
-			return -ETIMEDOUT;
+			ret = -ETIMEDOUT;
+			goto error;
 		}
 
 		val = spireg_read(a3700_spi, A3700_SPI_IF_CFG_REG);
-- 
2.17.1

Re: [PATCH] spi: a3700: fix hang caused by a3700_spi_transfer_one_fifo()

[Mark Brown]

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

On Tue, Jun 30, 2020 at 02:44:21AM +0900, Daisuke Yamane wrote:
> transfer_one() must call spi_finalize_current_transfer() before
> returning to inform current transfer has finished. Otherwise spi driver
> doesn't issue next transfer, and hang.

To be clear it can also return a positive value and then finalize later,
there's no need to finalize before returning (otherwise finalizing would
be a bit redundant) and if the driver doesn't return a positive value
there should be no need to finalize at all.

> However a3700_spi_transfer_one_fifo() doesn't call it if waiting for
> "wfifo empty" or "xfer ready" has timed out.
> Thus, this patch corrects error handling of them.

The core shouldn't be waiting at all if the driver returned an error, we
only wait if the return value was positive.  Looking at the code it's
not clear to me how we manage to end up waiting - it looks like the
driver passes back the error correctly and the core looks like it does
the right thing.  Have you seen hangs in operation?

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

Re: [PATCH] spi: a3700: fix hang caused by a3700_spi_transfer_one_fifo()

[Daisuke Yamane]

2020年6月30日(火) 20:11 Mark Brown <broonie@kernel.org>:
>
> On Tue, Jun 30, 2020 at 02:44:21AM +0900, Daisuke Yamane wrote:
> > transfer_one() must call spi_finalize_current_transfer() before
> > returning to inform current transfer has finished. Otherwise spi driver
> > doesn't issue next transfer, and hang.
>
> To be clear it can also return a positive value and then finalize later,
> there's no need to finalize before returning (otherwise finalizing would
> be a bit redundant) and if the driver doesn't return a positive value
> there should be no need to finalize at all.
>
> > However a3700_spi_transfer_one_fifo() doesn't call it if waiting for
> > "wfifo empty" or "xfer ready" has timed out.
> > Thus, this patch corrects error handling of them.
>
> The core shouldn't be waiting at all if the driver returned an error, we
> only wait if the return value was positive.  Looking at the code it's
> not clear to me how we manage to end up waiting - it looks like the
> driver passes back the error correctly and the core looks like it does
> the right thing.  Have you seen hangs in operation?
Yes, and I could avoid hanging by this patch. But I also understand that
your point is correct. Probably I'm misunderstanding the cause of the hang.
I will investigate a little more.