Re: [PATCH v17 01/12] media: mediatek: jpeg: fix jpeg cores' amounts setting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: mtk-jpeg: unbounded DT child nodes cause stack and heap buffer overflows
- [High] media: mtk-jpeg: overwriting hw_rdy to zero deadlocks worker threads
- [Medium] media: mtk-jpeg: missing wake_up in child probes stalls waiting workers
--

commit 0bd01f62e3fa6a9492c56fbcebce1fed2d29d3bd
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: fix jpeg cores' amounts setting
    
Different ICs have different amounts of cores,
use a variable to set the cores' amounts.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index d147ec4830811..41c4cf8dc2626 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1325,7 +1325,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  		for_each_child_of_node(pdev->dev.of_node, child)
>  			num_child++;
>  
> -		atomic_set(&jpeg->hw_rdy, num_child);
> +		jpeg->max_hw_count = num_child;
> +		atomic_set(&jpeg->hw_rdy, 0);

[Severity: High]
Does setting hw_rdy to 0 here cause a deadlock for worker threads?

Since mtk_jpeg_probe() synchronously probes available child devices via
devm_of_platform_populate() just before this, those probes will have already
incremented hw_rdy correctly.

If we unconditionally reset hw_rdy to 0 here, it overwrites that count.
When jobs are queued, mtk_jpegenc_get_hw() assigns an idle hardware core and
decrements hw_rdy, making it negative. When cores finish and increment
hw_rdy, it will remain <= 0, causing all subsequent jobs that block on
wait_event_interruptible(..., hw_rdy > 0) to sleep forever.

>  		atomic_set(&jpeg->hw_index, 0);

[ ... ]

> @@ -1462,7 +1463,7 @@ static int mtk_jpegenc_get_hw(struct mtk_jpeg_ctx *ctx)
>  	int i;
>  
>  	spin_lock_irqsave(&jpeg->hw_lock, flags);
> -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +	for (i = 0; i < jpeg->max_hw_count; i++) {
>  		comp_jpeg = jpeg->enc_hw_dev[i];
>  		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {

[Severity: High]
This isn't a bug introduced by this patch, but can this locklessly
dereference an unprobed child device?

If userspace queues a video job before child devices are guaranteed to
have probed (or if a child node is disabled in the device tree), this
loop can read a null pointer from jpeg->enc_hw_dev[i]. Because
mtk_jpegenc_hw_probe() publishes the device pointer to this array
without synchronization, comp_jpeg can be null here, leading to a panic
when checking comp_jpeg->hw_state.

>  			hw_id = i;

[ ... ]

> @@ -1592,7 +1593,7 @@ static void mtk_jpegenc_worker(struct work_struct *work)
>  		jpeg_work);
>  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  
> -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++)
> +	for (i = 0; i < jpeg->max_hw_count; i++)
>  		comp_jpeg[i] = jpeg->enc_hw_dev[i];

[Severity: High]
Does this code overflow the fixed-size comp_jpeg stack array?

Since jpeg->max_hw_count is derived directly from the device tree without
bounds checking against MTK_JPEGENC_HW_MAX, this loop can write out of
bounds if the device tree has more child nodes than expected.

>  	i = 0;

[ ... ]

> @@ -1687,7 +1688,7 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  	struct mtk_jpeg_fb fb;
>  	unsigned long flags;
>  
> -	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++)
> +	for (i = 0; i < jpeg->max_hw_count; i++)
>  		comp_jpeg[i] = jpeg->dec_hw_dev[i];

[Severity: High]
Does this code also overflow the fixed-size comp_jpeg stack array for the
same reason?

>  	i = 0;

[ ... ]

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> index 32372781daf57..9ca68cde45a48 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> @@ -668,6 +668,8 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	atomic_inc(&master_dev->hw_rdy);
> +

[Severity: Medium]
Does this increment need a corresponding wake-up signal for pending workers?

If child devices are probed asynchronously (e.g., due to probe deferral)
and a worker thread is already waiting on hw_wq for a core to become
ready, the thread will miss the readiness notification and sleep forever
without a wake_up(&master_dev->hw_wq) call.

>  	return 0;
>  }

[ ... ]

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> index b312a15d707b2..44a064dc7b3d0 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -389,6 +389,8 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	atomic_inc(&master_dev->hw_rdy);
> +

[Severity: Medium]
Does this also require a wake-up signal to prevent worker threads from
stalling, just like in mtk_jpegdec_hw_probe()?

>  	return 0;
>  }

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