[PATCH 0/2] spi: Fix DMA mapping ownership on partial map failure

[PATCH 0/2] spi: Fix DMA mapping ownership on partial map failure

[Honghui Jiang]

__spi_map_msg() can fail after mapping only part of a message.  The
normal cleanup path may then act on stale mapped flags using a NULL or
stale DMA device, causing a NULL dereference or an extra unmap.

The core DMA mapping path is used by 28 in-tree SPI controllers.  The
bug remains reachable until a controller completes its first DMA-mapped
message, since cur_{tx,rx}_dma_dev are only set on success.  can_dma()
length checks may postpone that point well beyond probe.  A two-transfer
message with a mapped command followed by an unmappable static payload
reproduces the failure without memory pressure.

Patch 1 publishes the mapping devices before mapping starts and uses a
common unwind path for all failures.  Patch 2 adds KUnit coverage for
partial TX and RX mapping failures, successful map/unmap, and messages
which require no mapping.

Tested on v7.2-rc6 under x86_64 QEMU with KASAN.  All three cases that
previously oopsed complete cleanly after the fix, and the RX-only case
no longer issues an empty unmap.  DMA map/unmap counts changed from
3/4, 2/3 and 5/6 to 3/3, 2/2 and 5/5.

Two of the four KUnit cases fail without patch 1; all four pass with it.
Patch 1 also builds independently with x86_64_defconfig plus SPI.

Honghui Jiang (2):
  spi: Fix DMA mapping ownership on partial map failure
  spi: Add KUnit coverage for DMA mapping error paths

 drivers/spi/.kunitconfig      |   4 +
 drivers/spi/Kconfig           |  11 ++
 drivers/spi/spi.c             |  37 +++--
 drivers/spi/tests/spi_kunit.c | 301 ++++++++++++++++++++++++++++++++++
 4 files changed, 338 insertions(+), 15 deletions(-)
 create mode 100644 drivers/spi/.kunitconfig
 create mode 100644 drivers/spi/tests/spi_kunit.c


base-commit: 075b74841bd0065a3bda3440873c747938e69b68
-- 
2.43.0

[PATCH 1/2] spi: Fix DMA mapping ownership on partial map failure

[Honghui Jiang]

If RX mapping fails after TX mapping succeeds, __spi_map_msg() unmaps
TX but leaves tx_sg_mapped set.  If TX mapping fails on a later
transfer, mappings created for earlier transfers remain active.

In both cases, cur_{tx,rx}_dma_dev have not yet been updated because they
are assigned only after every transfer has been mapped.  The subsequent
spi_unmap_msg() may therefore unmap the TX mapping again or release
earlier mappings using a NULL or stale device.  An empty SG table does
not prevent the NULL dereference because dma_unmap_sg_attrs() accesses
the device before checking the entry count.

Publish both mapping devices before mapping starts and unwind all
failures through __spi_unmap_msg().  This clears the mapping flags and
releases each mapping once with the device that created it.

Link: https://lore.kernel.org/r/20240531194723.1761567-9-andriy.shevchenko@linux.intel.com
Fixes: e289df82344f ("spi: Rework per message DMA mapped flag to be per transfer")
Cc: stable@vger.kernel.org
Signed-off-by: Honghui Jiang <jiang_hh2019@163.com>
---
 drivers/spi/spi.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index d9e6b4b87..05a852494 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1231,6 +1231,8 @@ void spi_unmap_buf(struct spi_controller *ctlr, struct device *dev,
 	spi_unmap_buf_attrs(ctlr, dev, sgt, dir, 0);
 }
 
+static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg);
+
 static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 {
 	struct device *tx_dev, *rx_dev;
@@ -1254,7 +1256,14 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 	else
 		rx_dev = ctlr->dev.parent;
 
-	ret = -ENOMSG;
+	/*
+	 * Store the devices before mapping so partial failures can be unwound
+	 * with the device that created each mapping.
+	 */
+	ctlr->cur_tx_dma_dev = tx_dev;
+	ctlr->cur_rx_dma_dev = rx_dev;
+
+	ret = 0;
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
 		/* The sync is done before each transfer. */
 		unsigned long attrs = DMA_ATTR_SKIP_CPU_SYNC;
@@ -1268,7 +1277,7 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 						xfer->len, DMA_TO_DEVICE,
 						attrs);
 			if (ret != 0)
