Skip to contentSkip to conversation

spi: fix explicit controller deregistration

PATCH In review Johan Hovold 2 participants 10 messages Apr 14 – Apr 20

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover spi: fix explicit controller deregistration

Turns out we have a few drivers that get the tear down ordering wrong also when not using device managed registration (cf. [1] and [2]).

Fix this to avoid issues like system errors due to unclocked accesses, NULL-pointer dereferences, hangs or failed I/O during during deregistration (e.g. when powering down devices).

Johan

[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/

Johan Hovold (8):
spi: cadence: fix controller deregistration
spi: cadence-quadspi: fix controller deregistration
spi: mpc52xx: fix controller deregistration
spi: mpc52xx: fix use-after-free on unbind
spi: mxic: fix controller deregistration
spi: orion: fix controller deregistration
spi: topcliff-pch: fix controller deregistration
spi: topcliff-pch: fix use-after-free on unbind

 drivers/spi/spi-cadence-quadspi.c |  4 ++--
 drivers/spi/spi-cadence.c         |  6 +++++-
 drivers/spi/spi-mpc52xx.c         |  6 ++++--
 drivers/spi/spi-mxic.c            |  3 ++-
 drivers/spi/spi-orion.c           |  7 ++++++-
 drivers/spi/spi-topcliff-pch.c    | 11 ++++++++---
 6 files changed, 27 insertions(+), 10 deletions(-)
Signature · 1 line
2.52.0
#1 lore raw

PATCH 1/8 spi: cadence: fix controller deregistration

Make sure to deregister the controller before disabling underlying resources like clocks during driver unbind.

Fixes c474b3866546 ("spi: Add driver for Cadence SPI controller")
Cc Harini Katakam <[email protected]>
---
 drivers/spi/spi-cadence.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/spi/spi-cadence.c +5 -1
index caa7a57e6d27..08d7dabe818d 100644
@@ -777,6 +777,10 @@ static void cdns_spi_remove(struct platform_device *pdev)
 	struct spi_controller *ctlr = platform_get_drvdata(pdev);
 	struct cdns_spi *xspi = spi_controller_get_devdata(ctlr);
 
+	spi_controller_get(ctlr);
+
+	spi_unregister_controller(ctlr);
+
 	cdns_spi_write(xspi, CDNS_SPI_ER, CDNS_SPI_ER_DISABLE);
 
 	if (!spi_controller_is_target(ctlr)) {
@@ -784,7 +788,7 @@ static void cdns_spi_remove(struct platform_device *pdev)
 		pm_runtime_set_suspended(&pdev->dev);
 	}
 
-	spi_unregister_controller(ctlr);
+	spi_controller_put(ctlr);
 }
 
 /**
Signature · 1 line
2.52.0
#2 lore raw

PATCH 2/8 spi: cadence-quadspi: fix controller deregistration

In reply to Johan Hovold

Make sure to deregister the controller before dropping the reference count that allows new operations to start to allow SPI drivers to do I/O during deregistration.

Fixes 7446284023e8 ("spi: cadence-quadspi: Implement refcount to handle unbind during busy")
Cc Khairul Anuar Romli <[email protected]>
---
 drivers/spi/spi-cadence-quadspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/spi/spi-cadence-quadspi.c +2 -2
index 2ead419e896e..50ef65fc5ded 100644
@@ -2020,13 +2020,13 @@ static void cqspi_remove(struct platform_device *pdev)
 
 	ddata = of_device_get_match_data(dev);
 
+	spi_unregister_controller(cqspi->host);
+
 	refcount_set(&cqspi->refcount, 0);
 
 	if (!refcount_dec_and_test(&cqspi->inflight_ops))
 		cqspi_wait_idle(cqspi);
 
-	spi_unregister_controller(cqspi->host);
-
 	if (cqspi->rx_chan)
 		dma_release_channel(cqspi->rx_chan);
 
Signature · 1 line
2.52.0
#3 lore raw

PATCH 3/8 spi: mpc52xx: fix controller deregistration

In reply to Johan Hovold

Make sure to deregister the controller before disabling and releasing underlying resources like interrupts and gpios during driver unbind.

Fixes 42bbb70980f3 ("powerpc/5200: Add mpc5200-spi (non-PSC) device driver")
Fixes b8d4e2ce60b6 ("mpc52xx_spi: add gpio chipselect")
Cc [email protected] # 2.6.33
Cc Grant Likely <[email protected]>
Cc Luotao Fu <[email protected]>
---
 drivers/spi/spi-mpc52xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/spi/spi-mpc52xx.c +2 -1
index 05bbd3795e7d..823b49f8ece2 100644
@@ -517,6 +517,8 @@ static void mpc52xx_spi_remove(struct platform_device *op)
 	struct mpc52xx_spi *ms = spi_controller_get_devdata(host);
 	int i;
 
+	spi_unregister_controller(host);
+
 	cancel_work_sync(&ms->work);
 	free_irq(ms->irq0, ms);
 	free_irq(ms->irq1, ms);
@@ -525,7 +527,6 @@ static void mpc52xx_spi_remove(struct platform_device *op)
 		gpiod_put(ms->gpio_cs[i]);
 
 	kfree(ms->gpio_cs);
-	spi_unregister_controller(host);
 	iounmap(ms->regs);
 	spi_controller_put(host);
 }
Signature · 1 line
2.52.0
#4 lore raw

PATCH 4/8 spi: mpc52xx: fix use-after-free on unbind

In reply to Johan Hovold

The state machine work is scheduled by the interrupt handler and therefore needs to be cancelled after disabling interrupts to avoid a potential use-after-free.

Fixes 984836621aad ("spi: mpc52xx: Add cancel_work_sync before module remove")
Cc Pei Xiao <[email protected]>
---
 drivers/spi/spi-mpc52xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/spi/spi-mpc52xx.c +2 -1
index 823b49f8ece2..c8c8e6bdf421 100644
@@ -519,10 +519,11 @@ static void mpc52xx_spi_remove(struct platform_device *op)
 
 	spi_unregister_controller(host);
 
-	cancel_work_sync(&ms->work);
 	free_irq(ms->irq0, ms);
 	free_irq(ms->irq1, ms);
 
+	cancel_work_sync(&ms->work);
+
 	for (i = 0; i < ms->gpio_cs_count; i++)
 		gpiod_put(ms->gpio_cs[i]);
 
Signature · 1 line
2.52.0
#5 lore raw

PATCH 5/8 spi: mxic: fix controller deregistration

In reply to Johan Hovold

Make sure to deregister the controller before disabling underlying resources like clocks (via runtime pm) during driver unbind.

Fixes b942d80b0a39 ("spi: Add MXIC controller driver")
Cc [email protected] # 5.0: cc53711b2191
Cc Mason Yang <[email protected]>
---
 drivers/spi/spi-mxic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/spi/spi-mxic.c +2 -1
index f9369c69911c..b0e7fc828a50 100644
@@ -832,9 +832,10 @@ static void mxic_spi_remove(struct platform_device *pdev)
 	struct spi_controller *host = platform_get_drvdata(pdev);
 	struct mxic_spi *mxic = spi_controller_get_devdata(host);
 
+	spi_unregister_controller(host);
+
 	pm_runtime_disable(&pdev->dev);
 	mxic_spi_mem_ecc_remove(mxic);
-	spi_unregister_controller(host);
 }
 
 static const struct of_device_id mxic_spi_of_ids[] = {
Signature · 1 line
2.52.0
#6 lore raw

PATCH 6/8 spi: orion: fix controller deregistration

In reply to Johan Hovold

Make sure to deregister the controller before disabling underlying resources like clocks during driver unbind.

Fixes 60cadec9da7b ("spi: new orion_spi driver")
Cc [email protected] # 2.6.27
---
 drivers/spi/spi-orion.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/spi/spi-orion.c +6 -1
index 7a2186b51b4c..c54cd4ef09bd 100644
@@ -801,10 +801,15 @@ static void orion_spi_remove(struct platform_device *pdev)
 	struct spi_controller *host = platform_get_drvdata(pdev);
 	struct orion_spi *spi = spi_controller_get_devdata(host);
 
+	spi_controller_get(host);
+
+	spi_unregister_controller(host);
+
 	pm_runtime_get_sync(&pdev->dev);
 	clk_disable_unprepare(spi->axi_clk);
 
-	spi_unregister_controller(host);
+	spi_controller_put(host);
+
 	pm_runtime_disable(&pdev->dev);
 }
 
Signature · 1 line
2.52.0
#7 lore raw

PATCH 7/8 spi: topcliff-pch: fix controller deregistration

In reply to Johan Hovold

Make sure to deregister the controller before disabling and releasing underlying resources like interrupts and DMA during driver unbind.

Fixes e8b17b5b3f30 ("spi/topcliff: Add topcliff platform controller hub (PCH) spi bus driver")
Cc [email protected] # 2.6.37
Cc Masayuki Ohtake <[email protected]>
---
 drivers/spi/spi-topcliff-pch.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/spi/spi-topcliff-pch.c +6 -1
index cae2dcefabea..c120436434d0 100644
@@ -1406,6 +1406,10 @@ static void pch_spi_pd_remove(struct platform_device *plat_dev)
 	dev_dbg(&plat_dev->dev, "%s:[ch%d] irq=%d\n",
 		__func__, plat_dev->id, board_dat->pdev->irq);
 
+	spi_controller_get(data->host);
+
+	spi_unregister_controller(data->host);
+
 	if (use_dma)
 		pch_free_dma_buf(board_dat, data);
 
@@ -1433,7 +1437,8 @@ static void pch_spi_pd_remove(struct platform_device *plat_dev)
 	}
 
 	pci_iounmap(board_dat->pdev, data->io_remap_addr);
-	spi_unregister_controller(data->host);
+
+	spi_controller_put(data->host);
 }
 #ifdef CONFIG_PM
 static int pch_spi_pd_suspend(struct platform_device *pd_dev,
Signature · 1 line
2.52.0
#8 lore raw

PATCH 8/8 spi: topcliff-pch: fix use-after-free on unbind

In reply to Johan Hovold

Give the driver a chance to flush its queue before releasing the DMA buffers on driver unbind

Fixes c37f3c2749b5 ("spi/topcliff_pch: DMA support")
Cc Tomoya MORINAGA <[email protected]>
---
 drivers/spi/spi-topcliff-pch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/spi/spi-topcliff-pch.c +3 -3
index c120436434d0..14d11450e86d 100644
@@ -1410,9 +1410,6 @@ static void pch_spi_pd_remove(struct platform_device *plat_dev)
 
 	spi_unregister_controller(data->host);
 
-	if (use_dma)
-		pch_free_dma_buf(board_dat, data);
-
 	/* check for any pending messages; no action is taken if the queue
 	 * is still full; but at least we tried.  Unload anyway */
 	count = 500;
