From 2ec6f20b33eb4f62ab90bdcd620436c883ec3af6 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Thu, 27 May 2021 23:10:56 +0200 Subject: spi: Cleanup on failure of initial setup Commit c7299fea6769 ("spi: Fix spi device unregister flow") changed the SPI core's behavior if the ->setup() hook returns an error upon adding an spi_device: Before, the ->cleanup() hook was invoked to free any allocations that were made by ->setup(). With the commit, that's no longer the case, so the ->setup() hook is expected to free the allocations itself. I've identified 5 drivers which depend on the old behavior and am fixing them up hereinafter: spi-bitbang.c spi-fsl-spi.c spi-omap-uwire.c spi-omap2-mcspi.c spi-pxa2xx.c Importantly, ->setup() is not only invoked on spi_device *addition*: It may subsequently be called to *change* SPI parameters. If changing these SPI parameters fails, freeing memory allocations would be wrong. That should only be done if the spi_device is finally destroyed. I am therefore using a bool "initial_setup" in 4 of the affected drivers to differentiate between the invocation on *adding* the spi_device and any subsequent invocations: spi-bitbang.c spi-fsl-spi.c spi-omap-uwire.c spi-omap2-mcspi.c In spi-pxa2xx.c, it seems the ->setup() hook can only fail on spi_device addition, not any subsequent calls. It therefore doesn't need the bool. It's worth noting that 5 other drivers already perform a cleanup if the ->setup() hook fails. Before c7299fea6769, they caused a double-free if ->setup() failed on spi_device addition. Since the commit, they're fine. These drivers are: spi-mpc512x-psc.c spi-pl022.c spi-s3c64xx.c spi-st-ssc4.c spi-tegra114.c (spi-pxa2xx.c also already performs a cleanup, but only in one of several error paths.) Fixes: c7299fea6769 ("spi: Fix spi device unregister flow") Signed-off-by: Lukas Wunner Cc: Saravana Kannan Acked-by: Andy Shevchenko # pxa2xx Link: https://lore.kernel.org/r/f76a0599469f265b69c371538794101fa37b5536.1622149321.git.lukas@wunner.de Signed-off-by: Mark Brown --- drivers/spi/spi-omap2-mcspi.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) (limited to 'drivers/spi/spi-omap2-mcspi.c') diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c index 999c22736416..ede7f05e5ced 100644 --- a/drivers/spi/spi-omap2-mcspi.c +++ b/drivers/spi/spi-omap2-mcspi.c @@ -1032,8 +1032,22 @@ static void omap2_mcspi_release_dma(struct spi_master *master) } } +static void omap2_mcspi_cleanup(struct spi_device *spi) +{ + struct omap2_mcspi_cs *cs; + + if (spi->controller_state) { + /* Unlink controller state from context save list */ + cs = spi->controller_state; + list_del(&cs->node); + + kfree(cs); + } +} + static int omap2_mcspi_setup(struct spi_device *spi) { + bool initial_setup = false; int ret; struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master); struct omap2_mcspi_regs *ctx = &mcspi->ctx; @@ -1051,35 +1065,28 @@ static int omap2_mcspi_setup(struct spi_device *spi) spi->controller_state = cs; /* Link this to context save list */ list_add_tail(&cs->node, &ctx->cs); + initial_setup = true; } ret = pm_runtime_get_sync(mcspi->dev); if (ret < 0) { pm_runtime_put_noidle(mcspi->dev); + if (initial_setup) + omap2_mcspi_cleanup(spi); return ret; } ret = omap2_mcspi_setup_transfer(spi, NULL); + if (ret && initial_setup) + omap2_mcspi_cleanup(spi); + pm_runtime_mark_last_busy(mcspi->dev); pm_runtime_put_autosuspend(mcspi->dev); return ret; } -static void omap2_mcspi_cleanup(struct spi_device *spi) -{ - struct omap2_mcspi_cs *cs; - - if (spi->controller_state) { - /* Unlink controller state from context save list */ - cs = spi->controller_state; - list_del(&cs->node); - - kfree(cs); - } -} - static irqreturn_t omap2_mcspi_irq_handler(int irq, void *data) { struct omap2_mcspi *mcspi = data; -- cgit v1.2.3-70-g09d2