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

[Nicolas Dufresne]

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;
> > > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20260706/94caa75b/attachment-0001.sig>