| Message ID | 20200922093228.24917-2-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] spi: fsl-dspi: Use devm_spi_register_controller() | expand |
On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote: > @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev) > struct spi_controller *ctlr = platform_get_drvdata(pdev); > struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr); > > - /* Disconnect from the SPI framework */ > - spi_unregister_controller(dspi->ctlr); > - > /* Disable RX and TX */ > regmap_update_bits(dspi->regmap, SPI_MCR, > SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF, Is this fix safe - what happens if we start another transaction between disabling RX/TX and the unregistration taking effect? Similar concerns apply to some of the other patches.
On Tue, Sep 22, 2020 at 10:44:37AM +0100, Mark Brown wrote: > On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote: > > > @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev) > > struct spi_controller *ctlr = platform_get_drvdata(pdev); > > struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr); > > > > - /* Disconnect from the SPI framework */ > > - spi_unregister_controller(dspi->ctlr); > > - > > /* Disable RX and TX */ > > regmap_update_bits(dspi->regmap, SPI_MCR, > > SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF, > > Is this fix safe - what happens if we start another transaction between > disabling RX/TX and the unregistration taking effect? Similar concerns > apply to some of the other patches. I asked myself the same question until I saw that a dozen of other drivers also do it like this. I don't think it's safe. Currently we only have spi_unregister_controller() which both unregisters child SPI devices and frees the SPI controller along with the driver data. There are many drivers have something in their remove function like disabling the hardware, clocks, freeing interrupts. They do it either before calling spi_unregister_controller(), in which case messages may be queued on a teared down hardware, or they do it after calling spi_unregister_controller(), in which case they operate on already freed data. IMO this only works when all drivers are fully managed with devm_* functions and runtime pm so that the remove functions for all drivers become empty. Until this happens spi_unregister_controller() has to be split up in a function actually unregistering the controller and another one freeing the resources. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Sep 22, 2020 at 01:06:52PM +0200, Sascha Hauer wrote: > On Tue, Sep 22, 2020 at 10:44:37AM +0100, Mark Brown wrote: > > On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote: > > > - /* Disconnect from the SPI framework */ > > > - spi_unregister_controller(dspi->ctlr); > > > - > > > /* Disable RX and TX */ > > > regmap_update_bits(dspi->regmap, SPI_MCR, > > > SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF, > > Is this fix safe - what happens if we start another transaction between > > disabling RX/TX and the unregistration taking effect? Similar concerns > > apply to some of the other patches. > I asked myself the same question until I saw that a dozen of other drivers > also do it like this. Right, it's unfortunately common for people to just use devm without thinking about it too hard and it doesn't always get spotted :( > data. IMO this only works when all drivers are fully managed with devm_* > functions and runtime pm so that the remove functions for all drivers > become empty. Until this happens spi_unregister_controller() has to be > split up in a function actually unregistering the controller and another > one freeing the resources. It's probably easier to just not have this option for allocating driver data as part of the controller TBH, it's probably mroe trouble to use safely than it's worth.
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index 91c6affe139c..a6c100b89360 100644 --- a/drivers/spi/spi-fsl-dspi.c +++ b/drivers/spi/spi-fsl-dspi.c @@ -1416,7 +1416,7 @@ static int dspi_probe(struct platform_device *pdev) platform_set_drvdata(pdev, ctlr); - ret = spi_register_controller(ctlr); + ret = devm_spi_register_controller(&pdev->dev, ctlr); if (ret != 0) { dev_err(&pdev->dev, "Problem registering DSPI ctlr\n"); goto out_free_irq; @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev) struct spi_controller *ctlr = platform_get_drvdata(pdev); struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr); - /* Disconnect from the SPI framework */ - spi_unregister_controller(dspi->ctlr); - /* Disable RX and TX */ regmap_update_bits(dspi->regmap, SPI_MCR, SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,
Calling spi_unregister_controller() during driver remove results in freeing the SPI controller and the associated driver data. Using it later in dspi_remove() is a use-after-free bug. Register the controller with devm_spi_register_controller() instead which makes calling spi_unregister_controller() unnecessary. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/spi/spi-fsl-dspi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)