| Message ID | 232281df1ab91d8f0f553a62d5f97fc264ace4da.1604874488.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8853b2503014aca5c793d2c7f0aabc990b32bdad |
| Headers | show |
| Series | spi: synquacer: Disable clock in probe error path | expand |
On Mon, Nov 9, 2020 at 12:52 AM Lukas Wunner <lukas@wunner.de> wrote: > > If the calls to platform_get_irq() or devm_request_irq() fail on probe > of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not > unprepared and disabled. > > If the clock rate "master->max_speed_hz" cannot be determined, the same > happens and in addition the spi_master struct is not freed. Wouldn't be better to switch over devm_add_action_or_reset() in such cases?
On Mon, Nov 09, 2020 at 04:22:38PM +0200, Andy Shevchenko wrote: > On Mon, Nov 9, 2020 at 12:52 AM Lukas Wunner <lukas@wunner.de> wrote: > > If the calls to platform_get_irq() or devm_request_irq() fail on probe > > of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not > > unprepared and disabled. > > > > If the clock rate "master->max_speed_hz" cannot be determined, the same > > happens and in addition the spi_master struct is not freed. > > Wouldn't be better to switch over devm_add_action_or_reset() in such cases? I'd rather prefer a devm_clk_prepare_enable(). This is common enough to merit a function of its own. As for the spi_master struct being leaked: I'm about to submit a series which introduces devm_spi_alloc_master/slave() and uses that to fix a use-after-free in 9 drivers and an spi_master leak in 8 drivers. Patches are on this development branch: https://github.com/l1k/linux/commits/spi_fixes I'm too busy with this series to also look into adding a devm_clk_prepare_enable(). Hopefully someone else can do that. Thanks, Lukas
On Sun, 8 Nov 2020 23:41:00 +0100, Lukas Wunner wrote: > If the calls to platform_get_irq() or devm_request_irq() fail on probe > of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not > unprepared and disabled. > > If the clock rate "master->max_speed_hz" cannot be determined, the same > happens and in addition the spi_master struct is not freed. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: synquacer: Disable clock in probe error path commit: 8853b2503014aca5c793d2c7f0aabc990b32bdad 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
diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c index 42e82dbe3d41..8cdca6ab8098 100644 --- a/drivers/spi/spi-synquacer.c +++ b/drivers/spi/spi-synquacer.c @@ -657,7 +657,8 @@ static int synquacer_spi_probe(struct platform_device *pdev) if (!master->max_speed_hz) { dev_err(&pdev->dev, "missing clock source\n"); - return -EINVAL; + ret = -EINVAL; + goto disable_clk; } master->min_speed_hz = master->max_speed_hz / 254; @@ -670,7 +671,7 @@ static int synquacer_spi_probe(struct platform_device *pdev) rx_irq = platform_get_irq(pdev, 0); if (rx_irq <= 0) { ret = rx_irq; - goto put_spi; + goto disable_clk; } snprintf(sspi->rx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-rx", dev_name(&pdev->dev)); @@ -678,13 +679,13 @@ static int synquacer_spi_probe(struct platform_device *pdev) 0, sspi->rx_irq_name, sspi); if (ret) { dev_err(&pdev->dev, "request rx_irq failed (%d)\n", ret); - goto put_spi; + goto disable_clk; } tx_irq = platform_get_irq(pdev, 1); if (tx_irq <= 0) { ret = tx_irq; - goto put_spi; + goto disable_clk; } snprintf(sspi->tx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-tx", dev_name(&pdev->dev)); @@ -692,7 +693,7 @@ static int synquacer_spi_probe(struct platform_device *pdev) 0, sspi->tx_irq_name, sspi); if (ret) { dev_err(&pdev->dev, "request tx_irq failed (%d)\n", ret); - goto put_spi; + goto disable_clk; } master->dev.of_node = np; @@ -710,7 +711,7 @@ static int synquacer_spi_probe(struct platform_device *pdev) ret = synquacer_spi_enable(master); if (ret) - goto fail_enable; + goto disable_clk; pm_runtime_set_active(sspi->dev); pm_runtime_enable(sspi->dev); @@ -723,7 +724,7 @@ static int synquacer_spi_probe(struct platform_device *pdev) disable_pm: pm_runtime_disable(sspi->dev); -fail_enable: +disable_clk: clk_disable_unprepare(sspi->clk); put_spi: spi_master_put(master);
If the calls to platform_get_irq() or devm_request_irq() fail on probe of the SynQuacer SPI driver, the clock "sspi->clk" is erroneously not unprepared and disabled. If the clock rate "master->max_speed_hz" cannot be determined, the same happens and in addition the spi_master struct is not freed. Fix it. Fixes: b0823ee35cf9 ("spi: Add spi driver for Socionext SynQuacer platform") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: <stable@vger.kernel.org> # v5.3+ Cc: Masahisa Kojima <masahisa.kojima@linaro.org> --- drivers/spi/spi-synquacer.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)