| Message ID | 20240129023153.28521-1-yunfei.dong@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] media: mediatek: vcodec: adding lock to protect decoder context list | expand |
Il 29/01/24 03:31, Yunfei Dong ha scritto: > The ctx_list will be deleted when scp getting unexpected behavior, then the > ctx_list->next will be NULL, the kernel driver maybe access NULL pointer in > function vpu_dec_ipi_handler when going through each context, then reboot. > > Need to add lock to protect the ctx_list to make sure the ctx_list->next isn't > NULL pointer. > > Hardware name: Google juniper sku16 board (DT) > pstate: 20400005 (nzCv daif +PAN -UAO -TCO BTYPE=--) > pc : vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec] > lr : scp_ipi_handler+0xd0/0x194 [mtk_scp] > sp : ffffffc0131dbbd0 > x29: ffffffc0131dbbd0 x28: 0000000000000000 > x27: ffffff9bb277f348 x26: ffffff9bb242ad00 > x25: ffffffd2d440d3b8 x24: ffffffd2a13ff1d4 > x23: ffffff9bb7fe85a0 x22: ffffffc0133fbdb0 > x21: 0000000000000010 x20: ffffff9b050ea328 > x19: ffffffc0131dbc08 x18: 0000000000001000 > x17: 0000000000000000 x16: ffffffd2d461c6e0 > x15: 0000000000000242 x14: 000000000000018f > x13: 000000000000004d x12: 0000000000000000 > x11: 0000000000000001 x10: fffffffffffffff0 > x9 : ffffff9bb6e793a8 x8 : 0000000000000000 > x7 : 0000000000000000 x6 : 000000000000003f > x5 : 0000000000000040 x4 : fffffffffffffff0 > x3 : 0000000000000020 x2 : ffffff9bb6e79080 > x1 : 0000000000000010 x0 : ffffffc0131dbc08 > Call trace: > vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec (HASH:6c3f 2)] > scp_ipi_handler+0xd0/0x194 [mtk_scp (HASH:7046 3)] > mt8183_scp_irq_handler+0x44/0x88 [mtk_scp (HASH:7046 3)] > scp_irq_handler+0x48/0x90 [mtk_scp (HASH:7046 3)] > irq_thread_fn+0x38/0x94 > irq_thread+0x100/0x1c0 > kthread+0x140/0x1fc > ret_from_fork+0x10/0x30 > Code: 54000088 f94ca50a eb14015f 54000060 (f9400108) > ---[ end trace ace43ce36cbd5c93 ]--- > Kernel panic - not syncing: Oops: Fatal exception > SMP: stopping secondary CPUs > Kernel Offset: 0x12c4000000 from 0xffffffc010000000 > PHYS_OFFSET: 0xffffffe580000000 > CPU features: 0x08240002,2188200c > Memory Limit: none > > 'Fixes: 655b86e52eac ("media: mediatek: vcodec: Fix possible invalid memory access for decoder")' Hello Yunfei, You've sent two patches as a v2, but: - The two patches are identical (!) apart from the commit message?! - It's Fixes: xxxx , not 'Fixes: xxxx' (please remove the quotes!) - There's no changelog from v1, so, what changed in v2?! Cheers, Angelo > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > --- > .../platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 4 ++-- > .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 5 +++++ > .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 ++ > drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c | 2 ++ > 4 files changed, 11 insertions(+), 2 deletions(-) >
Hi AngeloGioacchino, Thanks for your reviewing. On Mon, 2024-01-29 at 12:19 +0100, AngeloGioacchino Del Regno wrote: > Il 29/01/24 03:31, Yunfei Dong ha scritto: > > The ctx_list will be deleted when scp getting unexpected behavior, > > then the > > ctx_list->next will be NULL, the kernel driver maybe access NULL > > pointer in > > function vpu_dec_ipi_handler when going through each context, then > > reboot. > > > > Need to add lock to protect the ctx_list to make sure the ctx_list- > > >next isn't > > NULL pointer. > > > > Hardware name: Google juniper sku16 board (DT) > > pstate: 20400005 (nzCv daif +PAN -UAO -TCO BTYPE=--) > > pc : vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec] > > lr : scp_ipi_handler+0xd0/0x194 [mtk_scp] > > sp : ffffffc0131dbbd0 > > x29: ffffffc0131dbbd0 x28: 0000000000000000 > > x27: ffffff9bb277f348 x26: ffffff9bb242ad00 > > x25: ffffffd2d440d3b8 x24: ffffffd2a13ff1d4 > > x23: ffffff9bb7fe85a0 x22: ffffffc0133fbdb0 > > x21: 0000000000000010 x20: ffffff9b050ea328 > > x19: ffffffc0131dbc08 x18: 0000000000001000 > > x17: 0000000000000000 x16: ffffffd2d461c6e0 > > x15: 0000000000000242 x14: 000000000000018f > > x13: 000000000000004d x12: 0000000000000000 > > x11: 0000000000000001 x10: fffffffffffffff0 > > x9 : ffffff9bb6e793a8 x8 : 0000000000000000 > > x7 : 0000000000000000 x6 : 000000000000003f > > x5 : 0000000000000040 x4 : fffffffffffffff0 > > x3 : 0000000000000020 x2 : ffffff9bb6e79080 > > x1 : 0000000000000010 x0 : ffffffc0131dbc08 > > Call trace: > > vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec (HASH:6c3f 2)] > > scp_ipi_handler+0xd0/0x194 [mtk_scp (HASH:7046 3)] > > mt8183_scp_irq_handler+0x44/0x88 [mtk_scp (HASH:7046 3)] > > scp_irq_handler+0x48/0x90 [mtk_scp (HASH:7046 3)] > > irq_thread_fn+0x38/0x94 > > irq_thread+0x100/0x1c0 > > kthread+0x140/0x1fc > > ret_from_fork+0x10/0x30 > > Code: 54000088 f94ca50a eb14015f 54000060 (f9400108) > > ---[ end trace ace43ce36cbd5c93 ]--- > > Kernel panic - not syncing: Oops: Fatal exception > > SMP: stopping secondary CPUs > > Kernel Offset: 0x12c4000000 from 0xffffffc010000000 > > PHYS_OFFSET: 0xffffffe580000000 > > CPU features: 0x08240002,2188200c > > Memory Limit: none > > > > 'Fixes: 655b86e52eac ("media: mediatek: vcodec: Fix possible > > invalid memory access for decoder")' > > Hello Yunfei, > > You've sent two patches as a v2, but: > - The two patches are identical (!) apart from the commit message?! > - It's Fixes: xxxx , not 'Fixes: xxxx' (please remove the quotes!) > - There's no changelog from v1, so, what changed in v2?! > 1> These two patch used to fix the same issue, just used to separate encoder with decoder; 2> Will fix in next patch; 3> patch 1 are the same for v1 and v2, just the patch 2 (encoder) change something. Best Regards, Yunfei Dong > Cheers, > Angelo > > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> > > --- > > .../platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 4 > > ++-- > > .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 5 > > +++++ > > .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 > > ++ > > drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c | 2 > > ++ > > 4 files changed, 11 insertions(+), 2 deletions(-) > > > >
Il 30/01/24 07:29, Yunfei Dong (董云飞) ha scritto: > Hi AngeloGioacchino, > > Thanks for your reviewing. > On Mon, 2024-01-29 at 12:19 +0100, AngeloGioacchino Del Regno wrote: >> Il 29/01/24 03:31, Yunfei Dong ha scritto: >>> The ctx_list will be deleted when scp getting unexpected behavior, >>> then the >>> ctx_list->next will be NULL, the kernel driver maybe access NULL >>> pointer in >>> function vpu_dec_ipi_handler when going through each context, then >>> reboot. >>> >>> Need to add lock to protect the ctx_list to make sure the ctx_list- >>>> next isn't >>> NULL pointer. >>> >>> Hardware name: Google juniper sku16 board (DT) >>> pstate: 20400005 (nzCv daif +PAN -UAO -TCO BTYPE=--) >>> pc : vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec] >>> lr : scp_ipi_handler+0xd0/0x194 [mtk_scp] >>> sp : ffffffc0131dbbd0 >>> x29: ffffffc0131dbbd0 x28: 0000000000000000 >>> x27: ffffff9bb277f348 x26: ffffff9bb242ad00 >>> x25: ffffffd2d440d3b8 x24: ffffffd2a13ff1d4 >>> x23: ffffff9bb7fe85a0 x22: ffffffc0133fbdb0 >>> x21: 0000000000000010 x20: ffffff9b050ea328 >>> x19: ffffffc0131dbc08 x18: 0000000000001000 >>> x17: 0000000000000000 x16: ffffffd2d461c6e0 >>> x15: 0000000000000242 x14: 000000000000018f >>> x13: 000000000000004d x12: 0000000000000000 >>> x11: 0000000000000001 x10: fffffffffffffff0 >>> x9 : ffffff9bb6e793a8 x8 : 0000000000000000 >>> x7 : 0000000000000000 x6 : 000000000000003f >>> x5 : 0000000000000040 x4 : fffffffffffffff0 >>> x3 : 0000000000000020 x2 : ffffff9bb6e79080 >>> x1 : 0000000000000010 x0 : ffffffc0131dbc08 >>> Call trace: >>> vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec (HASH:6c3f 2)] >>> scp_ipi_handler+0xd0/0x194 [mtk_scp (HASH:7046 3)] >>> mt8183_scp_irq_handler+0x44/0x88 [mtk_scp (HASH:7046 3)] >>> scp_irq_handler+0x48/0x90 [mtk_scp (HASH:7046 3)] >>> irq_thread_fn+0x38/0x94 >>> irq_thread+0x100/0x1c0 >>> kthread+0x140/0x1fc >>> ret_from_fork+0x10/0x30 >>> Code: 54000088 f94ca50a eb14015f 54000060 (f9400108) >>> ---[ end trace ace43ce36cbd5c93 ]--- >>> Kernel panic - not syncing: Oops: Fatal exception >>> SMP: stopping secondary CPUs >>> Kernel Offset: 0x12c4000000 from 0xffffffc010000000 >>> PHYS_OFFSET: 0xffffffe580000000 >>> CPU features: 0x08240002,2188200c >>> Memory Limit: none >>> >>> 'Fixes: 655b86e52eac ("media: mediatek: vcodec: Fix possible >>> invalid memory access for decoder")' >> >> Hello Yunfei, >> >> You've sent two patches as a v2, but: >> - The two patches are identical (!) apart from the commit message?! >> - It's Fixes: xxxx , not 'Fixes: xxxx' (please remove the quotes!) >> - There's no changelog from v1, so, what changed in v2?! >> > 1> These two patch used to fix the same issue, just used to separate > encoder with decoder; I just noticed that - I'm sorry. > 2> Will fix in next patch; > 3> patch 1 are the same for v1 and v2, just the patch 2 (encoder) > change something. > Next time, can you please add a cover letter to your series? I think it would be easier for people to see what changed in the entire series, even if it is just two or three patches, as you'd be writing the changelog in there instead of writing it in each patch :-) > Best Regards, > Yunfei Dong >> Cheers, >> Angelo >> >>> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> >>> --- >>> .../platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 4 >>> ++-- >>> .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 5 >>> +++++ >>> .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 >>> ++ >>> drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c | 2 >>> ++ >>> 4 files changed, 11 insertions(+), 2 deletions(-) >>> >> >>
diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c index 9f6e4b59455d..9a11a2c24804 100644 --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c @@ -58,12 +58,12 @@ static void mtk_vcodec_vpu_reset_dec_handler(void *priv) dev_err(&dev->plat_dev->dev, "Watchdog timeout!!"); - mutex_lock(&dev->dev_mutex); + mutex_lock(&dev->dev_ctx_lock); list_for_each_entry(ctx, &dev->ctx_list, list) { ctx->state = MTK_STATE_ABORT; mtk_v4l2_vdec_dbg(0, ctx, "[%d] Change to state MTK_STATE_ABORT", ctx->id); } - mutex_unlock(&dev->dev_mutex); + mutex_unlock(&dev->dev_ctx_lock); } static void mtk_vcodec_vpu_reset_enc_handler(void *priv) diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c index f47c98faf068..2073781ccadb 100644 --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c @@ -268,7 +268,9 @@ static int fops_vcodec_open(struct file *file) ctx->dev->vdec_pdata->init_vdec_params(ctx); + mutex_lock(&dev->dev_ctx_lock); list_add(&ctx->list, &dev->ctx_list); + mutex_unlock(&dev->dev_ctx_lock); mtk_vcodec_dbgfs_create(ctx); mutex_unlock(&dev->dev_mutex); @@ -311,7 +313,9 @@ static int fops_vcodec_release(struct file *file) v4l2_ctrl_handler_free(&ctx->ctrl_hdl); mtk_vcodec_dbgfs_remove(dev, ctx->id); + mutex_lock(&dev->dev_ctx_lock); list_del_init(&ctx->list); + mutex_unlock(&dev->dev_ctx_lock); kfree(ctx); mutex_unlock(&dev->dev_mutex); return 0; @@ -404,6 +408,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev) for (i = 0; i < MTK_VDEC_HW_MAX; i++) mutex_init(&dev->dec_mutex[i]); mutex_init(&dev->dev_mutex); + mutex_init(&dev->dev_ctx_lock); spin_lock_init(&dev->irqlock); snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), "%s", diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h index 849b89dd205c..85b2c0d3d8bc 100644 --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h @@ -241,6 +241,7 @@ struct mtk_vcodec_dec_ctx { * * @dec_mutex: decoder hardware lock * @dev_mutex: video_device lock + * @dev_ctx_lock: the lock of context list * @decode_workqueue: decode work queue * * @irqlock: protect data access by irq handler and work thread @@ -282,6 +283,7 @@ struct mtk_vcodec_dec_dev { /* decoder hardware mutex lock */ struct mutex dec_mutex[MTK_VDEC_HW_MAX]; struct mutex dev_mutex; + struct mutex dev_ctx_lock; struct workqueue_struct *decode_workqueue; spinlock_t irqlock; diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c index 82e57ae983d5..da6be556727b 100644 --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c @@ -77,12 +77,14 @@ static bool vpu_dec_check_ap_inst(struct mtk_vcodec_dec_dev *dec_dev, struct vde struct mtk_vcodec_dec_ctx *ctx; int ret = false; + mutex_lock(&dec_dev->dev_ctx_lock); list_for_each_entry(ctx, &dec_dev->ctx_list, list) { if (!IS_ERR_OR_NULL(ctx) && ctx->vpu_inst == vpu) { ret = true; break; } } + mutex_unlock(&dec_dev->dev_ctx_lock); return ret; }
The ctx_list will be deleted when scp getting unexpected behavior, then the ctx_list->next will be NULL, the kernel driver maybe access NULL pointer in function vpu_dec_ipi_handler when going through each context, then reboot. Need to add lock to protect the ctx_list to make sure the ctx_list->next isn't NULL pointer. Hardware name: Google juniper sku16 board (DT) pstate: 20400005 (nzCv daif +PAN -UAO -TCO BTYPE=--) pc : vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec] lr : scp_ipi_handler+0xd0/0x194 [mtk_scp] sp : ffffffc0131dbbd0 x29: ffffffc0131dbbd0 x28: 0000000000000000 x27: ffffff9bb277f348 x26: ffffff9bb242ad00 x25: ffffffd2d440d3b8 x24: ffffffd2a13ff1d4 x23: ffffff9bb7fe85a0 x22: ffffffc0133fbdb0 x21: 0000000000000010 x20: ffffff9b050ea328 x19: ffffffc0131dbc08 x18: 0000000000001000 x17: 0000000000000000 x16: ffffffd2d461c6e0 x15: 0000000000000242 x14: 000000000000018f x13: 000000000000004d x12: 0000000000000000 x11: 0000000000000001 x10: fffffffffffffff0 x9 : ffffff9bb6e793a8 x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f x5 : 0000000000000040 x4 : fffffffffffffff0 x3 : 0000000000000020 x2 : ffffff9bb6e79080 x1 : 0000000000000010 x0 : ffffffc0131dbc08 Call trace: vpu_dec_ipi_handler+0x58/0x1f8 [mtk_vcodec_dec (HASH:6c3f 2)] scp_ipi_handler+0xd0/0x194 [mtk_scp (HASH:7046 3)] mt8183_scp_irq_handler+0x44/0x88 [mtk_scp (HASH:7046 3)] scp_irq_handler+0x48/0x90 [mtk_scp (HASH:7046 3)] irq_thread_fn+0x38/0x94 irq_thread+0x100/0x1c0 kthread+0x140/0x1fc ret_from_fork+0x10/0x30 Code: 54000088 f94ca50a eb14015f 54000060 (f9400108) ---[ end trace ace43ce36cbd5c93 ]--- Kernel panic - not syncing: Oops: Fatal exception SMP: stopping secondary CPUs Kernel Offset: 0x12c4000000 from 0xffffffc010000000 PHYS_OFFSET: 0xffffffe580000000 CPU features: 0x08240002,2188200c Memory Limit: none 'Fixes: 655b86e52eac ("media: mediatek: vcodec: Fix possible invalid memory access for decoder")' Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com> --- .../platform/mediatek/vcodec/common/mtk_vcodec_fw_vpu.c | 4 ++-- .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c | 5 +++++ .../platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h | 2 ++ drivers/media/platform/mediatek/vcodec/decoder/vdec_vpu_if.c | 2 ++ 4 files changed, 11 insertions(+), 2 deletions(-)