[PATCH 0/5] spi: imx: fix use-after-free on unbind

[PATCH 0/5] spi: imx: fix use-after-free on unbind

[Johan Hovold]

The SPI subsystem frees the controller and any subsystem allocated
driver data as part of deregistration (unless the allocation is device
managed).

This series fixes the IMX driver that got this wrong and then converts
it to use device managed allocation.

Included are also related cleanups for tegre20-slink and the rockchip
driver.

Johan


Johan Hovold (5):
  spi: imx: fix use-after-free on unbind
  spi: imx: switch to managed controller allocation
  spi: tegra20-slink: switch to managed controller allocation
  spi: rockchip: fix controller deregistration
  spi: rockchip: switch to managed controller allocation

 drivers/spi/spi-imx.c           | 41 +++++++++++----------------------
 drivers/spi/spi-rockchip.c      | 40 +++++++++++++-------------------
 drivers/spi/spi-tegra20-slink.c | 26 ++++++++-------------
 3 files changed, 40 insertions(+), 67 deletions(-)

-- 
2.52.0

[PATCH 1/5] spi: imx: fix use-after-free on unbind

[Johan Hovold]

The SPI subsystem frees the controller and any subsystem allocated
driver data as part of deregistration (unless the allocation is device
managed).

Take another reference before deregistering the controller so that the
driver data is not freed until the driver is done with it.

Fixes: 307c897db762 ("spi: spi-imx: replace struct spi_imx_data::bitbang by pointer to struct spi_controller")
Cc: stable@vger.kernel.org	# 5.19
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-imx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 64c6c09e1e7b..a8d90c86a8a1 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -2401,6 +2401,8 @@ static void spi_imx_remove(struct platform_device *pdev)
 	struct spi_imx_data *spi_imx = spi_controller_get_devdata(controller);
 	int ret;
 
+	spi_controller_get(controller);
+
 	spi_unregister_controller(controller);
 
 	ret = pm_runtime_get_sync(spi_imx->dev);
@@ -2414,6 +2416,8 @@ static void spi_imx_remove(struct platform_device *pdev)
 	pm_runtime_disable(spi_imx->dev);
 
 	spi_imx_sdma_exit(spi_imx);
+
+	spi_controller_put(controller);
 }
 
 static int spi_imx_runtime_resume(struct device *dev)
-- 
2.52.0

[PATCH 2/5] spi: imx: switch to managed controller allocation

[Johan Hovold]

Switch to device managed controller allocation to simplify error
handling and to avoid having to take another reference during
deregistration.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-imx.c | 45 ++++++++++++++-----------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index a8d90c86a8a1..4747899e0646 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -2231,11 +2231,9 @@ static int spi_imx_probe(struct platform_device *pdev)
 	target_mode = devtype_data->has_targetmode &&
 		      of_property_read_bool(np, "spi-slave");
 	if (target_mode)
-		controller = spi_alloc_target(&pdev->dev,
-					      sizeof(struct spi_imx_data));
+		controller = devm_spi_alloc_target(&pdev->dev, sizeof(*spi_imx));
 	else
-		controller = spi_alloc_host(&pdev->dev,
-					    sizeof(struct spi_imx_data));
+		controller = devm_spi_alloc_host(&pdev->dev, sizeof(*spi_imx));
 	if (!controller)
 		return -ENOMEM;
 
@@ -2304,40 +2302,31 @@ static int spi_imx_probe(struct platform_device *pdev)
 	init_completion(&spi_imx->xfer_done);
 
 	spi_imx->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(spi_imx->base)) {
-		ret = PTR_ERR(spi_imx->base);
-		goto out_controller_put;
-	}
+	if (IS_ERR(spi_imx->base))
+		return PTR_ERR(spi_imx->base);
+
 	spi_imx->base_phys = res->start;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		ret = irq;
-		goto out_controller_put;
-	}
+	if (irq < 0)
+		return irq;
 
 	ret = devm_request_irq(&pdev->dev, irq, spi_imx_isr, 0,
 			       dev_name(&pdev->dev), spi_imx);
-	if (ret) {
-		dev_err(&pdev->dev, "can't get irq%d: %d\n", irq, ret);
-		goto out_controller_put;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "can't get irq%d\n", irq);
 
 	spi_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
