[PATCH 01/14] spi: unoptimize message when queuing it fails

[PATCH 01/14] spi: unoptimize message when queuing it fails

[Joseph CT Chan]

From: Joseph CT Chan <josephctchan@google.com>

spi_async() and spi_sync() optimize a message before they queue it.
When queuing then fails, the message is never finalized, but finalizing
is the only place where the core undoes its own optimization
(spi_finalize_current_message() calls spi_maybe_unoptimize_message()).
So the error paths leak the split transfers and the controller's
optimize state, and leave msg->optimized set. A caller that resends the
message then optimizes an already optimized message.

This happens when spi_async() returns -EBUSY because another device
holds spi_bus_lock() on the controller (for example mmc_spi), when the
controller's message queue is stopped (-ESHUTDOWN), when the controller
has no ->transfer() callback (-ENOTSUPP), and when __spi_sync() gets an
error from a controller's legacy ->transfer() callback.

Call spi_maybe_unoptimize_message() on these two error paths. It only
acts on messages the core optimized itself, so messages optimized by
the peripheral driver and controllers that defer optimization (such as
spi-mux) are not affected. A queued message is never unoptimized here.

Tested with KUnit (spi_core_optimize: spi_core_queue_error_test,
added later in this series) on ARCH=um; builds with W=1 on x86_64.
Not tested on hardware.

Assisted-by: Claude, Gemini
Fixes: 7b1d87af14d9 ("spi: add spi_optimize_message() APIs")
Fixes: c86a918b1bdb ("spi: don't unoptimize message in spi_async()")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph CT Chan <josephctchan@google.com>
---
 drivers/spi/spi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 40cda0037ea5..0d36460e2c87 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4906,6 +4906,10 @@ int spi_async(struct spi_device *spi, struct spi_message *message)
 
 	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
 
+	/* The message was not queued, so it will not be finalized. */
+	if (ret)
+		spi_maybe_unoptimize_message(message);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(spi_async);
@@ -5009,6 +5013,9 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
 	if (status == 0) {
 		wait_for_completion(&done);
 		status = message->status;
+	} else {
+		/* The message was not queued, so it will not be finalized. */
+		spi_maybe_unoptimize_message(message);
 	}
 	message->complete = NULL;
 	message->context = NULL;
-- 
2.56.0.317.g454f8302e1-goog

Re: [PATCH 01/14] spi: unoptimize message when queuing it fails

[David Lechner]

