[EXT] Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done

[Ming Qian]

> From: Hans Verkuil <hverkuil-cisco at xs4all.nl>
> Sent: 2022年6月9日 18:27
> To: Ming Qian <ming.qian at nxp.com>; mchehab at kernel.org; Mirela Rabulea
> (OSS) <mirela.rabulea at oss.nxp.com>
> Cc: shawnguo at kernel.org; s.hauer at pengutronix.de; kernel at pengutronix.de;
> festevam at gmail.com; dl-linux-imx <linux-imx at nxp.com>;
> linux-media at vger.kernel.org; linux-kernel at vger.kernel.org;
> devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: [EXT] Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame
> done
> 
> Caution: EXT Email
> 
> Hi Ming Qian,
> 
> On 6/7/22 09:23, Ming Qian wrote:
> > The interrupt STMBUF_HALF may be triggered after frame done.
> > It may led to system hang if driver try to access the register after
> > power off.
> >
> > Disable the slot interrupt when frame done.
> >
> > Fixes: 2db16c6ed72ce ("media: imx-jpeg: Add V4L2 driver for i.MX8 JPEG
> > Encoder/Decoder")
> > Signed-off-by: Ming Qian <ming.qian at nxp.com>
> > ---
> >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c |  5 +++++
> > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h |  1 +
> >  drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c    | 11 ++---------
> >  3 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> > index c482228262a3..9418fcf740a8 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.c
> > @@ -79,6 +79,11 @@ void mxc_jpeg_enable_irq(void __iomem *reg, int
> slot)
> >       writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN));  }
> >
> > +void mxc_jpeg_disable_irq(void __iomem *reg, int slot) {
> > +     writel(0x0, reg + MXC_SLOT_OFFSET(slot, SLOT_IRQ_EN)); }
> > +
> >  void mxc_jpeg_sw_reset(void __iomem *reg)  {
> >       /*
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > index 07655502f4bd..ecf3b6562ba2 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg-hw.h
> > @@ -126,6 +126,7 @@ u32 mxc_jpeg_get_offset(void __iomem *reg, int
> > slot);  void mxc_jpeg_enable_slot(void __iomem *reg, int slot);  void
> > mxc_jpeg_set_l_endian(void __iomem *reg, int le);  void
> > mxc_jpeg_enable_irq(void __iomem *reg, int slot);
> > +void mxc_jpeg_disable_irq(void __iomem *reg, int slot);
> >  int mxc_jpeg_set_input(void __iomem *reg, u32 in_buf, u32 bufsize);
> > int mxc_jpeg_set_output(void __iomem *reg, u16 out_pitch, u32 out_buf,
> >                       u16 w, u16 h);
> > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > index 965021d3c7ef..b1f48835398e 100644
> > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c
> > @@ -592,15 +592,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void
> *priv)
> >       dev_dbg(dev, "Irq %d on slot %d.\n", irq, slot);
> >
> >       ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
> > -     if (!ctx) {
> > -             dev_err(dev,
> > -                     "Instance released before the end of
> transaction.\n");
> > -             /* soft reset only resets internal state, not registers */
> > -             mxc_jpeg_sw_reset(reg);
> > -             /* clear all interrupts */
> > -             writel(0xFFFFFFFF, reg + MXC_SLOT_OFFSET(slot,
> SLOT_STATUS));
> > -             goto job_unlock;
> > -     }
> > +     WARN_ON(!ctx);
> 
> This looks very scary, since if this happens,
> 
> >
> >       if (slot != ctx->slot) {
> 
> then it will crash here when it attempts to access ctx.
> 
> Shouldn't this be better?
> 
>         if (WARN_ON(!ctx))
>                 goto job_unlock;
> 
> It's certainly a lot more robust.

Yes, you're right, I'll make the v2 patch.
Thanks for your comments

Ming
> 
> Regards,
> 
>         Hans
> 
> >               /* TODO investigate when adding multi-instance support
> > */ @@ -673,6 +665,7 @@ static irqreturn_t mxc_jpeg_dec_irq(int irq, void
> *priv)
> >       buf_state = VB2_BUF_STATE_DONE;
> >
> >  buffers_done:
> > +     mxc_jpeg_disable_irq(reg, ctx->slot);
> >       jpeg->slot_data[slot].used = false; /* unused, but don't free */
> >       mxc_jpeg_check_and_set_last_buffer(ctx, src_buf, dst_buf);
> >       v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);