-	if (IS_ERR(spi_imx->clk_ipg)) {
-		ret = PTR_ERR(spi_imx->clk_ipg);
-		goto out_controller_put;
-	}
+	if (IS_ERR(spi_imx->clk_ipg))
+		return PTR_ERR(spi_imx->clk_ipg);
 
 	spi_imx->clk_per = devm_clk_get(&pdev->dev, "per");
-	if (IS_ERR(spi_imx->clk_per)) {
-		ret = PTR_ERR(spi_imx->clk_per);
-		goto out_controller_put;
-	}
+	if (IS_ERR(spi_imx->clk_per))
+		return PTR_ERR(spi_imx->clk_per);
 
 	ret = clk_prepare_enable(spi_imx->clk_per);
 	if (ret)
-		goto out_controller_put;
+		return ret;
 
 	ret = clk_prepare_enable(spi_imx->clk_ipg);
 	if (ret)
@@ -2389,8 +2378,6 @@ static int spi_imx_probe(struct platform_device *pdev)
 	clk_disable_unprepare(spi_imx->clk_ipg);
 out_put_per:
 	clk_disable_unprepare(spi_imx->clk_per);
-out_controller_put:
-	spi_controller_put(controller);
 
 	return ret;
 }
@@ -2401,8 +2388,6 @@ static void spi_imx_remove(struct platform_device *pdev)
 	struct spi_imx_data *spi_imx = spi_controller_get_devdata(controller);
 	int ret;
 
-	spi_controller_get(controller);
-
 	spi_unregister_controller(controller);
 
 	ret = pm_runtime_get_sync(spi_imx->dev);
@@ -2416,8 +2401,6 @@ static void spi_imx_remove(struct platform_device *pdev)
 	pm_runtime_disable(spi_imx->dev);
 
 	spi_imx_sdma_exit(spi_imx);
-
-	spi_controller_put(controller);
 }
 
 static int spi_imx_runtime_resume(struct device *dev)
-- 
2.52.0

[PATCH 3/5] spi: tegra20-slink: switch to managed controller allocation

[Johan Hovold]

Switch to device managed controller allocation to simplify error
handling and to avoid having to take another reference during
deregistration.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-tegra20-slink.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
index 8c608abd6076..c15c076295cd 100644
--- a/drivers/spi/spi-tegra20-slink.c
+++ b/drivers/spi/spi-tegra20-slink.c
@@ -1007,7 +1007,7 @@ static int tegra_slink_probe(struct platform_device *pdev)
 
 	cdata = of_device_get_match_data(&pdev->dev);
 
-	host = spi_alloc_host(&pdev->dev, sizeof(*tspi));
+	host = devm_spi_alloc_host(&pdev->dev, sizeof(*tspi));
 	if (!host) {
 		dev_err(&pdev->dev, "host allocation failed\n");
 		return -ENOMEM;
@@ -1034,37 +1034,34 @@ static int tegra_slink_probe(struct platform_device *pdev)
 		host->max_speed_hz = 25000000; /* 25MHz */
 
 	tspi->base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
-	if (IS_ERR(tspi->base)) {
-		ret = PTR_ERR(tspi->base);
-		goto exit_free_host;
-	}
+	if (IS_ERR(tspi->base))
+		return PTR_ERR(tspi->base);
+
 	tspi->phys = r->start;
 
 	/* disabled clock may cause interrupt storm upon request */
 	tspi->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(tspi->clk)) {
 		ret = PTR_ERR(tspi->clk);
-		dev_err(&pdev->dev, "Can not get clock %d\n", ret);
-		goto exit_free_host;
+		return dev_err_probe(&pdev->dev, ret, "Can not get clock\n");
 	}
 
 	tspi->rst = devm_reset_control_get_exclusive(&pdev->dev, "spi");
 	if (IS_ERR(tspi->rst)) {
-		dev_err(&pdev->dev, "can not get reset\n");
 		ret = PTR_ERR(tspi->rst);
-		goto exit_free_host;
+		return dev_err_probe(&pdev->dev, ret, "can not get reset\n");
 	}
 
 	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
 	if (ret)