On 11/2/26 10:30 AM, Joseph CT Chan wrote:
> From: Joseph CT Chan <josephctchan@google.com>
> 
> spi_async() and spi_sync() optimize a message before they queue it.
> When queuing then fails, the message is never finalized, but finalizing
> is the only place where the core undoes its own optimization
> (spi_finalize_current_message() calls spi_maybe_unoptimize_message()).
> So the error paths leak the split transfers and the controller's
> optimize state, and leave msg->optimized set. A caller that resends the
> message then optimizes an already optimized message.
> 
> This happens when spi_async() returns -EBUSY because another device
> holds spi_bus_lock() on the controller (for example mmc_spi), when the
> controller's message queue is stopped (-ESHUTDOWN), when the controller
> has no ->transfer() callback (-ENOTSUPP), and when __spi_sync() gets an
> error from a controller's legacy ->transfer() callback.
> 
> Call spi_maybe_unoptimize_message() on these two error paths. It only
> acts on messages the core optimized itself, so messages optimized by
> the peripheral driver and controllers that defer optimization (such as
> spi-mux) are not affected. A queued message is never unoptimized here.
> 
> Tested with KUnit (spi_core_optimize: spi_core_queue_error_test,
> added later in this series) on ARCH=um; builds with W=1 on x86_64.
> Not tested on hardware.
> 
> Assisted-by: Claude, Gemini
> Fixes: 7b1d87af14d9 ("spi: add spi_optimize_message() APIs")
> Fixes: c86a918b1bdb ("spi: don't unoptimize message in spi_async()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Joseph CT Chan <josephctchan@google.com>
> ---
>  drivers/spi/spi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 40cda0037ea5..0d36460e2c87 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -4906,6 +4906,10 @@ int spi_async(struct spi_device *spi, struct spi_message *message)
>  
>  	spin_unlock_irqrestore(&ctlr->bus_lock_spinlock, flags);
>  
> +	/* The message was not queued, so it will not be finalized. */
> +	if (ret)
> +		spi_maybe_unoptimize_message(message);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(spi_async);
> @@ -5009,6 +5013,9 @@ static int __spi_sync(struct spi_device *spi, struct spi_message *message)
>  	if (status == 0) {
>  		wait_for_completion(&done);
>  		status = message->status;
> +	} else {
> +		/* The message was not queued, so it will not be finalized. */
> +		spi_maybe_unoptimize_message(message);
>  	}
>  	message->complete = NULL;
>  	message->context = NULL;

Reviewed-by: David Lechner <dlechner@baylibre.com>

[PATCH 02/14] spi: spi-mux: release mux when parent setup fails

[Joseph CT Chan]

From: Joseph CT Chan <josephctchan@google.com>

When spi_mux_select() switches to a different child, it locks the mux
with mux_control_select(), copies the child's settings into the spi-mux
device and calls spi_setup() on the parent controller. If spi_setup()
fails, spi_mux_select() returns the error without calling
mux_control_deselect(), so the mux stays locked. The next message on
any child of this mux then blocks forever in mux_control_select(), and
so does every other user of the same mux control.

spi_mux_select() also records the new chip select in current_cs before
spi_setup() runs. After a failure, the next message for the same child
skips the copy and spi_setup(), and is sent with a parent that was
never set up for it.

On spi_setup() failure, set current_cs to SPI_MUX_NO_CS, so the next
message sets up the parent again, and release the mux. Record the new
chip select only after spi_setup() succeeds.

Tested with KUnit (spi_mux: spi_mux_error_test, added later in this
series) on ARCH=um; that case also needs "spi: spi-mux: finalize
message when forwarding fails" to see the error status. Builds with
W=1 on x86_64. Not tested on hardware.

Assisted-by: Claude, Gemini
Fixes: 5a5fc308418a ("spi: spi-mux: Fix coverity issue, unchecked return value")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph CT Chan <josephctchan@google.com>
---
 drivers/spi/spi-mux.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
index 08fe1fa32dea..28d7718138a5 100644
--- a/drivers/spi/spi-mux.c
+++ b/drivers/spi/spi-mux.c
@@ -66,9 +66,16 @@ static int spi_mux_select(struct spi_device *spi)
 	priv->spi->mode = spi->mode;
 	priv->spi->bits_per_word = spi->bits_per_word;
 
+	ret = spi_setup(priv->spi);
+	if (ret) {
+		priv->current_cs = SPI_MUX_NO_CS;
+		mux_control_deselect(priv->mux);
+		return ret;
+	}
+
 	priv->current_cs = spi_get_chipselect(spi, 0);
 
-	return spi_setup(priv->spi);
+	return 0;
 }
 
 static int spi_mux_setup(struct spi_device *spi)
-- 
2.56.0.317.g454f8302e1-goog

[PATCH 03/14] spi: spi-mux: re-apply settings after child calls spi_setup()

[Joseph CT Chan]

From: Joseph CT Chan <josephctchan@google.com>

spi_mux_select() copies a child's max_speed_hz, mode and bits_per_word
into the spi-mux device and sets up the parent controller only when
the mux switches to a different child (current_cs differs). If the
child that is already selected changes its settings and calls
spi_setup(), spi_mux_setup() only calls spi_setup() on the spi-mux
device with its old settings, and current_cs still matches. The next
transfers of that child then run with the old speed and mode.