-				return ret;
+				goto unwind;
 
 			xfer->tx_sg_mapped = true;
 		}
@@ -1277,25 +1286,19 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 			ret = spi_map_buf_attrs(ctlr, rx_dev, &xfer->rx_sg,
 						xfer->rx_buf, xfer->len,
 						DMA_FROM_DEVICE, attrs);
-			if (ret != 0) {
-				spi_unmap_buf_attrs(ctlr, tx_dev,
-						&xfer->tx_sg, DMA_TO_DEVICE,
-						attrs);
-
-				return ret;
-			}
+			if (ret != 0)
+				goto unwind;
 
 			xfer->rx_sg_mapped = true;
 		}
 	}
-	/* No transfer has been mapped, bail out with success */
-	if (ret)
-		return 0;
-
-	ctlr->cur_rx_dma_dev = rx_dev;
-	ctlr->cur_tx_dma_dev = tx_dev;
 
 	return 0;
+
+unwind:
+	__spi_unmap_msg(ctlr, msg);
+
+	return ret;
 }
 
 static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg)
-- 
2.43.0

[PATCH 2/2] spi: Add KUnit coverage for DMA mapping error paths

[Honghui Jiang]

Add KUnit tests for the __spi_map_msg() error paths.  The tests verify
that mappings created before a later TX or RX failure are unwound, their
flags are cleared, and cur_{tx,rx}_dma_dev point to the device used for
the mapping.

Include the tests from spi.c so they can call the static mapping helpers
without adding test hooks to the production path.  A zero-length
transfer makes SG allocation fail with -EINVAL, providing deterministic
failure injection without additional fault-injection support.

Two guard cases cover successful map/unmap and a message which requires
no mapping.

Run the tests with:

  ./tools/testing/kunit/kunit.py run --arch=x86_64 \
      --kunitconfig=drivers/spi/.kunitconfig 'spi_core_error_path*'

Signed-off-by: Honghui Jiang <jiang_hh2019@163.com>
---
 drivers/spi/.kunitconfig      |   4 +
 drivers/spi/Kconfig           |  11 ++
 drivers/spi/spi.c             |   4 +
 drivers/spi/tests/spi_kunit.c | 301 ++++++++++++++++++++++++++++++++++
 4 files changed, 320 insertions(+)
 create mode 100644 drivers/spi/.kunitconfig
 create mode 100644 drivers/spi/tests/spi_kunit.c

diff --git a/drivers/spi/.kunitconfig b/drivers/spi/.kunitconfig
new file mode 100644
index 000000000..4f88fe164
--- /dev/null
+++ b/drivers/spi/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_SPI=y
+CONFIG_SPI_MASTER=y
+CONFIG_SPI_KUNIT_TEST=y
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 8782514bb..96351a4dc 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -48,6 +48,17 @@ config SPI_MASTER
 
 if SPI_MASTER
 
+config SPI_KUNIT_TEST
+	bool "KUnit tests for the SPI core" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y && HAS_DMA
+	default KUNIT_ALL_TESTS
+	help
+	  Run the SPI core's KUnit tests, covering the DMA mapping error
+	  paths of the message map/unmap state machine.
+
+	  If unsure say N.
+
+
 config SPI_MEM
 	bool "SPI memory extension"
 	help
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 05a852494..42e337c54 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -5146,3 +5146,7 @@ static int __init spi_init(void)
  * include needing to have boardinfo data structures be much more public.
  */
 postcore_initcall(spi_init);
