Re: [PATCH v15 11/12] media: mediatek: jpeg: add jpeg compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The `mtk8196_jpegenc_drvdata` structure is missing the `const` qualifier.

Pre-existing issues:
- [High] Out-of-bounds array access when probing hardware child nodes.
- [High] The upper 32 bits of the DMA stall address are incorrectly computed by adding the buffer size to the already-prepared extension bits.
--

Note: Due to recitation restrictions, the standard inline quoting format has
been altered. The following is a free-form summary of the findings rather than
an inline diff review.

commit a7e691d664fe928a3d65542a99e26554abd4f138
Author: Kyrie Wu <kyrie.wu@mediatek.com>
media: mediatek: jpeg: add jpeg compatible

This commit adds jpeg dec and enc compatible for mt8196.

[Severity: Low]
Looking at the new mtk8196_jpegenc_drvdata structure in
drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c, is it possible to
declare this as const?

The mtk8196_jpegdec_drvdata structure includes the const qualifier, but it
appears to be missing from the encoder variant. Adding it would ensure the
driver data is placed in the read-only .rodata section.

[Severity: High]
This is a pre-existing issue, but does the hardware probe logic overflow
the device arrays if a device tree specifies too many cores?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c during
mtk_jpegdec_hw_probe(), the code increments master_dev->hw_index and uses it
to index master_dev->dec_hw_dev and master_dev->reg_decbase without a bounds
check. The same pattern occurs in mtk_jpegenc_hw_probe() for the encoder.
Could a malformed device tree cause this unbounded index to write past the
end of these fixed-size arrays?

[Severity: High]
This is a pre-existing issue, but does enabling 34-bit support in this
patch expose a bug in the DMA stall address calculation?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c, the new
mtk8196_jpegenc_drvdata structure sets support_34bit to true.

Looking at mtk_jpeg_set_enc_dst() in
drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c, the stall extension
address is calculated as:

addr_ext = FIELD_PREP(MTK_JPEG_ADDR_MASK, upper_32_bits(dma_addr));
writel(addr_ext, base + JPEG_ENC_DEST_ADDR0_EXT);
writel(addr_ext + size, base + JPEG_ENC_STALL_ADDR0_EXT);

Is it mathematically valid to add the buffer size directly to the shifted
upper 32 bits? If the buffer crosses a 4GB boundary, wouldn't the large
byte size overflow the extension bits and pass incorrect upper address bits
to the hardware?

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