-		goto exit_free_host;
+		return ret;
 
 	tspi->max_buf_size = SLINK_FIFO_DEPTH << 2;
 	tspi->dma_buf_size = DEFAULT_SPI_DMA_BUF_LEN;
 
 	ret = tegra_slink_init_dma_param(tspi, true);
 	if (ret < 0)
-		goto exit_free_host;
+		return ret;
 	ret = tegra_slink_init_dma_param(tspi, false);
 	if (ret < 0)
 		goto exit_rx_dma_free;
@@ -1125,14 +1122,13 @@ static int tegra_slink_probe(struct platform_device *pdev)
 	tegra_slink_deinit_dma_param(tspi, false);
 exit_rx_dma_free:
 	tegra_slink_deinit_dma_param(tspi, true);
-exit_free_host:
-	spi_controller_put(host);
+
 	return ret;
 }
 
 static void tegra_slink_remove(struct platform_device *pdev)
 {
-	struct spi_controller *host = spi_controller_get(platform_get_drvdata(pdev));
+	struct spi_controller *host = platform_get_drvdata(pdev);
 	struct tegra_slink_data	*tspi = spi_controller_get_devdata(host);
 
 	spi_unregister_controller(host);
@@ -1146,8 +1142,6 @@ static void tegra_slink_remove(struct platform_device *pdev)
 
 	if (tspi->rx_dma_chan)
 		tegra_slink_deinit_dma_param(tspi, true);
-
-	spi_controller_put(host);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.52.0

[PATCH 4/5] spi: rockchip: fix controller deregistration

[Johan Hovold]

Make sure to deregister the controller before freeing underlying
resources like DMA channels during driver unbind.

Fixes: 64e36824b32b ("spi/rockchip: add driver for Rockchip RK3xxx SoCs integrated SPI")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-rockchip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index fd2ebef4903f..eb1992b4178e 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -908,7 +908,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		break;
 	}
 
-	ret = devm_spi_register_controller(&pdev->dev, ctlr);
+	ret = spi_register_controller(ctlr);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register controller\n");
 		goto err_free_dma_rx;
@@ -936,6 +936,8 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
+	spi_unregister_controller(ctlr);
+
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
-- 
2.52.0

[PATCH 5/5] spi: rockchip: switch to managed controller allocation

[Johan Hovold]

Switch to device managed controller allocation to simplify error
handling and to avoid having to take another reference during
deregistration.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/spi/spi-rockchip.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index eb1992b4178e..14cd1b9d9793 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -767,9 +767,9 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	target_mode = of_property_read_bool(np, "spi-slave");
 
 	if (target_mode)
-		ctlr = spi_alloc_target(&pdev->dev, sizeof(struct rockchip_spi));
+		ctlr = devm_spi_alloc_target(&pdev->dev, sizeof(*rs));
 	else
-		ctlr = spi_alloc_host(&pdev->dev, sizeof(struct rockchip_spi));
+		ctlr = devm_spi_alloc_host(&pdev->dev, sizeof(*rs));
 
 	if (!ctlr)
 		return -ENOMEM;
@@ -780,35 +780,31 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 
 	/* Get basic io resource and map it */
 	rs->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
-	if (IS_ERR(rs->regs)) {
-		ret = PTR_ERR(rs->regs);
-		goto err_put_ctlr;
-	}
+	if (IS_ERR(rs->regs))
+		return PTR_ERR(rs->regs);
 
 	rs->apb_pclk = devm_clk_get_enabled(&pdev->dev, "apb_pclk");
 	if (IS_ERR(rs->apb_pclk)) {
-		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->apb_pclk),
-				    "Failed to get apb_pclk\n");
-		goto err_put_ctlr;
+		return dev_err_probe(&pdev->dev, PTR_ERR(rs->apb_pclk),
+				     "Failed to get apb_pclk\n");
 	}
 
 	rs->spiclk = devm_clk_get_enabled(&pdev->dev, "spiclk");
 	if (IS_ERR(rs->spiclk)) {
-		ret = dev_err_probe(&pdev->dev, PTR_ERR(rs->spiclk),
-				    "Failed to get spi_pclk\n");
-		goto err_put_ctlr;
+		return dev_err_probe(&pdev->dev, PTR_ERR(rs->spiclk),
+				     "Failed to get spi_pclk\n");
 	}
 
 	spi_enable_chip(rs, false);
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0)
-		goto err_put_ctlr;
+		return ret;
 
 	ret = devm_request_irq(&pdev->dev, ret, rockchip_spi_isr, 0,
 			       dev_name(&pdev->dev), ctlr);
 	if (ret)