+
+#ifdef CONFIG_SPI_KUNIT_TEST
+#include "tests/spi_kunit.c"
+#endif
diff --git a/drivers/spi/tests/spi_kunit.c b/drivers/spi/tests/spi_kunit.c
new file mode 100644
index 000000000..7a7e9e932
--- /dev/null
+++ b/drivers/spi/tests/spi_kunit.c
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit tests for the SPI core DMA mapping error paths.
+ *
+ * Included from spi.c so the tests can drive the static
+ * __spi_map_msg()/__spi_unmap_msg() state machine directly, without adding
+ * any indirection to the production mapping path.  Same arrangement as
+ * drivers/scsi/scsi_lib.c and lib/kunit/executor.c.
+ *
+ * The invariant under test:
+ *
+ *   When __spi_map_msg() returns an error, no transfer in the message may
+ *   still claim a DMA mapping.  Any transfer that was mapped before the
+ *   failure must have an empty SG table and a cleared *_sg_mapped flag.
+ *   ctlr->cur_{tx,rx}_dma_dev must identify the device used for this map,
+ *   rather than a device retained from an earlier message.
+ *
+ * That invariant is what makes the subsequent
+ * spi_finalize_current_message() -> spi_unmap_msg() -> __spi_unmap_msg()
+ * pass a no-op.  Without it, __spi_unmap_msg() unmaps again using
+ * ctlr->cur_{tx,rx}_dma_dev, which __spi_map_msg() only publishes after the
+ * whole loop succeeds -- so it is NULL on the controller's first
+ * DMA-mapped message.  dma_unmap_sg_attrs() dereferences that device before
+ * it ever looks at nents, so the second unmap is a NULL dereference
+ * regardless of the sgt having been emptied.
+ *
+ * How the failure is injected: a transfer with len == 0 makes
+ * spi_map_buf_attrs() compute sgs = DIV_ROUND_UP(0, desc_len) = 0, and
+ * __sg_alloc_table() rejects nents == 0 with -EINVAL.  That is instant,
+ * arch-independent and warning-free.  It is a test artifice standing in for
+ * any real map failure (-ENOMEM from sg_alloc_table(), -EIO from swiotlb
+ * exhaustion, -EINVAL from an unmappable buffer); the core's error handling
+ * does not depend on which one occurred.
+ */
+
+#include <kunit/device.h>
+#include <kunit/test.h>
+
+#define SPI_TEST_LEN		256
+#define SPI_TEST_XFERS		2
+
+struct spi_test_ctx {
+	struct spi_controller	*ctlr;
+	struct spi_device	*spi;
+	struct device		*dma_dev;
+	struct device		*stale_dma_dev;
+	struct spi_transfer	xfer[SPI_TEST_XFERS];
+	struct spi_message	msg;
+	void			*buf[SPI_TEST_XFERS * 2];
+};
+
+static bool spi_test_can_dma(struct spi_controller *ctlr,
+			     struct spi_device *spi,
+			     struct spi_transfer *xfer)
+{
+	/* Opt every transfer into the core DMA mapping path. */
+	return true;
+}
+
+/*
+ * A bare kzalloc'd controller is enough: __spi_map_msg() and
+ * __spi_unmap_msg() only touch can_dma, dma_tx, dma_rx, dma_map_dev,
+ * max_dma_len and cur_*_dma_dev.  Leaving dma_tx/dma_rx NULL makes both
+ * directions resolve to dma_map_dev, so there is no need to fake dmaengine
+ * channels, and ctlr->dev is never dereferenced.  Skipping
+ * spi_alloc_host()/spi_register_controller() keeps the fixture free of
+ * device and queue lifecycle.
+ */
+static struct spi_test_ctx *spi_test_ctx_new(struct kunit *test)
+{
+	struct spi_test_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ctx->dma_dev = kunit_device_register(test, "spi-core-error-path");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dma_dev);
+	ctx->stale_dma_dev =
+		kunit_device_register(test, "spi-core-stale-dma-device");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->stale_dma_dev);
+
+	/* Real masks keep either device safe if a failing assertion aborts. */
+	KUNIT_ASSERT_EQ(test, 0,
+			dma_coerce_mask_and_coherent(ctx->dma_dev,
+						     DMA_BIT_MASK(64)));
+	KUNIT_ASSERT_EQ(test, 0,
+			dma_coerce_mask_and_coherent(ctx->stale_dma_dev,
+						     DMA_BIT_MASK(64)));
+
+	ctx->ctlr = kunit_kzalloc(test, sizeof(*ctx->ctlr), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->ctlr);
+
+	ctx->spi = kunit_kzalloc(test, sizeof(*ctx->spi), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->spi);
+
+	ctx->ctlr->can_dma = spi_test_can_dma;
+	ctx->ctlr->dma_map_dev = ctx->dma_dev;
+	/* spi_register_controller() would do this; we are not registering. */
+	ctx->ctlr->max_dma_len = INT_MAX;
+
+	ctx->spi->controller = ctx->ctlr;
+	spi_message_init(&ctx->msg);
+	ctx->msg.spi = ctx->spi;
+
+	return ctx;
+}
+
+static void *spi_test_buf(struct kunit *test, struct spi_test_ctx *ctx,
+			  unsigned int slot)
+{
+	KUNIT_ASSERT_LT(test, slot, ARRAY_SIZE(ctx->buf));
+
+	ctx->buf[slot] = kunit_kzalloc(test, SPI_TEST_LEN, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->buf[slot]);
+
+	return ctx->buf[slot];
+}
+
+/*
+ * Pin cur_*_dma_dev to a different valid device before the failing map.  This
+ * emulates state retained from an earlier message and lets the test verify
+ * that __spi_map_msg() publishes the device which owns the new mappings.
+ * It also keeps an unfixed tree from dereferencing NULL while reporting the
+ * regression.  ASSERTs abort the case before cleanup can use the stale device.
+ */
+static void spi_test_pin_stale_dma_devs(struct spi_test_ctx *ctx)
+{
+	ctx->ctlr->cur_tx_dma_dev = ctx->stale_dma_dev;
+	ctx->ctlr->cur_rx_dma_dev = ctx->stale_dma_dev;
+}
+
+static void spi_test_assert_dma_devs_published(struct kunit *test,
+					       struct spi_test_ctx *ctx)
+{
+	KUNIT_ASSERT_PTR_EQ(test, ctx->ctlr->cur_tx_dma_dev, ctx->dma_dev);
+	KUNIT_ASSERT_PTR_EQ(test, ctx->ctlr->cur_rx_dma_dev, ctx->dma_dev);
+}
+
+static void spi_test_assert_nothing_mapped(struct kunit *test,
+					   struct spi_test_ctx *ctx,
+					   unsigned int nr_xfers)
+{
+	unsigned int i;
+
+	for (i = 0; i < nr_xfers; i++) {
+		KUNIT_ASSERT_FALSE_MSG(test, ctx->xfer[i].tx_sg_mapped,
+				       "xfer[%u] still claims a TX mapping after __spi_map_msg() failed",
+				       i);
+		KUNIT_ASSERT_FALSE_MSG(test, ctx->xfer[i].rx_sg_mapped,
+				       "xfer[%u] still claims an RX mapping after __spi_map_msg() failed",
+				       i);
+		KUNIT_EXPECT_PTR_EQ(test, ctx->xfer[i].tx_sg.sgl, NULL);
+		KUNIT_EXPECT_EQ(test, ctx->xfer[i].tx_sg.orig_nents, 0U);
+		KUNIT_EXPECT_EQ(test, ctx->xfer[i].tx_sg.nents, 0U);
+		KUNIT_EXPECT_PTR_EQ(test, ctx->xfer[i].rx_sg.sgl, NULL);
+		KUNIT_EXPECT_EQ(test, ctx->xfer[i].rx_sg.orig_nents, 0U);
+		KUNIT_EXPECT_EQ(test, ctx->xfer[i].rx_sg.nents, 0U);
+	}
+}
+
+/*
+ * xfer0 maps TX and RX; xfer1's TX map fails.
+ *
+ * This exits __spi_map_msg() through the bare `return ret` after the TX
+ * spi_map_buf_attrs() call, which has no rollback code at all -- not even
+ * the ad-hoc one the RX branch has.  xfer0 is left fully mapped with both
+ * flags set and cur_*_dma_dev unpublished.
+ *
+ * This is the deterministic real-world shape: a driver whose second transfer
+ * hands over a buffer the core cannot map (e.g. a static const payload table
+ * after a kmalloc'd command byte) hits it with no memory pressure at all.
+ */
+static void spi_later_tx_fail_rolls_back_earlier(struct kunit *test)
+{
+	struct spi_test_ctx *ctx = spi_test_ctx_new(test);
+	int ret;
+
+	ctx->xfer[0].tx_buf = spi_test_buf(test, ctx, 0);
+	ctx->xfer[0].rx_buf = spi_test_buf(test, ctx, 1);
+	ctx->xfer[0].len = SPI_TEST_LEN;
+
+	ctx->xfer[1].tx_buf = spi_test_buf(test, ctx, 2);
+	ctx->xfer[1].rx_buf = NULL;
+	ctx->xfer[1].len = 0;			/* forces -EINVAL */
+
+	spi_message_add_tail(&ctx->xfer[0], &ctx->msg);
+	spi_message_add_tail(&ctx->xfer[1], &ctx->msg);
+
+	spi_test_pin_stale_dma_devs(ctx);
+
+	ret = __spi_map_msg(ctx->ctlr, &ctx->msg);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+
+	spi_test_assert_dma_devs_published(test, ctx);
+	spi_test_assert_nothing_mapped(test, ctx, SPI_TEST_XFERS);
+
+	/* Only reached once the invariant holds: cleanup must be a no-op. */
+	KUNIT_EXPECT_EQ(test, 0, __spi_unmap_msg(ctx->ctlr, &ctx->msg));
+}
+
+/*
+ * xfer0 maps TX and RX; the RX-only xfer1 then fails to map.
+ *
+ * The old RX failure branch attempts to unmap xfer1's never-mapped TX table,
+ * then returns without rolling back xfer0 or publishing cur_*_dma_dev.
+ */
+static void spi_later_rx_fail_rolls_back_earlier(struct kunit *test)
+{
+	struct spi_test_ctx *ctx = spi_test_ctx_new(test);
+	int ret;
+
+	ctx->xfer[0].tx_buf = spi_test_buf(test, ctx, 0);
+	ctx->xfer[0].rx_buf = spi_test_buf(test, ctx, 1);
+	ctx->xfer[0].len = SPI_TEST_LEN;
+
+	ctx->xfer[1].tx_buf = NULL;
+	ctx->xfer[1].rx_buf = spi_test_buf(test, ctx, 2);
+	ctx->xfer[1].len = 0;			/* forces -EINVAL */
+
+	spi_message_add_tail(&ctx->xfer[0], &ctx->msg);
+	spi_message_add_tail(&ctx->xfer[1], &ctx->msg);
+
+	spi_test_pin_stale_dma_devs(ctx);
+
+	ret = __spi_map_msg(ctx->ctlr, &ctx->msg);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+
+	spi_test_assert_dma_devs_published(test, ctx);
+	spi_test_assert_nothing_mapped(test, ctx, SPI_TEST_XFERS);
+
+	KUNIT_EXPECT_EQ(test, 0, __spi_unmap_msg(ctx->ctlr, &ctx->msg));
+}
+
+/*
+ * Happy-path guard, so a fix that unwinds too eagerly cannot pass: a fully
+ * mappable message must still map both directions, publish both devices, and
+ * unmap cleanly.
+ */
+static void spi_map_success_publishes_dma_devs(struct kunit *test)
+{
+	struct spi_test_ctx *ctx = spi_test_ctx_new(test);
+	int ret;
+
+	ctx->xfer[0].tx_buf = spi_test_buf(test, ctx, 0);
+	ctx->xfer[0].rx_buf = spi_test_buf(test, ctx, 1);
+	ctx->xfer[0].len = SPI_TEST_LEN;
+
+	spi_message_add_tail(&ctx->xfer[0], &ctx->msg);
+
+	ret = __spi_map_msg(ctx->ctlr, &ctx->msg);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_TRUE(test, ctx->xfer[0].tx_sg_mapped);
+	KUNIT_EXPECT_TRUE(test, ctx->xfer[0].rx_sg_mapped);
+	KUNIT_EXPECT_PTR_EQ(test, ctx->ctlr->cur_tx_dma_dev, ctx->dma_dev);
+	KUNIT_EXPECT_PTR_EQ(test, ctx->ctlr->cur_rx_dma_dev, ctx->dma_dev);
+
+	KUNIT_EXPECT_EQ(test, 0, __spi_unmap_msg(ctx->ctlr, &ctx->msg));
+
+	KUNIT_EXPECT_FALSE(test, ctx->xfer[0].tx_sg_mapped);
+	KUNIT_EXPECT_FALSE(test, ctx->xfer[0].rx_sg_mapped);
+	KUNIT_EXPECT_PTR_EQ(test, ctx->xfer[0].tx_sg.sgl, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, ctx->xfer[0].rx_sg.sgl, NULL);
+}
+
+/*
+ * The "no transfer has been mapped, bail out with success" path: a message
+ * whose only transfer has neither buffer maps nothing and must still return
+ * success with no flags set.
+ */
+static void spi_map_nothing_is_success(struct kunit *test)
+{
+	struct spi_test_ctx *ctx = spi_test_ctx_new(test);
+	int ret;
+
+	ctx->xfer[0].tx_buf = NULL;
+	ctx->xfer[0].rx_buf = NULL;
+	ctx->xfer[0].len = SPI_TEST_LEN;
+
+	spi_message_add_tail(&ctx->xfer[0], &ctx->msg);
+
+	ret = __spi_map_msg(ctx->ctlr, &ctx->msg);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	spi_test_assert_nothing_mapped(test, ctx, 1);
+}
+
+static struct kunit_case spi_core_error_path_cases[] = {
+	KUNIT_CASE(spi_later_tx_fail_rolls_back_earlier),
+	KUNIT_CASE(spi_later_rx_fail_rolls_back_earlier),
+	KUNIT_CASE(spi_map_success_publishes_dma_devs),
+	KUNIT_CASE(spi_map_nothing_is_success),
+	{}
+};
+
+static struct kunit_suite spi_core_error_path_suite = {
+	.name = "spi_core_error_path",
+	.test_cases = spi_core_error_path_cases,
+};
+
+kunit_test_suite(spi_core_error_path_suite);
-- 
2.43.0

