Hi, On Tue, Sep 3, 2024 at 7:12 PM Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote: > > Since spi_geni_probe() use managed function in most places, also use > devm_request_irq() to request the interrupt, so we can avoid > having to manually clean this up. > > Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx> > --- > drivers/spi/spi-geni-qcom.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 37ef8c40b276..77eb874e4f54 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -1144,17 +1144,15 @@ static int spi_geni_probe(struct platform_device *pdev) > if (mas->cur_xfer_mode == GENI_GPI_DMA) > spi->flags = SPI_CONTROLLER_MUST_TX; > > - ret = request_irq(mas->irq, geni_spi_isr, 0, dev_name(dev), spi); > + ret = devm_request_irq(dev, mas->irq, geni_spi_isr, 0, dev_name(dev), spi); > if (ret) > goto spi_geni_release_dma; > > ret = spi_register_controller(spi); > if (ret) > - goto spi_geni_probe_free_irq; > + goto spi_geni_release_dma; > > return 0; > -spi_geni_probe_free_irq: > - free_irq(mas->irq, spi); > spi_geni_release_dma: > spi_geni_release_dma_chan(mas); > spi_geni_probe_runtime_disable: While the idea of using "devm" here is fine, this isn't quite the correct usage. You need to be really careful that using "devm" doesn't change the order that things happen in a way that breaks the driver. Specifically, before your patch the order we init things: 1. We enable runtime PM 2. We allocate DMA chans (spi_geni_init=>spi_geni_grab_gpi_chan) 3. We request the IRQ When we un-init in failed probe, we do the opposite order: 1. Free the IRQ 2. Free DMA chans 3. Disable runtime PM. Your patch switches IRQ the devm. devm handles un-initting things in the opposite order but all devm stuff happens _after_ the error handling in probe. That means that after your patch, the un-init is: 1. Free DMA chans 2. Disable runtime PM < start running devm stuff> 3. Free the IRQ ...so after your patch the IRQ stays enabled longer. I could imagine that an IRQ could go off and try to use a DMA channel or do something that needs runtime PM and then things will go boom. In the very least, parch #2 needs to come before this one and that would help, but it won't fix everything. Specifically in order to keep the order proper you'll need to use devm_add_action_or_reset() to "devm-ize" the freeing of the DMA channels. -Doug
