* [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
@ 2022-06-07 7:23 ` Ming Qian
0 siblings, 0 replies; 8+ messages in thread
From: Ming Qian @ 2022-06-07 7:23 UTC (permalink / raw)
To: mchehab, mirela.rabulea, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
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@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);
if (slot != ctx->slot) {
/* 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);
--
2.36.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
@ 2022-06-07 7:23 ` Ming Qian
0 siblings, 0 replies; 8+ messages in thread
From: Ming Qian @ 2022-06-07 7:23 UTC (permalink / raw)
To: mchehab, mirela.rabulea, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
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@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);
if (slot != ctx->slot) {
/* 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);
--
2.36.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
2022-06-07 7:23 ` Ming Qian
@ 2022-06-07 8:48 ` mirela.rabulea
-1 siblings, 0 replies; 8+ messages in thread
From: mirela.rabulea @ 2022-06-07 8:48 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
On 07.06.2022 10: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@nxp.com>
Thanks Ming,
Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Tested-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Regards,
Mirela
> ---
> 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);
>
> if (slot != ctx->slot) {
> /* 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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
@ 2022-06-07 8:48 ` mirela.rabulea
0 siblings, 0 replies; 8+ messages in thread
From: mirela.rabulea @ 2022-06-07 8:48 UTC (permalink / raw)
To: Ming Qian, mchehab, hverkuil-cisco
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
On 07.06.2022 10: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@nxp.com>
Thanks Ming,
Reviewed-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Tested-by: Mirela Rabulea <mirela.rabulea@nxp.com>
Regards,
Mirela
> ---
> 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);
>
> if (slot != ctx->slot) {
> /* 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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
2022-06-07 7:23 ` Ming Qian
@ 2022-06-09 10:27 ` Hans Verkuil
-1 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2022-06-09 10:27 UTC (permalink / raw)
To: Ming Qian, mchehab, mirela.rabulea
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
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@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.
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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
@ 2022-06-09 10:27 ` Hans Verkuil
0 siblings, 0 replies; 8+ messages in thread
From: Hans Verkuil @ 2022-06-09 10:27 UTC (permalink / raw)
To: Ming Qian, mchehab, mirela.rabulea
Cc: shawnguo, s.hauer, kernel, festevam, linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
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@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.
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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [EXT] Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
2022-06-09 10:27 ` Hans Verkuil
@ 2022-06-10 1:59 ` Ming Qian
-1 siblings, 0 replies; 8+ messages in thread
From: Ming Qian @ 2022-06-10 1:59 UTC (permalink / raw)
To: Hans Verkuil, mchehab, Mirela Rabulea (OSS)
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Sent: 2022年6月9日 18:27
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; Mirela Rabulea
> (OSS) <mirela.rabulea@oss.nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@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@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);
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [EXT] Re: [PATCH] media: imx-jpeg: Disable slot interrupt when frame done
@ 2022-06-10 1:59 ` Ming Qian
0 siblings, 0 replies; 8+ messages in thread
From: Ming Qian @ 2022-06-10 1:59 UTC (permalink / raw)
To: Hans Verkuil, mchehab, Mirela Rabulea (OSS)
Cc: shawnguo, s.hauer, kernel, festevam, dl-linux-imx, linux-media,
linux-kernel, devicetree, linux-arm-kernel
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Sent: 2022年6月9日 18:27
> To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org; Mirela Rabulea
> (OSS) <mirela.rabulea@oss.nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>;
> linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@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@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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-10 2:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-07 7:23 [PATCH] media: imx-jpeg: Disable slot interrupt when frame done Ming Qian
2022-06-07 7:23 ` Ming Qian
2022-06-07 8:48 ` mirela.rabulea
2022-06-07 8:48 ` mirela.rabulea
2022-06-09 10:27 ` Hans Verkuil
2022-06-09 10:27 ` Hans Verkuil
2022-06-10 1:59 ` [EXT] " Ming Qian
2022-06-10 1:59 ` Ming Qian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.