Re: [PATCH 2/2] spi: Add KUnit coverage for DMA mapping error paths

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Wed, Aug 05, 2026 at 11:14:56PM +0800, Honghui Jiang wrote:
> Add KUnit tests for the __spi_map_msg() error paths.  The tests verify
> that mappings created before a later TX or RX failure are unwound, their
> flags are cleared, and cur_{tx,rx}_dma_dev point to the device used for
> the mapping.

The tests themselves look good but some style/integration stuff:

> --- /dev/null
> +++ b/drivers/spi/.kunitconfig
> @@ -0,0 +1,4 @@
> +CONFIG_KUNIT=y
> +CONFIG_SPI=y
> +CONFIG_SPI_MASTER=y
> +CONFIG_SPI_KUNIT_TEST=y

This also needs enabling in the KUnit defconfigs in
tools/testing/kunit/configs.

> index 05a852494..42e337c54 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -5146,3 +5146,7 @@ static int __init spi_init(void)
>   * include needing to have boardinfo data structures be much more public.
>   */
>  postcore_initcall(spi_init);
> +
> +#ifdef CONFIG_SPI_KUNIT_TEST
> +#include "tests/spi_kunit.c"
> +#endif

If this is just so we can peer into the internals it'd be better to add
a local header for internals and build as a separate translation unit
like normal rather than doing this sort of bodge.

