Re: [PATCH] media: rockchip: rga: quiesce IRQ before releasing m2m state

[Nicolas Dufresne]

Hi,

Le samedi 04 juillet 2026 à 02:28 +0000, Fan Wu a écrit :
> rga_probe() requests the interrupt with devm_request_irq(), so devres
> does not release the IRQ until after rga_remove() returns. rga_remove()
> currently releases rga->m2m_dev before that point.
> 
> rga_isr() uses rga->m2m_dev through v4l2_m2m_job_finish(),
> leaving a window where an interrupt can run after the m2m device has been
> released.

I have a doubt that this can really happen for this type of hardware. Its not a
free-running HW that emits IRQ randomly, plus we have the abort sequence that
ensure all jobs are completed before we pull it down.

> 
> Unregister the video device first to stop new userspace submissions, then
> free the devm-managed IRQ explicitly before releasing the m2m device. Move
> the command buffer release after the IRQ teardown as well, so it is not
> released while a completion interrupt can still arrive.
> 
> Store the IRQ number in struct rockchip_rga so rga_remove() can free the
> IRQ without looking it up again.
> 
> Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
> 
> ---
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 43f6a8d..118887a 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -828,6 +828,8 @@ static int rga_probe(struct platform_device *pdev)
>  		goto err_put_clk;
>  	}
>  
> +	rga->irq = irq;
> +
>  	ret = devm_request_irq(rga->dev, irq, rga_isr, 0,
>  			       dev_name(rga->dev), rga);
>  	if (ret < 0) {
> @@ -919,13 +921,21 @@ static void rga_remove(struct platform_device *pdev)
>  {
>  	struct rockchip_rga *rga = platform_get_drvdata(pdev);
>  
> -	dma_free_attrs(rga->dev, RGA_CMDBUF_SIZE, rga->cmdbuf_virt,
> -		       rga->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE);
> -
>  	v4l2_info(&rga->v4l2_dev, "Removing\n");
>  
> -	v4l2_m2m_release(rga->m2m_dev);
>  	video_unregister_device(rga->vfd);
> +
> +	/*
> +	 * The IRQ was requested with devm_request_irq() and is freed by devm
> +	 * only after this function returns. Free it explicitly here, after the
> +	 * video device is unregistered, but before v4l2_m2m_release() frees
> +	 * rga->m2m_dev, which rga_isr() dereferences via v4l2_m2m_job_finish().
> +	 */
> +	devm_free_irq(rga->dev, rga->irq, rga);

I'm not saying we cannot do that, but its quite verbose for something that
probably can't happen in practice.

Nicolas

> +
> +	dma_free_attrs(rga->dev, RGA_CMDBUF_SIZE, rga->cmdbuf_virt,
> +		       rga->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE);
> +	v4l2_m2m_release(rga->m2m_dev);
>  	v4l2_device_unregister(&rga->v4l2_dev);
>  
>  	pm_runtime_disable(rga->dev);
> diff --git a/drivers/media/platform/rockchip/rga/rga.h b/drivers/media/platform/rockchip/rga/rga.h
> index 72a28b1..f76c45b 100644
> --- a/drivers/media/platform/rockchip/rga/rga.h
> +++ b/drivers/media/platform/rockchip/rga/rga.h
> @@ -81,6 +81,7 @@ struct rockchip_rga {
>  	struct device *dev;
>  	struct regmap *grf;
>  	void __iomem *regs;
> +	int irq;
>  	struct clk *sclk;
>  	struct clk *aclk;
>  	struct clk *hclk;
>

Attachment: signature.asc
Description: This is a digitally signed message part