@@ -1436,6 +1433,9 @@ static void pch_spi_pd_remove(struct platform_device *plat_dev)
 		free_irq(board_dat->pdev->irq, data);
 	}
 
+	if (use_dma)
+		pch_free_dma_buf(board_dat, data);
+
 	pci_iounmap(board_dat->pdev, data->io_remap_addr);
 
 	spi_controller_put(data->host);
Signature · 1 line
2.52.0
maintainer
#9 lore raw
In reply to Johan Hovold
Quoted 10 lines · Johan Hovold

spi: fix explicit controller deregistration

Turns out we have a few drivers that get the tear down ordering wrong also when not using device managed registration (cf. [1] and [2]).

Fix this to avoid issues like system errors due to unclocked accesses, NULL-pointer dereferences, hangs or failed I/O during during deregistration (e.g. when powering down devices).

[...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-7.1

Thanks!

[1/8] spi: cadence: fix controller deregistration

      https://git.kernel.org/broonie/spi/c/666fa7e9ca98
[2/8] spi: cadence-quadspi: fix controller deregistration
      https://git.kernel.org/broonie/spi/c/964ee9793760
[3/8] spi: mpc52xx: fix controller deregistration
      https://git.kernel.org/broonie/spi/c/0f997fdae819
[4/8] spi: mpc52xx: fix use-after-free on unbind
      https://git.kernel.org/broonie/spi/c/706b3dc2ac7a
[5/8] spi: mxic: fix controller deregistration
      https://git.kernel.org/broonie/spi/c/adbc595e2720
[6/8] spi: orion: fix controller deregistration
      https://git.kernel.org/broonie/spi/c/220f4f11104a
[7/8] spi: topcliff-pch: fix controller deregistration
      https://git.kernel.org/broonie/spi/c/5d6f477d6fc0
[8/8] spi: topcliff-pch: fix use-after-free on unbind
      https://git.kernel.org/broonie/spi/c/9d72732fe70c

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark