Re: [PATCH 2/3] media: mali-c55: Keep IRQ requested during suspend

[Dan Scally <dan.scally@ideasonboard.com>]

Hi Linus

On 29/09/2026 13:02, Linus Walleij wrote:
> The interrupt is currently freed on every runtime suspend and requested
> again on runtime resume. Apart from tying interrupt ownership to the
> power state rather than to the driver lifetime, this leaves remove to
> guess whether an action is installed for the IRQ.
> 
> Request the interrupt once during probe and free it during remove.
> 
> Disable and synchronize the IRQ before powering the ISP off, and enable
> it only after a successful power-on.
> 
> Hold a runtime PM reference during probe until the IRQ is installed,
> and stop runtime PM and drain the IRQ before unregistering the media
> entities during remove.
> 
> If firmware marks the ISP as a wakeup source, initialize device wakeup
> and enable IRQ wake during system suspend. Disable it again before
> resuming the device.

I think that adding this could be a separate commit to fixing the irq handling...and perhaps that 
should be the case given the Fixes tag?

> 
> This configures the interrupt controller wake path.
> 
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>   .../media/platform/arm/mali-c55/mali-c55-core.c    | 84 ++++++++++++++++------
>   1 file changed, 64 insertions(+), 20 deletions(-)
> 
> 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..07267b79801b 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -17,6 +17,8 @@
>   #include <linux/of_reserved_mem.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/property.h>
>   #include <linux/reset.h>
>   #include <linux/slab.h>
>   #include <linux/string.h>
> @@ -675,8 +677,7 @@ static int __maybe_unused mali_c55_runtime_suspend(struct device *dev)
>   {
>   	struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
>   
> -	if (irq_has_action(mali_c55->irqnum))
> -		free_irq(mali_c55->irqnum, dev);
> +	disable_irq(mali_c55->irqnum);
>   	__mali_c55_power_off(mali_c55);
>   
>   	return 0;
> @@ -745,25 +746,41 @@ static int __maybe_unused mali_c55_runtime_resume(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	/*
> -	 * The driver needs to transfer large amounts of register settings to
> -	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
> -	 * threaded IRQ to avoid disabling interrupts the entire time that's
> -	 * happening.
> -	 */
> -	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
> -				   IRQF_ONESHOT, dev_driver_string(dev), dev);
> -	if (ret) {
> -		__mali_c55_power_off(mali_c55);
> -		dev_err(dev, "failed to request irq\n");
> +	enable_irq(mali_c55->irqnum);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mali_c55_suspend(struct device *dev)
> +{
> +	struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (device_may_wakeup(dev)) {
> +		ret = enable_irq_wake(mali_c55->irqnum);
> +		if (ret)
> +			return ret;
>   	}
>   
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret && device_may_wakeup(dev))
> +		disable_irq_wake(mali_c55->irqnum);
> +
>   	return ret;
>   }
>   
> +static int __maybe_unused mali_c55_resume(struct device *dev)
> +{
> +	struct mali_c55 *mali_c55 = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(mali_c55->irqnum);
> +
> +	return pm_runtime_force_resume(dev);
> +}
> +
>   static const struct dev_pm_ops mali_c55_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	SET_SYSTEM_SLEEP_PM_OPS(mali_c55_suspend, mali_c55_resume)
>   	SET_RUNTIME_PM_OPS(mali_c55_runtime_suspend, mali_c55_runtime_resume,
>   			   NULL)
>   };
> @@ -825,27 +842,53 @@ static int mali_c55_probe(struct platform_device *pdev)
>   	pm_runtime_set_autosuspend_delay(&pdev->dev, 2000);
>   	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_get_noresume(dev);
>   	pm_runtime_enable(&pdev->dev);
>   
>   	ret = mali_c55_media_frameworks_init(mali_c55);
>   	if (ret)
>   		goto err_pm_runtime_disable;
>   
> -	pm_runtime_idle(&pdev->dev);
> -
>   	mali_c55->irqnum = platform_get_irq(pdev, 0);
>   	if (mali_c55->irqnum < 0) {
>   		ret = mali_c55->irqnum;
>   		goto err_deinit_media_frameworks;
>   	}
>   
> +	/*
> +	 * The driver needs to transfer large amounts of register settings to
> +	 * the ISP each frame, using either a DMA transfer or memcpy. We use a
> +	 * threaded IRQ to avoid disabling interrupts the entire time that's
> +	 * happening.
> +	 */
> +	ret = request_threaded_irq(mali_c55->irqnum, NULL, mali_c55_isr,
> +				   IRQF_ONESHOT, dev_driver_string(dev), dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		goto err_deinit_media_frameworks;
> +	}
> +
> +	if (device_property_read_bool(dev, "wakeup-source")) {
> +		ret = devm_device_init_wakeup(dev);
> +		if (ret) {
> +			ret = dev_err_probe(dev, ret,
> +					    "failed to initialize wakeup\n");
> +			goto err_free_irq;
> +		}
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);
> +
>   	return 0;
>   
> +err_free_irq:
> +	free_irq(mali_c55->irqnum, dev);
>   err_deinit_media_frameworks:
>   	mali_c55_media_frameworks_deinit(mali_c55);
>   err_pm_runtime_disable:
> -	pm_runtime_set_suspended(&pdev->dev);
>   	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(&pdev->dev);

I would say that this re-ordering of the pm_runtime_set_suspended() and pm_runtime_disable() calls 
probably ought to be in a separate commit too, since it's a distinct change that should be 
backported to fix stable branches.

Thanks
Dan

>   	kfree(mali_c55->context.registers);
>   err_power_off:
>   	__mali_c55_power_off(mali_c55);
> @@ -859,12 +902,13 @@ static void mali_c55_remove(struct platform_device *pdev)
>   {
>   	struct mali_c55 *mali_c55 = platform_get_drvdata(pdev);
>   
> +	pm_runtime_disable(&pdev->dev);
> +	free_irq(mali_c55->irqnum, &pdev->dev);
>   	mali_c55_media_frameworks_deinit(mali_c55);
> -	if (!pm_runtime_suspended(&pdev->dev)) {
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
>   		__mali_c55_power_off(mali_c55);
>   		pm_runtime_set_suspended(&pdev->dev);
>   	}
> -	pm_runtime_disable(&pdev->dev);
>   	kfree(mali_c55->context.registers);
>   	of_reserved_mem_device_release(&pdev->dev);
>   }
>