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.
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);