Set current_cs to SPI_MUX_NO_CS in spi_mux_setup(), so the next
transfer copies the child's settings and sets up the parent again.
spi_mux_setup() runs under the spi-mux controller's io_mutex, which the
message pump holds while a message is forwarded, so this does not race
with spi_mux_select(). The cost is one extra parent spi_setup() on the
next transfer.

Tested with KUnit (spi_mux: spi_mux_settings_lifecycle_test, added
later in this series) on ARCH=um; builds with W=1 on x86_64. Not tested
on hardware.

Assisted-by: Claude, Gemini
Fixes: e9e40543ad5b ("spi: Add generic SPI multiplexer")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph CT Chan <josephctchan@google.com>
---
 drivers/spi/spi-mux.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
index 28d7718138a5..a7ea6585b374 100644
--- a/drivers/spi/spi-mux.c
+++ b/drivers/spi/spi-mux.c
@@ -86,7 +86,13 @@ static int spi_mux_setup(struct spi_device *spi)
 	 * can be called multiple times, won't do a valid setup now but we will
 	 * change the settings when we do a transfer (necessary because we
 	 * can't predict from which device it will be anyway)
+	 *
+	 * The child may have changed its settings, so make the next transfer
+	 * copy them again even if this child is the current one. This runs
+	 * under our io_mutex, so no message is being forwarded right now.
 	 */
+	priv->current_cs = SPI_MUX_NO_CS;
+
 	return spi_setup(priv->spi);
 }
 
-- 
2.56.0.317.g454f8302e1-goog

[PATCH 04/14] spi: spi-mux: finalize message when forwarding fails

[Joseph CT Chan]

From: Joseph CT Chan <josephctchan@google.com>

spi_mux_transfer_one_message() forwards a child's message to the parent
controller. When spi_mux_select() or spi_async() on the parent fails,
it returns the error without finalizing the message. The SPI core then
logs "failed to transfer one message from queue" and does not finalize
it either, so the child's completion callback never runs and spi_sync()
on the child waits forever. After a spi_async() failure the mux also
stays locked, so the next message on any child of the mux blocks the
spi-mux message pump in mux_control_select(), and the message keeps the
spi-mux device and callback in place of the child's.

This is easy to hit: spi_async() on the parent returns -EBUSY whenever
another device on the parent bus holds spi_bus_lock(), which mmc_spi
does for every request. It also returns an error when the parent
rejects the message, for example a transfer with 16-bit words and an
odd length, which spi-mux does not validate because it defers message
optimization to the parent.