> diff --git a/drivers/spi/tests/spi_kunit.c b/drivers/spi/tests/spi_kunit.c

Everywhere else we use - as a separator.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for the SPI core DMA mapping error paths.
> + *

Please make the entire comment a C++ one so things look more intentional.

> + * The invariant under test:
> + *
> + *   When __spi_map_msg() returns an error, no transfer in the message may
> + *   still claim a DMA mapping.  Any transfer that was mapped before the
> + *   failure must have an empty SG table and a cleared *_sg_mapped flag.
> + *   ctlr->cur_{tx,rx}_dma_dev must identify the device used for this map,
> + *   rather than a device retained from an earlier message.

This is for one specific test, either this file should have a test
specific name with everything else namespaced to make room for further
tests or all this should go with the specific test.  This looks like a
DMA subsuite so possibly named after that?  The same applies to internal
identifiers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: Add KUnit coverage for DMA mapping error paths

[江宏辉]

Thanks for the review.

I'll address these comments in v2 and rework the test integration
as suggested.

At 2026-08-06 05:57:41, "Mark Brown" <broonie@kernel.org> wrote:
>On Wed, Aug 05, 2026 at 11:14:56PM +0800, Honghui Jiang wrote:
>> Add KUnit tests for the __spi_map_msg() error paths.  The tests verify
>> that mappings created before a later TX or RX failure are unwound, their
>> flags are cleared, and cur_{tx,rx}_dma_dev point to the device used for
>> the mapping.
>
>The tests themselves look good but some style/integration stuff:
>
>> --- /dev/null
>> +++ b/drivers/spi/.kunitconfig
>> @@ -0,0 +1,4 @@
>> +CONFIG_KUNIT=y
>> +CONFIG_SPI=y
>> +CONFIG_SPI_MASTER=y
>> +CONFIG_SPI_KUNIT_TEST=y
>
>This also needs enabling in the KUnit defconfigs in
>tools/testing/kunit/configs.
>
>> index 05a852494..42e337c54 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -5146,3 +5146,7 @@ static int __init spi_init(void)
>>   * include needing to have boardinfo data structures be much more public.
>>   */
>>  postcore_initcall(spi_init);
>> +
>> +#ifdef CONFIG_SPI_KUNIT_TEST
>> +#include "tests/spi_kunit.c"
>> +#endif
>
>If this is just so we can peer into the internals it'd be better to add
>a local header for internals and build as a separate translation unit
>like normal rather than doing this sort of bodge.
>
>> diff --git a/drivers/spi/tests/spi_kunit.c b/drivers/spi/tests/spi_kunit.c
>
>Everywhere else we use - as a separator.
>
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * KUnit tests for the SPI core DMA mapping error paths.
>> + *
>
>Please make the entire comment a C++ one so things look more intentional.
>
>> + * The invariant under test:
>> + *
>> + *   When __spi_map_msg() returns an error, no transfer in the message may
>> + *   still claim a DMA mapping.  Any transfer that was mapped before the
>> + *   failure must have an empty SG table and a cleared *_sg_mapped flag.
>> + *   ctlr->cur_{tx,rx}_dma_dev must identify the device used for this map,
>> + *   rather than a device retained from an earlier message.
>
>This is for one specific test, either this file should have a test
>specific name with everything else namespaced to make room for further
>tests or all this should go with the specific test.  This looks like a
>DMA subsuite so possibly named after that?  The same applies to internal
>identifiers.

