On Mon, Jun 15, 2020 at 12:23:41PM +0300, Vladimir Oltean wrote:
On Mon, 15 Jun 2020 at 11:18, Marc Kleine-Budde mkl@pengutronix.de wrote:
On 6/15/20 10:07 AM, Krzysztof Kozlowski wrote:
If interrupt comes late, during probe error path or device remove (could be triggered with CONFIG_DEBUG_SHIRQ), the interrupt handler dspi_interrupt() will access registers with the clock being disabled. This leads to external abort on non-linefetch on Toradex Colibri VF50 module (with Vybrid VF5xx):
$ echo 4002d000.spi > /sys/devices/platform/soc/40000000.bus/4002d000.spi/driver/unbind Unhandled fault: external abort on non-linefetch (0x1008) at 0x8887f02c Internal error: : 1008 [#1] ARM CPU: 0 PID: 136 Comm: sh Not tainted 5.7.0-next-20200610-00009-g5c913fa0f9c5-dirty #74 Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree) (regmap_mmio_read32le) from [<8061885c>] (regmap_mmio_read+0x48/0x68) (regmap_mmio_read) from [<8060e3b8>] (_regmap_bus_reg_read+0x24/0x28) (_regmap_bus_reg_read) from [<80611c50>] (_regmap_read+0x70/0x1c0) (_regmap_read) from [<80611dec>] (regmap_read+0x4c/0x6c) (regmap_read) from [<80678ca0>] (dspi_interrupt+0x3c/0xa8) (dspi_interrupt) from [<8017acec>] (free_irq+0x26c/0x3cc) (free_irq) from [<8017dcec>] (devm_irq_release+0x1c/0x20) (devm_irq_release) from [<805f98ec>] (release_nodes+0x1e4/0x298) (release_nodes) from [<805f9ac8>] (devres_release_all+0x40/0x60) (devres_release_all) from [<805f5134>] (device_release_driver_internal+0x108/0x1ac) (device_release_driver_internal) from [<805f521c>] (device_driver_detach+0x20/0x24)Fixes: 349ad66c0ab0 ("spi:Add Freescale DSPI driver for Vybrid VF610 platform") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzk@kernel.org
This is an follow up of my other patch for I2C IMX driver [1]. Let's fix the issues consistently.
[1] https://lore.kernel.org/lkml/1592130544-19759-2-git-send-email-krzk@kernel.o...
Changes since v1:
- Disable the IRQ instead of using non-devm interface.
drivers/spi/spi-fsl-dspi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index 58190c94561f..023e05c53b85 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -1400,7 +1400,7 @@ static int dspi_probe(struct platform_device *pdev) ret = dspi_request_dma(dspi, res->start); if (ret < 0) { dev_err(&pdev->dev, "can't get dma channels\n");
goto out_clk_put;
goto disable_irq; } }@@ -1415,11 +1415,14 @@ static int dspi_probe(struct platform_device *pdev) ret = spi_register_controller(ctlr); if (ret != 0) { dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
goto out_clk_put;
goto disable_irq; } return ret;+disable_irq:
if (dspi->irq > 0)disable_irq(dspi->irq);out_clk_put: clk_disable_unprepare(dspi->clk); out_ctlr_put: @@ -1435,6 +1438,8 @@ static int dspi_remove(struct platform_device *pdev)
/* Disconnect from the SPI framework */ dspi_release_dma(dspi);
if (dspi->irq > 0)disable_irq(dspi->irq);What happens, if you re-bind the driver? Is the IRQ still working? Who is taking care of calling the enable_irq() again? What happens, if you really have a shared IRQ line? Is the IRQ disabled for all other devices on the same IRQ line?
Yup, devm is completely broken for shared IRQs.
Then we're back to this massive rework of using non-devm interface.
Best regards, Krzysztof