Re: [PATCH v2,1/2] media: mediatek: vcodec: adding lock to protect decoder context list

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxx>
> > > ---
> > >    .../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(-)