Re: [PATCH 2/2] spi: Add KUnit coverage for DMA mapping error paths

[Andy Shevchenko]

On Thu, Aug 6, 2026 at 12:57 AM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 05, 2026 at 11:14:56PM +0800, Honghui Jiang wrote:

...

> >  postcore_initcall(spi_init);
> > +
> > +#ifdef CONFIG_SPI_KUNIT_TEST
> > +#include "tests/spi_kunit.c"
> > +#endif
>
> If this is just so we can peer into the internals it'd be better to add
> a local header for internals and build as a separate translation unit
> like normal rather than doing this sort of bodge.

Yep, and we have kunit/visibility.h in case it is needed.

> > diff --git a/drivers/spi/tests/spi_kunit.c b/drivers/spi/tests/spi_kunit.c
>
> Everywhere else we use - as a separator.

I think for kunit tests there is a pattern which is xxx_kunit.c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] spi: Fix DMA mapping ownership on partial map failure

[Andy Shevchenko]

On Wed, Aug 5, 2026 at 6:15 PM Honghui Jiang <jiang_hh2019@163.com> wrote:
>
> If RX mapping fails after TX mapping succeeds, __spi_map_msg() unmaps
> TX but leaves tx_sg_mapped set.  If TX mapping fails on a later
> transfer, mappings created for earlier transfers remain active.
>
> In both cases, cur_{tx,rx}_dma_dev have not yet been updated because they
> are assigned only after every transfer has been mapped.  The subsequent
> spi_unmap_msg() may therefore unmap the TX mapping again or release
> earlier mappings using a NULL or stale device.  An empty SG table does
> not prevent the NULL dereference because dma_unmap_sg_attrs() accesses
> the device before checking the entry count.
>
> Publish both mapping devices before mapping starts and unwind all
> failures through __spi_unmap_msg().  This clears the mapping flags and
> releases each mapping once with the device that created it.

