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

[Fan Wu]

Hi Nicolas,

Thanks for the explanation.

I agree this is better treated as defensive hardening rather than a
demonstrated teardown bug. The normal m2m release path is expected to wait
for a running job to finish, and the remaining case I was concerned about
would require unbind to race a still-running RGA job. Without a reproducer,
the explicit IRQ teardown and extra field look too heavy for that narrow
case.

I will drop this patch.

Thanks for the review,
Fan

> On Jul 6, 2026, at 23:57, Nicolas Dufresne <nicolas at ndufresne.ca> wrote:
> 
> Hi,
> 
> Le lundi 06 juillet 2026 à 23:27 +0800, Fan Wu a écrit :
>> Hi Nicolas,
>> 
>> Thanks, that is a fair point that RGA is not a free-running IRQ source;
>> the interrupt should only be raised for job completion.
>> 
>> I looked again for the abort/drain path, though, and I could not find one
>> in the RGA teardown path. `v4l2_m2m_release()` only frees the m2m device.
>> RGA does not provide a `.job_abort` callback, and `rga_remove()` does not
>> reset the engine or disable the IRQ before releasing the m2m state. The
>> driver also leaves the devm-managed IRQ installed until devres cleanup
>> after remove returns.
>> 
>> So unless I am missing another path, teardown does not actively abort an
>> in-flight RGA job; it relies on there being no in-flight job by the time
>> remove gets there. In the normal case that is probably true, since the
>> hardware is not free-running and the submitted job has normally completed
>> already. The case I was trying to cover is the narrower one where
>> unbind/remove races with a still-running job.
> 
> I've never looked very deep into that, and so this report isn't a bad thing at
> all. With the m2m frame work, we have m2m_dev (which is in fact the scheduler)
> and m2m_ctx, which are the instanced. The second are found to open/release, the
> first is probe/remove.
> 
> I don't know by which mechanism, so I'll try and learn that, but we expect
> platform remove() to only be called once all the file ops release() have been
> called. If that is broken, we should certainly do something about it.
> 
> Now, about not having RGA specific code to abort, this is fine. What the m2m
> framework do, in the default ctx_release() impelemtation is to call
> v4l2_m2m_cancel_job(). That function optionally call job_abort(), for drivers
> that requires it, but otherwise, it will just wait for running jobs to finish.
> Since most m2m, specially the simple case like RGA finishes quickly, the delay
> isn't a problem. After this, no more jobs will be scheduled, and no more IRQ are
> expected. Meaning protection against late IRQ is defensive. We've seen some HW
> bugs with other drivers, so its not generally wrong to do so.
> 
>> 
>> That said, I agree the explicit irq field, `devm_free_irq()` and the long
>> comment may be too much for a defensive corner-case fix without a
>> reproducer. I can drop this patch, or respin it as a smaller ordering
>> cleanup if you think that is useful.
> 
> What I meant is that a hardening patch based on quite hypothetical case should
> come with matching wording. Reading your submission, it felt nearly critical
> (and it could have been for sure). Let's investigate the remaining bits above.
> Then you can either reword or drop.
> 
> Nicolas
> 
>> 
>> Thanks,
>> Fan
>> 
>>> On Jul 6, 2026, at 22:22, Nicolas Dufresne <nicolas at ndufresne.ca> wrote:
>>> 
>>> 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 at vger.kernel.org
>>>> Signed-off-by: Fan Wu <fanwu01 at zju.edu.cn>
>>>> 
>>>> ---
>>>> 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;
>>>>