diff mbox series

[v3,3/4] spi: spi-geni-qcom: Don't try to set CS if an xfer is pending

Message ID 20201217142842.v3.3.I07afdedcc49655c5d26880f8df9170aac5792378@changeid
State Accepted
Commit 3d7d916f9bc98ce88272b3e4405c7c685afbfcd6
Headers show
Series [v3,1/4] spi: spi-geni-qcom: Fix geni_spi_isr() NULL dereference in timeout case | expand

Commit Message

Doug Anderson Dec. 17, 2020, 10:29 p.m. UTC
If we get a timeout sending then this happens:

spi_transfer_one_message()
 ->transfer_one() AKA spi_geni_transfer_one()
  setup_fifo_xfer()
   mas->cur_xfer = non-NULL
 spi_transfer_wait() => TIMES OUT
 if (msg->status != -EINPROGRESS)
  goto out
 if (ret != 0 ...)
  spi_set_cs()
   ->set_cs AKA spi_geni_set_cs()
    # mas->cur_xfer is non-NULL

The above happens _before_ the SPI core calls ->handle_err() AKA
handle_fifo_timeout().

Unfortunately that won't work so well on geni.  If we got a timeout
transferring then it's likely that our interrupt handler is blocked,
but we need that same interrupt handler to run and the command channel
to be unblocked in order to adjust the chip select.  Trying to set the
chip select doesn't crash us but ends up confusing our state machine
and leads to messages like: Premature done. rx_rem = 32 bpw8

Let's just drop the chip select request in this case.  We can detect
the case because cur_xfer is non-NULL--it would have been set to NULL
in the interrupt handler if the previous transfer had finished.  Sure,
we might leave the chip select in the wrong state but it's likely it
was going to fail anyway and this avoids getting the driver even more
confused about what it's doing.

The SPI core in general assumes that setting chip select is a simple
operation that doesn't fail.  Yet another reason to just reconfigure
the chip select line as GPIOs.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- xfter => xfer in error message.
- More obvious in commit message how this happens

Changes in v2:
- ("spi: spi-geni-qcom: Don't try to set CS if an xfer is pending") new for v2.

 drivers/spi/spi-geni-qcom.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Stephen Boyd Dec. 18, 2020, 2:54 a.m. UTC | #1
Quoting Douglas Anderson (2020-12-17 14:29:13)
> If we get a timeout sending then this happens:

> 

> spi_transfer_one_message()

>  ->transfer_one() AKA spi_geni_transfer_one()

>   setup_fifo_xfer()

>    mas->cur_xfer = non-NULL

>  spi_transfer_wait() => TIMES OUT

>  if (msg->status != -EINPROGRESS)

>   goto out

>  if (ret != 0 ...)

>   spi_set_cs()

>    ->set_cs AKA spi_geni_set_cs()

>     # mas->cur_xfer is non-NULL

> 

> The above happens _before_ the SPI core calls ->handle_err() AKA

> handle_fifo_timeout().

> 

> Unfortunately that won't work so well on geni.  If we got a timeout

> transferring then it's likely that our interrupt handler is blocked,

> but we need that same interrupt handler to run and the command channel

> to be unblocked in order to adjust the chip select.  Trying to set the

> chip select doesn't crash us but ends up confusing our state machine

> and leads to messages like: Premature done. rx_rem = 32 bpw8

> 

> Let's just drop the chip select request in this case.  We can detect

> the case because cur_xfer is non-NULL--it would have been set to NULL

> in the interrupt handler if the previous transfer had finished.  Sure,

> we might leave the chip select in the wrong state but it's likely it

> was going to fail anyway and this avoids getting the driver even more

> confused about what it's doing.

> 

> The SPI core in general assumes that setting chip select is a simple

> operation that doesn't fail.  Yet another reason to just reconfigure

> the chip select line as GPIOs.

> 

> Signed-off-by: Douglas Anderson <dianders@chromium.org>

> ---


Reviewed-by: Stephen Boyd <swboyd@chromium.org>
diff mbox series

Patch

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index cf3db40ae5ba..b3ba092db489 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -207,9 +207,14 @@  static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
 		goto exit;
 	}
 
-	mas->cs_flag = set_flag;
-
 	spin_lock_irq(&mas->lock);
+	if (mas->cur_xfer) {
+		dev_err(mas->dev, "Can't set CS when prev xfer running\n");
+		spin_unlock_irq(&mas->lock);
+		goto exit;
+	}
+
+	mas->cs_flag = set_flag;
 	reinit_completion(&mas->cs_done);
 	if (set_flag)
 		geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0);