The Fixes tag refers to the commit that moved from per message to per
transfer mapping flags. Can you elaborate why it was no problem
before? (Not sure if we need the answer to be included in the commit
message, probably the cover letter is the best choice for this info.)

...

> +static int __spi_unmap_msg(struct spi_controller *ctlr, struct spi_message *msg);

For a fix this is probably okay to introduce, but can we get rid of
forward declaration by moving the unmapping routine above? If so, can
you also add another patch for that?

>  static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)

...

> -       ret = -ENOMSG;
> +       /*
> +        * Store the devices before mapping so partial failures can be unwound
> +        * with the device that created each mapping.
> +        */
> +       ctlr->cur_tx_dma_dev = tx_dev;
> +       ctlr->cur_rx_dma_dev = rx_dev;

Does unwind routine nullify them? If not, there is a chance that some
code (maybe in the future) might use stale pointers.

...

>                         if (ret != 0)
> -                               return ret;
> +                               goto unwind;

> +                       if (ret != 0)
> +                               goto unwind;

Since you touched these lines, perhaps it makes sense to drop ' != 0'
parts to make it follow the regular pattern.


--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] spi: Fix DMA mapping ownership on partial map failure

[Honghui Jiang]

Thanks for the review.

On Thu, Aug 06, 2026 at 10:34:16PM +0300, Andy Shevchenko wrote:
> The Fixes tag refers to the commit that moved from per message to per
> transfer mapping flags. Can you elaborate why it was no problem
> before? (Not sure if we need the answer to be included in the commit
> message, probably the cover letter is the best choice for this info.)

