[PATCH] media: mali-c55: free IRQ when remove while runtime-active

[PATCH] media: mali-c55: free IRQ when remove while runtime-active

[Li Youhong]

From: Li Youhong <liyouhong@kylinos.cn>

The IRQ is requested in runtime_resume() and released in
runtime_suspend(). remove() powers the device off when it is still
runtime-active, but does not free the IRQ.

If the driver is unbound while streaming (or otherwise runtime-active
after a successful resume), the threaded IRQ handler remains registered
against a powered-off device.

Free the IRQ in remove() the same way runtime_suspend() does.

Fixes: 2c9b9bcc2569 ("media: mali-c55: Power-off the peripheral in remove()")
Signed-off-by: Li Youhong <liyouhong@kylinos.cn>
---
 drivers/media/platform/arm/mali-c55/mali-c55-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
index f28e9f4354ac..2f6658a2ac15 100644
--- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
+++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
@@ -861,6 +861,8 @@ static void mali_c55_remove(struct platform_device *pdev)
 
 	mali_c55_media_frameworks_deinit(mali_c55);
 	if (!pm_runtime_suspended(&pdev->dev)) {
+		if (irq_has_action(mali_c55->irqnum))
+			free_irq(mali_c55->irqnum, &pdev->dev);
 		__mali_c55_power_off(mali_c55);
 		pm_runtime_set_suspended(&pdev->dev);
 	}
-- 
2.25.1

Re: [PATCH] media: mali-c55: free IRQ when remove while runtime-active

[Linus Walleij]

On Thu, Sep 3, 2026 at 4:55 AM Li Youhong <dayou5941@163.com> wrote:

> @@ -861,6 +861,8 @@ static void mali_c55_remove(struct platform_device *pdev)
>
>         mali_c55_media_frameworks_deinit(mali_c55);
>         if (!pm_runtime_suspended(&pdev->dev)) {
> +               if (irq_has_action(mali_c55->irqnum))
> +                       free_irq(mali_c55->irqnum, &pdev->dev);
>                 __mali_c55_power_off(mali_c55);
>                 pm_runtime_set_suspended(&pdev->dev);

This whole runtime suspend/resume looks a bit odd to me.

Normally you keep the IRQ requested perpetually in a driver,
this looks like some really odd workaround to explicitly deal with
wakeup capable/non-wakeup capable IRQs.

I tried to see if this was addressed during review of the driver
and whether there was a reason for doing things like this.
But there are too many threads on lore.

I think the proper fix is to keep the IRQ requested during
suspend/resume and *not* mark it as wakeup-source; in the
device tree for the SoC.

Support code is needed in the driver, something along the line
of this:

suspend():
                if (device_may_wakeup(dev))
                        enable_irq_wake(irq);

resume():
                if (device_may_wakeup(dev))
                        disable_irq_wake(irq);

Do you want to make the patch or should I?

Yours,
Linus Walleij

Re: [PATCH] media: mali-c55: free IRQ when remove while runtime-active

[Jacopo Mondi]

Hi Linus

On Fri, Sep 04, 2026 at 12:12:20PM +0200, Linus Walleij wrote:
> On Thu, Sep 3, 2026 at 4:55 AM Li Youhong <dayou5941@163.com> wrote:
>
> > @@ -861,6 +861,8 @@ static void mali_c55_remove(struct platform_device *pdev)
> >
> >         mali_c55_media_frameworks_deinit(mali_c55);
> >         if (!pm_runtime_suspended(&pdev->dev)) {
> > +               if (irq_has_action(mali_c55->irqnum))
> > +                       free_irq(mali_c55->irqnum, &pdev->dev);
> >                 __mali_c55_power_off(mali_c55);
> >                 pm_runtime_set_suspended(&pdev->dev);
>
> This whole runtime suspend/resume looks a bit odd to me.
>
> Normally you keep the IRQ requested perpetually in a driver,
> this looks like some really odd workaround to explicitly deal with
> wakeup capable/non-wakeup capable IRQs.
>
> I tried to see if this was addressed during review of the driver
> and whether there was a reason for doing things like this.
> But there are too many threads on lore.

Indeed it's weird.

I've a branch that I started that tried to fix that, but the issue I
was hitting was due the fact the ISP starts with all ISP unmasked, and
I get very bad races between the power-up routine and the IRQ.

I run out of time and gave up, but if you're interested I can provide
you references to that work

>
> I think the proper fix is to keep the IRQ requested during
> suspend/resume and *not* mark it as wakeup-source; in the
> device tree for the SoC.
>
> Support code is needed in the driver, something along the line
> of this:
>
> suspend():
>                 if (device_may_wakeup(dev))
>                         enable_irq_wake(irq);
>
> resume():
>                 if (device_may_wakeup(dev))
>                         disable_irq_wake(irq);
>
> Do you want to make the patch or should I?

That's something I didn't consider maybe :)

>
> Yours,
> Linus Walleij

Re: [PATCH] media: mali-c55: free IRQ when remove while runtime-active

[Dan Scally]

Hi all

On 04/09/2026 11:40, Jacopo Mondi wrote:
> Hi Linus
> 
> On Fri, Sep 04, 2026 at 12:12:20PM +0200, Linus Walleij wrote:
>> On Thu, Sep 3, 2026 at 4:55 AM Li Youhong <dayou5941@163.com> wrote:
>>
>>> @@ -861,6 +861,8 @@ static void mali_c55_remove(struct platform_device *pdev)
>>>
>>>          mali_c55_media_frameworks_deinit(mali_c55);
>>>          if (!pm_runtime_suspended(&pdev->dev)) {
>>> +               if (irq_has_action(mali_c55->irqnum))
>>> +                       free_irq(mali_c55->irqnum, &pdev->dev);
>>>                  __mali_c55_power_off(mali_c55);
>>>                  pm_runtime_set_suspended(&pdev->dev);
>>
>> This whole runtime suspend/resume looks a bit odd to me.
>>
>> Normally you keep the IRQ requested perpetually in a driver,
>> this looks like some really odd workaround to explicitly deal with
>> wakeup capable/non-wakeup capable IRQs.
>>
>> I tried to see if this was addressed during review of the driver
>> and whether there was a reason for doing things like this.
>> But there are too many threads on lore.
> 
> Indeed it's weird.

It's my bad; and there's no specific reason for doing it that way to workaround anything, I was just 
mistaken in what best practice was. I think I got the impression that this was the proper way to do 
it from a misreading of the Interrupt Handling chapter in Linux Device Drivers. I don't think it was 
really discussed during review of this driver, but for the rzv2h-ivc driver it was picked up and we 
corrected it there.

Dan
> 
> I've a branch that I started that tried to fix that, but the issue I
> was hitting was due the fact the ISP starts with all ISP unmasked, and
> I get very bad races between the power-up routine and the IRQ.
> 
> I run out of time and gave up, but if you're interested I can provide
> you references to that work
> 
>>
>> I think the proper fix is to keep the IRQ requested during
>> suspend/resume and *not* mark it as wakeup-source; in the
>> device tree for the SoC.
>>
>> Support code is needed in the driver, something along the line
>> of this:
>>
>> suspend():
>>                  if (device_may_wakeup(dev))
>>                          enable_irq_wake(irq);
>>
>> resume():
>>                  if (device_may_wakeup(dev))
>>                          disable_irq_wake(irq);
>>
>> Do you want to make the patch or should I?
> 
> That's something I didn't consider maybe :)
> 
>>
>> Yours,
>> Linus Walleij