Re: [PATCH v5 08/14] media: mediatek: vcodec: clean xpc status

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly introduced `mtk_vdec_hw_clean_xpc` function performs non-atomic read-modify-write (RMW) operations on a shared hardware register from concurrent interrupt handlers without any locking, leading to potential state corruption and lost XPC clean pulses.
- [Low] Type confusion in enum usage for indexing main_dev->reg_base.
--

commit 6d6918cc2e35280ad95cd095aa0042f4f69bdfc5
Author: Yunfei Dong <yunfei.dong@mediatek.com>

media: mediatek: vcodec: clean xpc status

This commit adds functionality to clean the XPC status upon receiving a 
decoder hardware interrupt for the MT8196 platform.

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> index 881d5de41e050..e4e527fe54dcb 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> @@ -61,6 +61,31 @@ static int mtk_vdec_hw_prob_done(struct mtk_vcodec_dec_dev *vdec_dev)
>  	return 0;
>  }
>  
> +static void mtk_vdec_hw_write_reg_mask(void __iomem *reg_base, u32 reg_offset, u32 val, u32 mask)
> +{
> +	void __iomem *reg_addr = reg_base + reg_offset;
> +	u32 reg_val;
> +
> +	reg_val =  readl(reg_addr);
> +	reg_val &= ~mask;
> +	reg_val |= (val & mask);
> +	writel(reg_val, reg_addr);
> +}

[Severity: High]
Since LAT0 and CORE hardware subdevices share the same parent device register
space and have separate interrupt lines, can their interrupt handlers race
here? 

If both execute concurrently on different CPUs and call
mtk_vdec_hw_write_reg_mask(), could the unsynchronized read-modify-write
sequence cause one CPU to overwrite and lose the XPC clean pulse from the
other? 

Does this sequence need a lock to protect the shared register state?

> +
> +static void mtk_vdec_hw_clean_xpc(struct mtk_vdec_hw_dev *dev)
> +{
> +	u32 val, mask, addr = VDEC_XPC_CLEAN_ADDR;

[ ... ]

> @@ -166,6 +193,7 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev)
>  	subdev_dev->hw_idx = hw_idx;
>  	subdev_dev->main_dev = main_dev;
>  	subdev_dev->reg_base[VDEC_HW_SYS] = main_dev->reg_base[VDEC_HW_SYS];
> +	subdev_dev->reg_base[VDEC_HW_XPC] = main_dev->reg_base[VDEC_HW_MISC];

[Severity: Low]
Is the index VDEC_HW_MISC correct for main_dev->reg_base? 

It appears main_dev->reg_base is typically indexed by enum mtk_hw_reg_idx
(like VDEC_MISC), while VDEC_HW_MISC belongs to enum mtk_vdec_hw_reg_idx. 

Even though both currently evaluate to 1 and map correctly, could this cause
issues or point to the wrong hardware registers if the enums drift apart in
future updates?

>  	set_bit(subdev_dev->hw_idx, main_dev->subdev_bitmap);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603084045.17488-1-kyrie.wu@mediatek.com?part=8