The partial-failure handling was already incomplete before that commit,
but the failure mode was different. __spi_unmap_msg() was gated by
ctlr->cur_msg_mapped, which was set only after the whole message had
been mapped successfully. A partial failure therefore skipped the
normal unmap path: it could leak mappings made for earlier transfers,
but it could not unmap them again using an unpublished DMA device.

The per-transfer conversion removed that message-wide gate. The
per-transfer flags can now remain set after a partial failure, while
cur_{tx,rx}_dma_dev are still published only after the whole mapping
loop succeeds. The subsequent cleanup can therefore unmap those
transfers using a NULL or stale device. That is the regression referred
to by the Fixes tag.

Agreed. I'll add this explanation to the v2 cover letter.

> For a fix this is probably okay to introduce, but can we get rid of
> forward declaration by moving the unmapping routine above? If so, can
> you also add another patch for that?

Yes. I'll keep the forward declaration in the fix so that it remains
independently buildable and backportable. I'll then add a follow-up
cleanup patch moving __spi_unmap_msg() above __spi_map_msg() and
removing the declaration.

> Does unwind routine nullify them? If not, there is a chance that some
> code (maybe in the future) might use stale pointers.

It currently does not.

I checked the current users. The three users in the SPI core only use a
device when the corresponding *_sg_mapped flag is set. The accesses in
spi-amlogic-spisg also occur before spi_finalize_current_message() on
every path which reaches them.

Nevertheless, I agree that the pointers should not survive the message.
I'll clear them in a separate follow-up cleanup patch, in
spi_unmap_msg() after __spi_unmap_msg() returns, rather than inside
__spi_unmap_msg() itself. The latter is also used for the in-message
DMA-to-PIO fallback and therefore does not mark the end of the message
lifetime.

> Since you touched these lines, perhaps it makes sense to drop ' != 0'
> parts to make it follow the regular pattern.

Will do. Both checks will use plain if (ret) in v2.

Thanks,
Honghui