spi: spi-mux: error handling and delay fixes

From: Joseph CT Chan <[email protected]>

Hi Mark,

This series fixes four problems in spi-mux:
- If spi_setup() on the parent fails, the mux stays locked and every later message on that mux blocks.
- When the selected child changes its settings and calls spi_setup(), the new settings never reach the parent.
- If selecting the mux or spi_async() on the parent fails, the message is never finalized and spi_sync() on the child waits forever. -EBUSY while mmc_spi holds the parent bus lock is one easy way to hit this.
- The child's CS setup, hold, inactive and word delays are not passed to the parent.
Patch 5 adds KUnit tests for all four. Tested with KUnit on ARCH=um:
the 9 spi_mux cases pass with patches 1-4 and fail without them. Not tested on hardware.

Changes in v2:
- Split out of the v1 14-patch series, as David suggested.
- Patches 3 and 4: updated the patch references in the commit messages.
- Patch 5: designated initializers, trimmed the comments.
- Moved the AI assistance note from an Assisted-by tag into the commit message.

v1: https://lore.kernel.org/all/[email protected]/

This series depends on the SPI core series v2: without "spi: unoptimize message when queuing it fails", a failed spi_async() on the parent leaves the message optimized. It is also based on the spi-ti-qspi series v2, because both add lines at the end of the same KUnit Makefile and .kunitconfig. Patches 1-4 apply on their own.

SPI core series v2: https://lore.kernel.org/linux-spi/[email protected]/
spi-ti-qspi series v2: https://lore.kernel.org/linux-spi/[email protected]/

Walkthrough: https://gist.github.com/joectchan/9a14b5e6facb75058ebc5a6ff1757131

Please consider this patch series for the next merge window.

Thanks,
Joseph

Joseph CT Chan (5):
spi: spi-mux: release mux when parent setup fails
spi: spi-mux: re-apply settings after child calls spi_setup()
spi: spi-mux: finalize message when forwarding fails
spi: spi-mux: pass child CS and word delays to the parent
spi: spi-mux: add KUnit tests for forwarding errors and settings

 drivers/spi/Kconfig                  |  14 +
 drivers/spi/Makefile                 |   1 +
 drivers/spi/spi-mux.c                | 124 ++++++-
 drivers/spi/tests/.kunitconfig       |   4 +
 drivers/spi/tests/Makefile           |   2 +
 drivers/spi/tests/kunit_spi_mux.dtso |  42 +++
 drivers/spi/tests/spi-mux-kunit.c    | 506 +++++++++++++++++++++++++++
 7 files changed, 689 insertions(+), 4 deletions(-)
 create mode 100644 drivers/spi/tests/kunit_spi_mux.dtso
 create mode 100644 drivers/spi/tests/spi-mux-kunit.c

base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
prerequisite-patch-id: 6999a3baa2d32de8fa42f2626a845fe3cb63b0b2
prerequisite-patch-id: 686a62b9223209bf1e83740a0874e20d7607f69a
prerequisite-patch-id: 616b18b757805a6f1520177e1a3e572ac95be582
prerequisite-patch-id: 0d46943b5bec4900701286d93b8b3e975701fb3d

2.56.0.385.gac671c819a-goog

From: Joseph CT Chan <[email protected]>

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.

Developed with help from the Claude and Gemini AI assistants.

---
 drivers/spi/spi-mux.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

index 08fe1fa32dea..28d7718138a5 100644
@@ -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.385.gac671c819a-goog

From: Joseph CT Chan <[email protected]>

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.

Developed with help from the Claude and Gemini AI assistants.

---
 drivers/spi/spi-mux.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 28d7718138a5..a7ea6585b374 100644
@@ -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.385.gac671c819a-goog

From: Joseph CT Chan <[email protected]>

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 1 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 "spi: unoptimize message when queuing it fails", which is in a separate SPI core series: spi_async() on the parent optimizes the message for the parent, and without that patch 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.

Developed with help from the Claude and Gemini AI assistants.

---
v2: Updated the patch references in the commit message after the split.

 drivers/spi/spi-mux.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

index a7ea6585b374..0e770137b5bb 100644
@@ -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.385.gac671c819a-goog

