Re: [PATCH -next 1/3] spi: geni-qcom: Use devm_request_irq() helper

[Doug Anderson <dianders@xxxxxxxxxxxx>]

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