> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Sent: 2022年6月9日 18:27
> To: Ming Qian <ming.qian@xxxxxxx>; mchehab@xxxxxxxxxx; Mirela Rabulea
> (OSS) <mirela.rabulea@xxxxxxxxxxx>
> Cc: shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > ---
> > 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);