From: Joseph CT Chan <[email protected]>

spi_mux_select() copies only max_speed_hz, mode and bits_per_word from the child to the spi-mux device on the parent controller. The child's cs_setup, cs_hold, cs_inactive and word_delay are never passed on, so the parent controller uses the spi-mux device's own delays (usually zero) for every child. A child that needs a CS setup time or a delay between words, for example from spi-cs-setup-delay-ns in its device tree node, does not get it when it sits behind a mux.

Save the spi-mux device's own delays at probe time, and when switching to a child, merge them with the child's in a new helper, spi_mux_merge_cs_delay(): use the longer CS delay of the two, so that the needs of the mux and of the child are both met; on a tie the spi-mux delay is kept. A delay given in SCK cycles cannot be converted to time without a transfer, so it cannot be compared: a child delay in SCK cycles is used when the spi-mux delay is zero, and otherwise the spi-mux delay is kept and a warning is printed once. If only the spi-mux delay is in SCK cycles, it is kept without a warning; device tree only gives delays in ns or us, so this case does not come from DT. The child's word_delay is used when it is set, else the spi-mux device's own. The delays are written before spi_setup() on the parent, so they reach both set_cs_timing() and the software CS delays in spi_set_cs().

spi_mux_select() overwrites the delays of the spi-mux device, so put its own delays back when the driver unbinds. Otherwise a later probe would save the last child's merged delays as the spi-mux device's own.

Unlike a device directly on a controller, whose CS delays are read on every message, the delays are now copied when the mux switches to the child or after the child calls spi_setup(). A child that changes its delays must call spi_setup() for the change to take effect.

This depends on patch 2 of this series ("spi: spi-mux: re-apply settings after child calls spi_setup()"), which makes the copy happen again after the child calls spi_setup().

Tested with KUnit (spi_mux: spi_mux_cs_delay_test and spi_mux_settings_lifecycle_test, added later in this series) on ARCH=um; they fail without this patch. Builds with W=1 on x86_64. Not tested on hardware.

Developed with help from the Claude and Gemini AI assistants.

---
v2: Updated the patch reference in the commit message after the split.

 drivers/spi/spi-mux.c | 83 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

index 0e770137b5bb..45d8920727e0 100644
@@ -34,6 +34,10 @@
  * @child_msg_dev:	Used to store the spi_device pointer to the child
  * @mux:		mux_control structure used to provide chip selects for
  *			downstream spi devices
