| Message ID | 20200629174421.25784-1-yamane07ynct@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: a3700: fix hang caused by a3700_spi_transfer_one_fifo() | expand |
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?
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.
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);
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(-)