Re: [PATCH] media: s5p_cec: release IRQ before deleting the adapter

[Myeonghun Pak <mhun512@gmail.com>]

Hi Marek,

Agreed. I'll switch to request_threaded_irq() and free_irq() in v2.

Thanks for the review.

Best regards,
Myeonghun

2026년 10월 19일 (월) 오전 2:54, Marek Szyprowski <m.szyprowski@samsung.com>님이 작성:
>
> On 18.10.2026 05:51, Myeonghun Pak wrote:
> > The threaded IRQ handler reports transmit and receive events through
> > cec->adap. Adapter unregistration masks the hardware interrupts but does
> > not wait for this handler. With no open device-node references, it can
> > free the adapter while an already queued IRQ thread still uses it.
> > Devres also unmaps the registers before releasing the IRQ, since the
> > mapping was acquired after the IRQ.
> >
> > Free the managed IRQ before unregistering the adapter and disabling
> > runtime PM. Request it only after the register mapping and adapter have
> > been initialized, and free it on adapter registration failure before
> > deleting the adapter. This also makes IRQ release precede MMIO release
> > on probe unwind.
> >
> > This issue was identified as part of our ongoing research while
> > reviewing kernel code.
> >
> > Fixes: 1bcbf6f4b6b0 ("[media] cec: s5p-cec: Add s5p-cec driver")
> > Co-developed-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Ijae Kim <ae878000@gmail.com>
> > Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> >
> > ---
> > Compile-tested with Clang, W=1. No hardware runtime testing.
> >
> > Based on mainline 0fcb05b72344f40806cbe810de8872f1698bf053.
> >
> > diff --git a/drivers/media/cec/platform/s5p/s5p_cec.c b/drivers/media/cec/platform/s5p/s5p_cec.c
> > index 4a92d32..e83705f 100644
> > --- a/drivers/media/cec/platform/s5p/s5p_cec.c
> > +++ b/drivers/media/cec/platform/s5p/s5p_cec.c
> > @@ -199,11 +199,6 @@ static int s5p_cec_probe(struct platform_device *pdev)
> >       if (cec->irq < 0)
> >               return cec->irq;
> >
> > -     ret = devm_request_threaded_irq(dev, cec->irq, s5p_cec_irq_handler,
> > -             s5p_cec_irq_handler_thread, 0, pdev->name, cec);
> > -     if (ret)
> > -             return ret;
> > -
> >       cec->clk = devm_clk_get(dev, "hdmicec");
> >       if (IS_ERR(cec->clk))
> >               return PTR_ERR(cec->clk);
> > @@ -231,16 +226,24 @@ static int s5p_cec_probe(struct platform_device *pdev)
> >               goto err_delete_adapter;
> >       }
> >
> > -     ret = cec_register_adapter(cec->adap, &pdev->dev);
> > +     ret = devm_request_threaded_irq(dev, cec->irq, s5p_cec_irq_handler,
> > +                                     s5p_cec_irq_handler_thread, 0, pdev->name, cec);
> >       if (ret)
> >               goto err_notifier;
> >
> > +     ret = cec_register_adapter(cec->adap, &pdev->dev);
> > +     if (ret)
> > +             goto err_free_irq;
> > +
> >       platform_set_drvdata(pdev, cec);
> >       pm_runtime_enable(dev);
> >
> >       dev_dbg(dev, "successfully probed\n");
> >       return 0;
> >
> > +err_free_irq:
> > +     devm_free_irq(dev, cec->irq, cec);
> There is no point in using devm_* API if one needs to manually free the
> resource for whatever reason.
>
>
> > +
> >  err_notifier:
> >       cec_notifier_cec_adap_unregister(cec->notifier, cec->adap);
> >
> > @@ -253,6 +256,7 @@ static void s5p_cec_remove(struct platform_device *pdev)
> >  {
> >       struct s5p_cec_dev *cec = platform_get_drvdata(pdev);
> >
> > +     devm_free_irq(&pdev->dev, cec->irq, cec);
> >       cec_notifier_cec_adap_unregister(cec->notifier, cec->adap);
> >       cec_unregister_adapter(cec->adap);
> >       pm_runtime_disable(&pdev->dev);
> >
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>