[ Upstream commit 5e844cc37a5cbaa460e68f9a989d321d63088a89 ]
SPI driver probing currently comprises two steps, whereas removal comprises only one step:
spi_alloc_master() spi_register_controller()
spi_unregister_controller()
That's because spi_unregister_controller() calls device_unregister() instead of device_del(), thereby releasing the reference on the spi_controller which was obtained by spi_alloc_master().
An SPI driver's private data is contained in the same memory allocation as the spi_controller struct. Thus, once spi_unregister_controller() has been called, the private data is inaccessible. But some drivers need to access it after spi_unregister_controller() to perform further teardown steps.
Introduce devm_spi_alloc_master() and devm_spi_alloc_slave(), which release a reference on the spi_controller struct only after the driver has unbound, thereby keeping the memory allocation accessible. Change spi_unregister_controller() to not release a reference if the spi_controller was allocated by one of these new devm functions.
The present commit is small enough to be backportable to stable. It allows fixing drivers which use the private data in their ->remove() hook after it's been freed. It also allows fixing drivers which neglect to release a reference on the spi_controller in the probe error path.
Long-term, most SPI drivers shall be moved over to the devm functions introduced herein. The few that can't shall be changed in a treewide commit to explicitly release the last reference on the controller. That commit shall amend spi_unregister_controller() to no longer release a reference, thereby completing the migration.
As a result, the behaviour will be less surprising and more consistent with subsystems such as IIO, which also includes the private data in the allocation of the generic iio_dev struct, but calls device_del() in iio_device_unregister().
Signed-off-by: Lukas Wunner lukas@wunner.de Link: https://lore.kernel.org/r/272bae2ef08abd21388c98e23729886663d19192.160512103... Signed-off-by: Mark Brown broonie@kernel.org --- drivers/spi/spi.c | 58 ++++++++++++++++++++++++++++++++++++++++- include/linux/spi/spi.h | 19 ++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index acc8eeed73f0..ca9970a63fdf 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -2043,6 +2043,49 @@ struct spi_controller *__spi_alloc_controller(struct device *dev, } EXPORT_SYMBOL_GPL(__spi_alloc_controller);
+static void devm_spi_release_controller(struct device *dev, void *ctlr) +{ + spi_controller_put(*(struct spi_controller **)ctlr); +} + +/** + * __devm_spi_alloc_controller - resource-managed __spi_alloc_controller() + * @dev: physical device of SPI controller + * @size: how much zeroed driver-private data to allocate + * @slave: whether to allocate an SPI master (false) or SPI slave (true) + * Context: can sleep + * + * Allocate an SPI controller and automatically release a reference on it + * when @dev is unbound from its driver. Drivers are thus relieved from + * having to call spi_controller_put(). + * + * The arguments to this function are identical to __spi_alloc_controller(). + * + * Return: the SPI controller structure on success, else NULL. + */ +struct spi_controller *__devm_spi_alloc_controller(struct device *dev, + unsigned int size, + bool slave) +{ + struct spi_controller **ptr, *ctlr; + + ptr = devres_alloc(devm_spi_release_controller, sizeof(*ptr), + GFP_KERNEL); + if (!ptr) + return NULL; + + ctlr = __spi_alloc_controller(dev, size, slave); + if (ctlr) { + *ptr = ctlr; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return ctlr; +} +EXPORT_SYMBOL_GPL(__devm_spi_alloc_controller); + #ifdef CONFIG_OF static int of_spi_register_master(struct spi_controller *ctlr) { @@ -2261,6 +2304,11 @@ int devm_spi_register_controller(struct device *dev, } EXPORT_SYMBOL_GPL(devm_spi_register_controller);
+static int devm_spi_match_controller(struct device *dev, void *res, void *ctlr) +{ + return *(struct spi_controller **)res == ctlr; +} + static int __unregister(struct device *dev, void *null) { spi_unregister_device(to_spi_device(dev)); @@ -2300,7 +2348,15 @@ void spi_unregister_controller(struct spi_controller *ctlr) list_del(&ctlr->list); mutex_unlock(&board_lock);
- device_unregister(&ctlr->dev); + device_del(&ctlr->dev); + + /* Release the last reference on the controller if its driver + * has not yet been converted to devm_spi_alloc_master/slave(). + */ + if (!devres_find(ctlr->dev.parent, devm_spi_release_controller, + devm_spi_match_controller, ctlr)) + put_device(&ctlr->dev); + /* free bus id */ mutex_lock(&board_lock); if (found == ctlr) diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 7b2170bfd6e7..715bd276a041 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -638,6 +638,25 @@ static inline struct spi_controller *spi_alloc_slave(struct device *host, return __spi_alloc_controller(host, size, true); }
+struct spi_controller *__devm_spi_alloc_controller(struct device *dev, + unsigned int size, + bool slave); + +static inline struct spi_controller *devm_spi_alloc_master(struct device *dev, + unsigned int size) +{ + return __devm_spi_alloc_controller(dev, size, false); +} + +static inline struct spi_controller *devm_spi_alloc_slave(struct device *dev, + unsigned int size) +{ + if (!IS_ENABLED(CONFIG_SPI_SLAVE)) + return NULL; + + return __devm_spi_alloc_controller(dev, size, true); +} + extern int spi_register_controller(struct spi_controller *ctlr); extern int devm_spi_register_controller(struct device *dev, struct spi_controller *ctlr);
commit 63c5395bb7a9777a33f0e7b5906f2c0170a23692 upstream
bcm_qspi_remove() calls spi_unregister_master() even though bcm_qspi_probe() calls devm_spi_register_master(). The spi_master is therefore unregistered and freed twice on unbind.
Moreover, since commit 0392727c261b ("spi: bcm-qspi: Handle clock probe deferral"), bcm_qspi_probe() leaks the spi_master allocation if the call to devm_clk_get_optional() fails.
Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound and also avoids the spi_master leak on probe.
While at it, fix an ordering issue in bcm_qspi_remove() wherein spi_unregister_master() is called after uninitializing the hardware, disabling the clock and freeing an IRQ data structure. The correct order is to call spi_unregister_master() *before* those teardown steps because bus accesses may still be ongoing until that function returns.
Fixes: fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver") Signed-off-by: Lukas Wunner lukas@wunner.de Cc: stable@vger.kernel.org # v4.9+: 123456789abc: spi: Introduce device-managed SPI controller allocation Cc: stable@vger.kernel.org # v4.9+ Cc: Kamal Dasu kdasu.kdev@gmail.com Acked-by: Florian Fainelli f.fainelli@gmail.com Tested-by: Florian Fainelli f.fainelli@gmail.com Link: https://lore.kernel.org/r/5e31a9a59fd1c0d0b795b2fe219f25e5ee855f9d.160512103... Signed-off-by: Mark Brown broonie@kernel.org [sudip: adjust context] Signed-off-by: Sudip Mukherjee sudipm.mukherjee@gmail.com --- drivers/spi/spi-bcm-qspi.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c index 7c86a8ee03aa..11adc2c13a74 100644 --- a/drivers/spi/spi-bcm-qspi.c +++ b/drivers/spi/spi-bcm-qspi.c @@ -1219,7 +1219,7 @@ int bcm_qspi_probe(struct platform_device *pdev, if (!of_match_node(bcm_qspi_of_match, dev->of_node)) return -ENODEV;
- master = spi_alloc_master(dev, sizeof(struct bcm_qspi)); + master = devm_spi_alloc_master(dev, sizeof(struct bcm_qspi)); if (!master) { dev_err(dev, "error allocating spi_master\n"); return -ENOMEM; @@ -1253,21 +1253,17 @@ int bcm_qspi_probe(struct platform_device *pdev,
if (res) { qspi->base[MSPI] = devm_ioremap_resource(dev, res); - if (IS_ERR(qspi->base[MSPI])) { - ret = PTR_ERR(qspi->base[MSPI]); - goto qspi_resource_err; - } + if (IS_ERR(qspi->base[MSPI])) + return PTR_ERR(qspi->base[MSPI]); } else { - goto qspi_resource_err; + return 0; }
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi"); if (res) { qspi->base[BSPI] = devm_ioremap_resource(dev, res); - if (IS_ERR(qspi->base[BSPI])) { - ret = PTR_ERR(qspi->base[BSPI]); - goto qspi_resource_err; - } + if (IS_ERR(qspi->base[BSPI])) + return PTR_ERR(qspi->base[BSPI]); qspi->bspi_mode = true; } else { qspi->bspi_mode = false; @@ -1278,18 +1274,14 @@ int bcm_qspi_probe(struct platform_device *pdev, res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cs_reg"); if (res) { qspi->base[CHIP_SELECT] = devm_ioremap_resource(dev, res); - if (IS_ERR(qspi->base[CHIP_SELECT])) { - ret = PTR_ERR(qspi->base[CHIP_SELECT]); - goto qspi_resource_err; - } + if (IS_ERR(qspi->base[CHIP_SELECT])) + return PTR_ERR(qspi->base[CHIP_SELECT]); }
qspi->dev_ids = kcalloc(num_irqs, sizeof(struct bcm_qspi_dev_id), GFP_KERNEL); - if (!qspi->dev_ids) { - ret = -ENOMEM; - goto qspi_resource_err; - } + if (!qspi->dev_ids) + return -ENOMEM;
for (val = 0; val < num_irqs; val++) { irq = -1; @@ -1365,7 +1357,7 @@ int bcm_qspi_probe(struct platform_device *pdev, qspi->xfer_mode.addrlen = -1; qspi->xfer_mode.hp = -1;
- ret = devm_spi_register_master(&pdev->dev, master); + ret = spi_register_master(master); if (ret < 0) { dev_err(dev, "can't register master\n"); goto qspi_reg_err; @@ -1378,8 +1370,6 @@ int bcm_qspi_probe(struct platform_device *pdev, clk_disable_unprepare(qspi->clk); qspi_probe_err: kfree(qspi->dev_ids); -qspi_resource_err: - spi_master_put(master); return ret; } /* probe function to be called by SoC specific platform driver probe */ @@ -1389,10 +1379,10 @@ int bcm_qspi_remove(struct platform_device *pdev) { struct bcm_qspi *qspi = platform_get_drvdata(pdev);
+ spi_unregister_master(qspi->master); bcm_qspi_hw_uninit(qspi); clk_disable_unprepare(qspi->clk); kfree(qspi->dev_ids); - spi_unregister_master(qspi->master);
return 0; }
[ Upstream commit e1483ac030fb4c57734289742f1c1d38dca61e22 ]
bcm2835_spi_remove() accesses the driver's private data after calling spi_unregister_controller() even though that function releases the last reference on the spi_controller and thereby frees the private data.
Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound.
Fixes: f8043872e796 ("spi: add driver for BCM2835") Reported-by: Sascha Hauer s.hauer@pengutronix.de Reported-by: Florian Fainelli f.fainelli@gmail.com Signed-off-by: Lukas Wunner lukas@wunner.de Cc: stable@vger.kernel.org # v3.10+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: stable@vger.kernel.org # v3.10+ Cc: Vladimir Oltean olteanv@gmail.com Tested-by: Florian Fainelli f.fainelli@gmail.com Acked-by: Florian Fainelli f.fainelli@gmail.com Link: https://lore.kernel.org/r/ad66e0a0ad96feb848814842ecf5b6a4539ef35c.160512103... Signed-off-by: Mark Brown broonie@kernel.org --- drivers/spi/spi-bcm2835.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index df6abc75bc16..2908df35466f 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -737,7 +737,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev) struct resource *res; int err;
- master = spi_alloc_master(&pdev->dev, sizeof(*bs)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(*bs)); if (!master) { dev_err(&pdev->dev, "spi_alloc_master() failed\n"); return -ENOMEM; @@ -759,23 +759,20 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); bs->regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(bs->regs)) { - err = PTR_ERR(bs->regs); - goto out_master_put; - } + if (IS_ERR(bs->regs)) + return PTR_ERR(bs->regs);
bs->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(bs->clk)) { err = PTR_ERR(bs->clk); dev_err(&pdev->dev, "could not get clk: %d\n", err); - goto out_master_put; + return err; }
bs->irq = platform_get_irq(pdev, 0); if (bs->irq <= 0) { dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq); - err = bs->irq ? bs->irq : -ENODEV; - goto out_master_put; + return bs->irq ? bs->irq : -ENODEV; }
clk_prepare_enable(bs->clk); @@ -803,8 +800,6 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
out_clk_disable: clk_disable_unprepare(bs->clk); -out_master_put: - spi_master_put(master); return err; }
[ Upstream commit e13ee6cc4781edaf8c7321bee19217e3702ed481 ]
bcm2835aux_spi_remove() accesses the driver's private data after calling spi_unregister_master() even though that function releases the last reference on the spi_master and thereby frees the private data.
Fix by switching over to the new devm_spi_alloc_master() helper which keeps the private data accessible until the driver has unbound.
Fixes: b9dd3f6d4172 ("spi: bcm2835aux: Fix controller unregister order") Signed-off-by: Lukas Wunner lukas@wunner.de Cc: stable@vger.kernel.org # v4.4+: 5e844cc37a5c: spi: Introduce device-managed SPI controller allocation Cc: stable@vger.kernel.org # v4.4+: b9dd3f6d4172: spi: bcm2835aux: Fix controller unregister order Cc: stable@vger.kernel.org # v4.4+ Link: https://lore.kernel.org/r/b290b06357d0c0bdee9cecc539b840a90630f101.160512103... Signed-off-by: Mark Brown broonie@kernel.org --- drivers/spi/spi-bcm2835aux.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c index b7f78e6d9bec..88772efda830 100644 --- a/drivers/spi/spi-bcm2835aux.c +++ b/drivers/spi/spi-bcm2835aux.c @@ -407,7 +407,7 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) unsigned long clk_hz; int err;
- master = spi_alloc_master(&pdev->dev, sizeof(*bs)); + master = devm_spi_alloc_master(&pdev->dev, sizeof(*bs)); if (!master) { dev_err(&pdev->dev, "spi_alloc_master() failed\n"); return -ENOMEM; @@ -439,30 +439,27 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) /* the main area */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); bs->regs = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(bs->regs)) { - err = PTR_ERR(bs->regs); - goto out_master_put; - } + if (IS_ERR(bs->regs)) + return PTR_ERR(bs->regs);
bs->clk = devm_clk_get(&pdev->dev, NULL); if ((!bs->clk) || (IS_ERR(bs->clk))) { err = PTR_ERR(bs->clk); dev_err(&pdev->dev, "could not get clk: %d\n", err); - goto out_master_put; + return err; }
bs->irq = platform_get_irq(pdev, 0); if (bs->irq <= 0) { dev_err(&pdev->dev, "could not get IRQ: %d\n", bs->irq); - err = bs->irq ? bs->irq : -ENODEV; - goto out_master_put; + return bs->irq ? bs->irq : -ENODEV; }
/* this also enables the HW block */ err = clk_prepare_enable(bs->clk); if (err) { dev_err(&pdev->dev, "could not prepare clock: %d\n", err); - goto out_master_put; + return err; }
/* just checking if the clock returns a sane value */ @@ -495,8 +492,6 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev)
out_clk_disable: clk_disable_unprepare(bs->clk); -out_master_put: - spi_master_put(master); return err; }
From: Peter Ujfalusi peter.ujfalusi@ti.com
[ Upstream commit 666224b43b4bd4612ce3b758c038f9bc5c5e3fcb ]
The DMA channel was not released if either devm_request_irq() or devm_spi_register_controller() failed.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Reviewed-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Link: https://lore.kernel.org/r/20191212135550.4634-3-peter.ujfalusi@ti.com Signed-off-by: Mark Brown broonie@kernel.org [lukas: backport to 4.19-stable] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/spi/spi-bcm2835.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index 2908df35466f..6824beae18e4 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -787,18 +787,19 @@ static int bcm2835_spi_probe(struct platform_device *pdev) dev_name(&pdev->dev), master); if (err) { dev_err(&pdev->dev, "could not request IRQ: %d\n", err); - goto out_clk_disable; + goto out_dma_release; }
err = spi_register_master(master); if (err) { dev_err(&pdev->dev, "could not register SPI master: %d\n", err); - goto out_clk_disable; + goto out_dma_release; }
return 0;
-out_clk_disable: +out_dma_release: + bcm2835_dma_release(master); clk_disable_unprepare(bs->clk); return err; }
linux-stable-mirror@lists.linaro.org