-		goto err_put_ctlr;
+		return ret;
 
 	rs->dev = &pdev->dev;
 	rs->freq = clk_get_rate(rs->spiclk);
@@ -830,10 +826,8 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	}
 
 	rs->fifo_len = get_fifo_len(rs);
-	if (!rs->fifo_len) {
-		ret = dev_err_probe(&pdev->dev, -EINVAL, "Failed to get fifo length\n");
-		goto err_put_ctlr;
-	}
+	if (!rs->fifo_len)
+		return dev_err_probe(&pdev->dev, -EINVAL, "Failed to get fifo length\n");
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, ROCKCHIP_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
@@ -924,15 +918,13 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		dma_release_channel(ctlr->dma_tx);
 err_disable_pm_runtime:
 	pm_runtime_disable(&pdev->dev);
-err_put_ctlr:
-	spi_controller_put(ctlr);
 
 	return ret;
 }
 
 static void rockchip_spi_remove(struct platform_device *pdev)
 {
-	struct spi_controller *ctlr = spi_controller_get(platform_get_drvdata(pdev));
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
 
 	pm_runtime_get_sync(&pdev->dev);
 
@@ -946,8 +938,6 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 		dma_release_channel(ctlr->dma_tx);
 	if (ctlr->dma_rx)
 		dma_release_channel(ctlr->dma_rx);
-
-	spi_controller_put(ctlr);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.52.0

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Marc Kleine-Budde]

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

On 23.03.2026 11:49:44, Johan Hovold wrote:
> The SPI subsystem frees the controller and any subsystem allocated
> driver data as part of deregistration (unless the allocation is device
> managed).
>
> Take another reference before deregistering the controller so that the
> driver data is not freed until the driver is done with it.

Would re-ordering the spi_imx_remove() function be an alternative fix?
I.e. call spi_unregister_controller() last?

regards,
Marc

>
> Fixes: 307c897db762 ("spi: spi-imx: replace struct spi_imx_data::bitbang by pointer to struct spi_controller")
> Cc: stable@vger.kernel.org	# 5.19
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/spi/spi-imx.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 64c6c09e1e7b..a8d90c86a8a1 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -2401,6 +2401,8 @@ static void spi_imx_remove(struct platform_device *pdev)
>  	struct spi_imx_data *spi_imx = spi_controller_get_devdata(controller);
>  	int ret;
>
> +	spi_controller_get(controller);
> +
>  	spi_unregister_controller(controller);
>
>  	ret = pm_runtime_get_sync(spi_imx->dev);
> @@ -2414,6 +2416,8 @@ static void spi_imx_remove(struct platform_device *pdev)
>  	pm_runtime_disable(spi_imx->dev);
>
>  	spi_imx_sdma_exit(spi_imx);
> +
> +	spi_controller_put(controller);
>  }
>
>  static int spi_imx_runtime_resume(struct device *dev)
> --
> 2.52.0
>
>

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 12:00:59PM +0100, Marc Kleine-Budde wrote:
> On 23.03.2026 11:49:44, Johan Hovold wrote:
> > The SPI subsystem frees the controller and any subsystem allocated
> > driver data as part of deregistration (unless the allocation is device
> > managed).
> >
> > Take another reference before deregistering the controller so that the
> > driver data is not freed until the driver is done with it.
> 
> Would re-ordering the spi_imx_remove() function be an alternative fix?
> I.e. call spi_unregister_controller() last?

No, the controller needs to be deregistered before disabling clocks and
releasing other resources.

Johan

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

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Marc Kleine-Budde]

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

On 23.03.2026 12:20:08, Johan Hovold wrote:
> On Mon, Mar 23, 2026 at 12:00:59PM +0100, Marc Kleine-Budde wrote:
> > On 23.03.2026 11:49:44, Johan Hovold wrote:
> > > The SPI subsystem frees the controller and any subsystem allocated
> > > driver data as part of deregistration (unless the allocation is device
> > > managed).
> > >
> > > Take another reference before deregistering the controller so that the
> > > driver data is not freed until the driver is done with it.
> >
> > Would re-ordering the spi_imx_remove() function be an alternative fix?
> > I.e. call spi_unregister_controller() last?
>
> No, the controller needs to be deregistered before disabling clocks and
> releasing other resources.

I see. So the API is a bit strange to use:

Allocate with spi_alloc_host(), free with spi_controller_put() before
spi_register_controller(), the free with spi_unregister_controller()
afterwards.

But spi_unregister_controller() shuts down the SPI interface _and_ frees
the memory. Which is the culprit here.

Would using devm_spi_alloc_host() be an option here?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 12:57:42PM +0100, Marc Kleine-Budde wrote:
> On 23.03.2026 12:20:08, Johan Hovold wrote:
> > On Mon, Mar 23, 2026 at 12:00:59PM +0100, Marc Kleine-Budde wrote:
> > > On 23.03.2026 11:49:44, Johan Hovold wrote:
> > > > The SPI subsystem frees the controller and any subsystem allocated
> > > > driver data as part of deregistration (unless the allocation is device
> > > > managed).
> > > >
> > > > Take another reference before deregistering the controller so that the
> > > > driver data is not freed until the driver is done with it.
> > >
> > > Would re-ordering the spi_imx_remove() function be an alternative fix?
> > > I.e. call spi_unregister_controller() last?
> >
> > No, the controller needs to be deregistered before disabling clocks and
> > releasing other resources.
> 
> I see. So the API is a bit strange to use:
> 
> Allocate with spi_alloc_host(), free with spi_controller_put() before
> spi_register_controller(), the free with spi_unregister_controller()
> afterwards.
>
> But spi_unregister_controller() shuts down the SPI interface _and_ frees
> the memory. Which is the culprit here.

Indeed, it's a known issue with the SPI API. See for example:

	68b892f1fdc4 ("spi: document odd controller reference handling")
	5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
	f0c35a024cce ("spi: fix misleading controller deregistration kernel-doc")

> Would using devm_spi_alloc_host() be an option here?

It can also be used, but that's more intrusive so I did that as a
follow-on cleanup to the fix (see patch 2/5).

Johan

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

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Marc Kleine-Budde]

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

On 23.03.2026 14:59:49, Johan Hovold wrote:
> On Mon, Mar 23, 2026 at 12:57:42PM +0100, Marc Kleine-Budde wrote:
> > On 23.03.2026 12:20:08, Johan Hovold wrote:
> > > On Mon, Mar 23, 2026 at 12:00:59PM +0100, Marc Kleine-Budde wrote:
> > > > On 23.03.2026 11:49:44, Johan Hovold wrote:
> > > > > The SPI subsystem frees the controller and any subsystem allocated
> > > > > driver data as part of deregistration (unless the allocation is device
> > > > > managed).
> > > > >
> > > > > Take another reference before deregistering the controller so that the
> > > > > driver data is not freed until the driver is done with it.
> > > >
> > > > Would re-ordering the spi_imx_remove() function be an alternative fix?
> > > > I.e. call spi_unregister_controller() last?
> > >
> > > No, the controller needs to be deregistered before disabling clocks and
> > > releasing other resources.
> >
> > I see. So the API is a bit strange to use:
> >
> > Allocate with spi_alloc_host(), free with spi_controller_put() before
> > spi_register_controller(), the free with spi_unregister_controller()
> > afterwards.
> >
> > But spi_unregister_controller() shuts down the SPI interface _and_ frees
> > the memory. Which is the culprit here.
>
> Indeed, it's a known issue with the SPI API. See for example:
>
> 	68b892f1fdc4 ("spi: document odd controller reference handling")
> 	5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")
> 	f0c35a024cce ("spi: fix misleading controller deregistration kernel-doc")
>
> > Would using devm_spi_alloc_host() be an option here?
>
> It can also be used, but that's more intrusive so I did that as a
> follow-on cleanup to the fix (see patch 2/5).

Ah, nice! At the time I replied to the patch, the whole series was not
available on lore, yet.

Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

And thanks for taking the time in explaining me the details :)

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/5] spi: imx: fix use-after-free on unbind

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 03:47:45PM +0100, Marc Kleine-Budde wrote:
> On 23.03.2026 14:59:49, Johan Hovold wrote:

> > Indeed, it's a known issue with the SPI API. See for example:
> >
> > 	68b892f1fdc4 ("spi: document odd controller reference handling")
> > 	5e844cc37a5c ("spi: Introduce device-managed SPI controller allocation")

> > 	f0c35a024cce ("spi: fix misleading controller deregistration kernel-doc")

This was supposed to say

	3f174274d224 ("spi: fix misleading controller deregistration kernel-doc")

> > > Would using devm_spi_alloc_host() be an option here?
> >
> > It can also be used, but that's more intrusive so I did that as a
> > follow-on cleanup to the fix (see patch 2/5).
> 
> Ah, nice! At the time I replied to the patch, the whole series was not
> available on lore, yet.

Sorry, I should have CC:ed you the whole series.

> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> And thanks for taking the time in explaining me the details :)

No worries. :)

Johan

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

Re: [PATCH 4/5] spi: rockchip: fix controller deregistration

[Mark Brown]

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

On Mon, Mar 23, 2026 at 11:49:47AM +0100, Johan Hovold wrote:
> Make sure to deregister the controller before freeing underlying
> resources like DMA channels during driver unbind.

Fixes should go before any non-fix patches in the series to avoid
spurious dependencies and make it easier to manage applying the series.

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

Re: [PATCH 4/5] spi: rockchip: fix controller deregistration

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 04:35:39PM +0000, Mark Brown wrote:
> On Mon, Mar 23, 2026 at 11:49:47AM +0100, Johan Hovold wrote:
> > Make sure to deregister the controller before freeing underlying
> > resources like DMA channels during driver unbind.
> 
> Fixes should go before any non-fix patches in the series to avoid
> spurious dependencies and make it easier to manage applying the series.

Sure, but here there are no inter-driver dependencies (that is, the sets
of patches for each driver respectively are independent).

And I didn't expect this one to go into 7.0-rc (no stable tag).

Do you want me to resend a reordered series?

Johan

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

Re: [PATCH 4/5] spi: rockchip: fix controller deregistration

[Mark Brown]

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

On Mon, Mar 23, 2026 at 05:51:20PM +0100, Johan Hovold wrote:
> On Mon, Mar 23, 2026 at 04:35:39PM +0000, Mark Brown wrote:

> > Fixes should go before any non-fix patches in the series to avoid
> > spurious dependencies and make it easier to manage applying the series.

> Sure, but here there are no inter-driver dependencies (that is, the sets
> of patches for each driver respectively are independent).

If you're treating the patches as independent then send a series per
driver.

> And I didn't expect this one to go into 7.0-rc (no stable tag).

Why wouldn't we apply bug fixes as bug fixes?

> Do you want me to resend a reordered series?

Please.

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

Re: [PATCH 4/5] spi: rockchip: fix controller deregistration

[Johan Hovold]

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

On Mon, Mar 23, 2026 at 05:02:49PM +0000, Mark Brown wrote:
> On Mon, Mar 23, 2026 at 05:51:20PM +0100, Johan Hovold wrote:
> > On Mon, Mar 23, 2026 at 04:35:39PM +0000, Mark Brown wrote:
> 
> > > Fixes should go before any non-fix patches in the series to avoid
> > > spurious dependencies and make it easier to manage applying the series.
> 
> > Sure, but here there are no inter-driver dependencies (that is, the sets
> > of patches for each driver respectively are independent).
> 
> If you're treating the patches as independent then send a series per
> driver.

I sent these as a series as the changes are related, but sure, it is
possible to split it in three series as well.

> > And I didn't expect this one to go into 7.0-rc (no stable tag).
> 
> Why wouldn't we apply bug fixes as bug fixes?

Not all bug fixes meet the stable backport criteria, and late in the
release cycle we tend to defer non-critical fixes to the next cycle.

This one is border line, but I'll include a stable tag when respinning.
 
> > Do you want me to resend a reordered series?
> 
> Please.

Will do.

Johan

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