+ * @mux_cs_setup:	CS setup delay of the spi-mux device itself
+ * @mux_cs_hold:	CS hold delay of the spi-mux device itself
+ * @mux_cs_inactive:	CS inactive delay of the spi-mux device itself
+ * @mux_word_delay:	word delay of the spi-mux device itself
  */
 struct spi_mux_priv {
 	struct spi_device	*spi;

@@ -43,8 +47,55 @@ struct spi_mux_priv {
 	void			*child_msg_context;
 	struct spi_device	*child_msg_dev;
 	struct mux_control	*mux;
+
+	struct spi_delay	mux_cs_setup;
+	struct spi_delay	mux_cs_hold;
+	struct spi_delay	mux_cs_inactive;
+	struct spi_delay	mux_word_delay;
 };
 
+/*
+ * Pick the CS delay for the parent from the spi-mux device's own delay and
+ * the child's: the longer of the two, so that both are honored; on a tie the
+ * spi-mux delay is kept. A delay in SCK cycles cannot be converted to time
+ * without a transfer, so it cannot be compared:
+ * - a child delay in SCK cycles is used when the spi-mux delay is zero;
+ *   otherwise the spi-mux delay is kept and a warning is printed once;
+ * - if only the spi-mux delay is in SCK cycles, it is kept. Device tree
+ *   only gives delays in ns or us, so this case does not come from DT.
+ */
+static void spi_mux_merge_cs_delay(struct spi_mux_priv *priv,
+				   struct spi_delay *dst,
+				   struct spi_delay *mux,
+				   struct spi_delay *child)
+{
+	int mux_ns, child_ns;
+
+	if (!child->value) {
+		*dst = *mux;
+		return;
+	}
+
+	mux_ns = spi_delay_to_ns(mux, NULL);
+	child_ns = spi_delay_to_ns(child, NULL);
+
+	if (mux_ns >= 0 && child_ns >= 0) {
+		*dst = child_ns > mux_ns ? *child : *mux;
+		return;
+	}
+
+	if (child_ns < 0) {
+		if (!mux->value) {
+			*dst = *child;
+			return;
+		}
+		dev_warn_once(&priv->spi->dev,
+			      "child CS delay in SCK cycles ignored, using the mux delay\n");
+	}
+
+	*dst = *mux;
+}
+
 /* should not get called when the parent controller is doing a transfer */
 static int spi_mux_select(struct spi_device *spi)
 {

@@ -65,6 +116,14 @@ static int spi_mux_select(struct spi_device *spi)
 	priv->spi->max_speed_hz = spi->max_speed_hz;
 	priv->spi->mode = spi->mode;
 	priv->spi->bits_per_word = spi->bits_per_word;
+	spi_mux_merge_cs_delay(priv, &priv->spi->cs_setup, &priv->mux_cs_setup,
+			       &spi->cs_setup);
+	spi_mux_merge_cs_delay(priv, &priv->spi->cs_hold, &priv->mux_cs_hold,
+			       &spi->cs_hold);
+	spi_mux_merge_cs_delay(priv, &priv->spi->cs_inactive,
+			       &priv->mux_cs_inactive, &spi->cs_inactive);
+	priv->spi->word_delay = spi->word_delay.value ? spi->word_delay :
+							 priv->mux_word_delay;
 
 	ret = spi_setup(priv->spi);
 	if (ret) {

@@ -156,10 +215,26 @@ static int spi_mux_transfer_one_message(struct spi_controller *ctlr,
 	return 0;
 }
 
+/*
+ * spi_mux_select() overwrites the delays of the spi-mux device with the
+ * merged ones. Put its own delays back when the driver unbinds, so that a
+ * later probe does not take the last child's delays as its own.
+ */
+static void spi_mux_restore_delays(void *data)
+{
+	struct spi_mux_priv *priv = data;
+
+	priv->spi->cs_setup = priv->mux_cs_setup;
+	priv->spi->cs_hold = priv->mux_cs_hold;
+	priv->spi->cs_inactive = priv->mux_cs_inactive;
+	priv->spi->word_delay = priv->mux_word_delay;
+}
+
 static int spi_mux_probe(struct spi_device *spi)
 {
 	struct spi_controller *ctlr;
 	struct spi_mux_priv *priv;
+	int ret;
 
 	ctlr = devm_spi_alloc_host(&spi->dev, sizeof(*priv));
 	if (!ctlr)

@@ -168,6 +243,14 @@ static int spi_mux_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, ctlr);
 	priv = spi_controller_get_devdata(ctlr);
 	priv->spi = spi;
+	priv->mux_cs_setup = spi->cs_setup;
+	priv->mux_cs_hold = spi->cs_hold;
+	priv->mux_cs_inactive = spi->cs_inactive;
+	priv->mux_word_delay = spi->word_delay;
+
+	ret = devm_add_action_or_reset(&spi->dev, spi_mux_restore_delays, priv);
+	if (ret)
+		return ret;
 
 	/*
 	 * Increase lockdep class as these lock are taken while the parent bus

2.56.0.385.gac671c819a-goog

From: Joseph CT Chan <[email protected]>

Add a KUnit test suite (spi_mux) with a Device Tree overlay that instantiates a fake mux controller, a fake parent SPI controller, the real spi-mux driver, and two child SPI devices.

The suite covers three areas:
- spi_mux_error_test: forwarding errors (locked parent bus, invalid transfer length, mux select failure, and parent spi_setup() failure), verifying that the message is finalized, the mux is released, the caller's message fields and optimize state are restored, and subsequent transfers succeed.
- spi_mux_cs_delay_test: CS setup, hold, and inactive delay merging across ns, us, and SCK units, including zero-value SCK delays.
- spi_mux_settings_lifecycle_test: re-applying child mode, speed, bits_per_word, and delay changes after spi_setup(), switching between children with different delays, and restoring the spi-mux device's own delays on driver unbind.

Developed with help from the Claude and Gemini AI assistants.

---
v2: Designated initializers for the test tables, trimmed the comments.

 drivers/spi/Kconfig                  |  14 +
 drivers/spi/Makefile                 |   1 +
 drivers/spi/tests/.kunitconfig       |   4 +
 drivers/spi/tests/Makefile           |   2 +
 drivers/spi/tests/kunit_spi_mux.dtso |  42 +++
 drivers/spi/tests/spi-mux-kunit.c    | 506 +++++++++++++++++++++++++++
 6 files changed, 569 insertions(+)
 create mode 100644 drivers/spi/tests/kunit_spi_mux.dtso
 create mode 100644 drivers/spi/tests/spi-mux-kunit.c

index feb9265b7d0f..9a36bf9cf94f 100644
@@ -1386,6 +1386,20 @@ config SPI_MUX
 	  to be chip selects on this controller. It is still necessary to
 	  select one or more specific mux-controller drivers.
 
+config SPI_MUX_KUNIT_TEST
+	tristate "KUnit tests for spi-mux" if !KUNIT_ALL_TESTS
+	depends on KUNIT && SPI_MUX && OF_OVERLAY
+	select DTC
+	default KUNIT_ALL_TESTS
+	help
+	  Build KUnit tests for the SPI multiplexer driver. A device tree
+	  overlay adds a fake parent controller, a test mux chip and an
+	  spi-mux device with two children. The tests check that a message
+	  which cannot be forwarded is completed with an error and releases
+	  the mux, and that child settings and delays reach the parent.
+
+	  If unsure say N.
+
 #
 # There are lots of SPI device types, with sensors and memory
 # being probably the most widely used ones.

index 24497d3936f6..9826626995a9 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 obj-$(CONFIG_SPI_DMA_KUNIT_TEST)	+= tests/
 obj-$(CONFIG_SPI_CORE_KUNIT_TEST)	+= tests/
 obj-$(CONFIG_SPI_TI_QSPI_KUNIT_TEST)	+= tests/
+obj-$(CONFIG_SPI_MUX_KUNIT_TEST)	+= tests/
 
 # SPI master controller drivers (bus)
 obj-$(CONFIG_SPI_AIROHA_SNFI)		+= spi-airoha-snfi.o

index 354bafc42c2b..1631a16ce6ec 100644
@@ -6,3 +6,7 @@ CONFIG_SPI_CORE_KUNIT_TEST=y
 CONFIG_COMPILE_TEST=y
 CONFIG_SPI_TI_QSPI=y
 CONFIG_SPI_TI_QSPI_KUNIT_TEST=y
+CONFIG_OF=y
+CONFIG_OF_OVERLAY=y
+CONFIG_SPI_MUX=y
+CONFIG_SPI_MUX_KUNIT_TEST=y

index a5a8fc9435d4..0ec04ecd4d55 100644
@@ -3,3 +3,5 @@
 obj-$(CONFIG_SPI_DMA_KUNIT_TEST) += spi-dma-kunit.o
 obj-$(CONFIG_SPI_CORE_KUNIT_TEST) += spi-core-kunit.o
 obj-$(CONFIG_SPI_TI_QSPI_KUNIT_TEST) += spi-ti-qspi-kunit.o
+obj-$(CONFIG_SPI_MUX_KUNIT_TEST) += spi-mux-kunit-test.o
+spi-mux-kunit-test-y := spi-mux-kunit.o kunit_spi_mux.dtbo.o

2.56.0.385.gac671c819a-goog

This also does not apply against current code, please check and resend. It is not sustainable for me to manage some stack of random interdependencies between multiple in flight serieses, please do not spam out stacks of things that can't even be applied without each other. Please rate limit what you are sending and wait for things to be applicable, or untangle the interdependencies between what appear to be tangentially related serieses. Some of these appear to contain fixes for other stuff you're sending...

Sorry about that, and thanks for explaining. I will wait for the SPI core series to be applied, then resend the others one at a time so that each one applies on its own against your tree.

Thanks,
Joseph