On select failure, set the message status and finalize it; there is
nothing to restore yet and, with patch 2 of this series ("spi: spi-mux:
release mux when parent setup fails"), spi_mux_select() has already
released the mux. On spi_async() failure, set the status and call
spi_mux_complete_cb(), which restores the child's pointers, finalizes
the message and releases the mux, in the same order as a successful
transfer. In both cases return 0: the message is already finalized
with m->status set, so the error reaches the caller through the
message, and the core does not wait for it. Returning the error would
only make the core log "failed to transfer one message from queue"
once per -EBUSY while another device holds the bus lock.

Behavior change: a device behind spi-mux now gets -EBUSY while another
device holds spi_bus_lock() on the parent, instead of hanging. A device
wired directly to the parent would instead wait in spi_sync(), because
spi_sync() takes the bus_lock_mutex that spi_bus_lock() holds
(drivers/spi/spi.c spi_sync() and spi_bus_lock()); spi-mux forwards
with spi_async(), which returns -EBUSY while the bus is locked. Making
spi-mux wait for the bus lock needs a core helper and is left for a
follow-up.

This depends on patch 1 of this series ("spi: unoptimize message when
queuing it fails"): spi_async() on the parent optimizes the message
for the parent, and without patch 1 a failed spi_async() leaves it in
that state when it is handed back to the child.

Tested with KUnit (spi_mux: spi_mux_error_test, added later in this
series) on ARCH=um; before this patch they time out with the message
never finalized. Builds with W=1 on x86_64. Not tested on hardware.

Assisted-by: Claude, Gemini
Fixes: e9e40543ad5b ("spi: Add generic SPI multiplexer")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph CT Chan <josephctchan@google.com>
---
 drivers/spi/spi-mux.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-mux.c b/drivers/spi/spi-mux.c
index a7ea6585b374..0e770137b5bb 100644
--- a/drivers/spi/spi-mux.c
+++ b/drivers/spi/spi-mux.c
@@ -117,8 +117,12 @@ static int spi_mux_transfer_one_message(struct spi_controller *ctlr,
 	int ret;
 
 	ret = spi_mux_select(spi);
-	if (ret)
-		return ret;
+	if (ret) {
+		/* spi_mux_select() released the mux on failure */
+		m->status = ret;
+		spi_finalize_current_message(ctlr);
+		return 0;
+	}
 
 	/*
 	 * Replace the complete callback, context and spi_device with our own
@@ -133,7 +137,23 @@ static int spi_mux_transfer_one_message(struct spi_controller *ctlr,
 	m->spi = priv->spi;
 
 	/* do the transfer */
-	return spi_async(priv->spi, m);
+	ret = spi_async(priv->spi, m);
+	if (ret) {
+		/*
+		 * The parent did not take the message, so it will not call
+		 * spi_mux_complete_cb(). Call it here to restore the child's
+		 * pointers, finalize the message and release the mux.
+		 */
+		m->status = ret;
+		spi_mux_complete_cb(priv);
+	}
+
+	/*
+	 * On error the message is already finalized with m->status set, so
+	 * the error reaches the caller; returning it would only make the core
+	 * log it once more.
+	 */
+	return 0;
 }
 
 static int spi_mux_probe(struct spi_device *spi)
-- 
2.56.0.317.g454f8302e1-goog

[PATCH 05/14] spi: spi-ti-qspi: finalize message on transfer error

[Joseph CT Chan]

From: Joseph CT Chan <josephctchan@google.com>

When ti_qspi_start_transfer_one() hits an error in qspi_transfer_msg(),
such as a transfer timeout (-ETIMEDOUT), it unlocks qspi->list_lock and
returns ret directly without writing QSPI_INVAL to QSPI_SPI_CMD_REG,
setting m->status, or calling spi_finalize_current_message(host).
Because the message is never finalized, spi_sync() or the spi_async()
callback waits forever on queued transfers. If spi_sync() took the
no-queue path, it returns -EINPROGRESS instead of the error, and the
message's optimize state is never released. The frame is also left
open, because the QSPI_INVAL write that ends it is skipped.

Set status to the transfer error and leave the transfer loop, so the
error goes through the existing tail: unlock list_lock, write
QSPI_INVAL, set m->status, and finalize the message. This fixes both
paths, and the caller now gets the error from qspi_transfer_msg()
instead of -EINVAL or -EINPROGRESS.

Tested at runtime with KUnit (spi_ti_qspi, ti_qspi_test, added later in
this series) by injecting a QSPI_SPI_STATUS_REG WC poll timeout
(-ETIMEDOUT) via logic_iomem emulated MMIO on ARCH=um.

Assisted-by: Claude, Gemini
Fixes: 505a14954e2d ("spi/qspi: Add qspi flash controller")
Cc: stable@vger.kernel.org
Signed-off-by: Joseph CT Chan <josephctchan@google.com>
---
 drivers/spi/spi-ti-qspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 091a8b8842f5..7360ee6bd122 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -708,8 +708,8 @@ static int ti_qspi_start_transfer_one(struct spi_controller *host,
 		ret = qspi_transfer_msg(qspi, t, transfer_len_words * wlen);
 		if (ret) {
 			dev_dbg(qspi->dev, "transfer message failed\n");
-			mutex_unlock(&qspi->list_lock);
-			return -EINVAL;
+			status = ret;
+			break;
 		}
 
 		m->actual_length += transfer_len_words * wlen;
-- 
2.56.0.317.g454f8302e1-goog