| Message ID | 20250116225521.2688224-5-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: zynqmp-gqspi: Improve error recovery by resetting | expand |
Hello Sean, On 16/01/2025 at 17:55:20 -05, Sean Anderson <sean.anderson@linux.dev> wrote: > Some operations (such as reading several megabytes of data from a flash) > can take several seconds or more. Users may want to cancel such > operations. Allow them to do so now that we have a way to recover. I fully agree with the observation, I tried myself interrupting too long transfers with another spi controller: e0205d6203c2c ("spi: atmel: Prevent false timeouts on long transfers") But there were issues reported, so we limited the signals to SIGKILLs: 1ca2761a77349 ("spi: atmel: Do not cancel a transfer upon any signal") But jffs2 plays with sigkills, so for spi memories it does not work well, we had to revert: 890188d2d7e4a ("spi: atmel: Prevent spi transfers from being killed") Same thing was also observed on Zynq7000: 26cfc0dbe43aa ("spi: spi-zynq-qspi: use wait_for_completion_timeout to make zynq_qspi_exec_mem_op not interruptible") I would however hint to use a specific helper for deriving your timeouts if you play with spi memories, because it is interesting to adapt the values nevertheless: d8e4ebf870187 ("spi: Create a helper to derive adaptive timeouts") Thanks, Miquèl
On 1/17/25 03:41, Miquel Raynal wrote: > Hello Sean, > > On 16/01/2025 at 17:55:20 -05, Sean Anderson <sean.anderson@linux.dev> wrote: > >> Some operations (such as reading several megabytes of data from a flash) >> can take several seconds or more. Users may want to cancel such >> operations. Allow them to do so now that we have a way to recover. > > I fully agree with the observation, I tried myself interrupting too long > transfers with another spi controller: > > e0205d6203c2c ("spi: atmel: Prevent false timeouts on long transfers") > > But there were issues reported, so we limited the signals to SIGKILLs: > > 1ca2761a77349 ("spi: atmel: Do not cancel a transfer upon any signal") > > But jffs2 plays with sigkills, so for spi memories it does not work > well, we had to revert: > > 890188d2d7e4a ("spi: atmel: Prevent spi transfers from being killed") > > Same thing was also observed on Zynq7000: > > 26cfc0dbe43aa ("spi: spi-zynq-qspi: use wait_for_completion_timeout to make zynq_qspi_exec_mem_op not interruptible") > > I would however hint to use a specific helper for deriving your timeouts > if you play with spi memories, because it is interesting to adapt the > values nevertheless: > > d8e4ebf870187 ("spi: Create a helper to derive adaptive timeouts") Hm, ok. I wasn't sure whether this was allowed, but I saw a lot of interruptable users under drivers/spi. I guess I'll drop this patch for v2. --Sean
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index cf47466ec982..fa4bff73324e 100644 --- a/drivers/spi/spi-zynqmp-gqspi.c +++ b/drivers/spi/spi-zynqmp-gqspi.c @@ -1062,14 +1062,24 @@ static int zynqmp_qspi_wait(struct zynqmp_qspi *xqspi, unsigned long timeout) { int ret; - ret = wait_for_completion_timeout(&xqspi->data_completion, timeout); - if (ret) + /* Only allow interrupting if we can reset the device */ + if (xqspi->reset) + ret = wait_for_completion_interruptible_timeout(&xqspi->data_completion, + timeout); + else + ret = wait_for_completion_timeout(&xqspi->data_completion, + timeout); + if (ret > 0) return 0; - dev_err(xqspi->dev, "Operation timed out\n"); + + if (!ret) { + dev_err(xqspi->dev, "Operation timed out\n"); + ret = -ETIMEDOUT; + } /* Attempt to recover as best we can */ zynqmp_qspi_init_hw(xqspi); - return -ETIMEDOUT; + return ret; } /**
Some operations (such as reading several megabytes of data from a flash) can take several seconds or more. Users may want to cancel such operations. Allow them to do so now that we have a way to recover. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/spi/spi-zynqmp-gqspi.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)