Re: [PATCH v7 15/19] media: meson: vdec: Fix concurrent firmware loading race and hardware timeout

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Suboptimal use of atomic polling in a sleepable context.

Pre-existing issues:
- [High] Missing memory barrier between firmware memcpy and DMA start.
- [High] Incomplete fix: identical bugs (static variables and raw decrement loop) remain in HEVC firmware loader.
- [High] Potential DMA Use-After-Free if firmware DMA transfer times out.
--

--- Patch [15]: [PATCH v7 15/19] media: meson: vdec: Fix concurrent firmware loading race and hardware timeout ---
commit 7acf244d598b1d1a15fbe219b6ceaead31b2210b
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix concurrent firmware loading race and hardware timeout

This commit fixes a concurrent firmware loading race condition by replacing
static variables with local ones in vdec_1_load_firmware(). It also addresses
hardware DMA hang timeouts by replacing a fast decrement loop with
readl_poll_timeout_atomic().

> diff --git a/drivers/staging/media/meson/vdec/vdec_1.c b/drivers/staging/media/meson/vdec/vdec_1.c
> --- a/drivers/staging/media/meson/vdec/vdec_1.c
> +++ b/drivers/staging/media/meson/vdec/vdec_1.c

[ ... ]

> @@ -63,11 +63,11 @@ vdec_1_load_firmware(struct amvdec_session *sess, const char *fwname)
>  	amvdec_write_dos(core, IMEM_DMA_COUNT, MC_SIZE / 4);
>  	amvdec_write_dos(core, IMEM_DMA_CTRL, (0x8000 | (7 << 16)));

[Severity: High]
This is a pre-existing issue, but is there a missing memory barrier between
the firmware memcpy and the DMA start?

Earlier in vdec_1_load_firmware(), the firmware is copied to a DMA-coherent
buffer:

    memcpy(mc_addr, fw->data, MC_SIZE);

Since amvdec_write_dos() maps to writel_relaxed(), there is no memory-to-I/O
ordering guarantee. Could the CPU issue the MMIO write to start the DMA
before the firmware data is flushed to main memory, causing the hardware to
read stale data?

> -	while (--i && amvdec_read_dos(core, IMEM_DMA_CTRL) & 0x8000);
> -
> -	if (i == 0) {
> +	ret = readl_poll_timeout_atomic(core->dos_base + IMEM_DMA_CTRL, val,
> +					!(val & 0x8000), 10, 10000);

[Severity: Low]
Does this need to use the atomic polling variant?

Since vdec_1_load_firmware() runs in a sleepable context (it successfully
calls request_firmware() and allocates memory with GFP_KERNEL just prior),
could this use the non-atomic readl_poll_timeout() macro instead?

Using the atomic variant forces the CPU to busy-wait in a udelay() loop for
up to 10ms, which might be inefficient.

> +	if (ret) {
>  		dev_err(dev, "Firmware load fail (DMA hang?)\n");
> -		ret = -EINVAL;
> +		ret = -ETIMEDOUT;
>  		goto free_mc;

[Severity: High]
This is a pre-existing issue, but does this create a DMA use-after-free if
the firmware DMA transfer times out?

When a timeout occurs, the code jumps to free_mc:

free_mc:
    dma_free_coherent(core->dev, MC_SIZE, mc_addr, mc_addr_map);

If the hardware is merely slow and still executing the DMA transfer, will it
continue to read from these freed memory pages? Should the DMA controller be
explicitly halted (for example, by clearing the IMEM_DMA_CTRL register)
before releasing the buffer?

>  	}

[Severity: High]
This is a pre-existing issue, but does the same problem still exist in the
HEVC firmware loader?

Looking at vdec_hevc_load_firmware() in vdec_hevc.c, it uses the exact same
static variables and fast decrement loop that this patch fixes in vdec_1.c:

static int vdec_hevc_load_firmware(...)
{
    ...
    static void *mc_addr;
    static dma_addr_t mc_addr_map;
    ...
    while (i && (readl(core->dos_base + HEVC_IMEM_DMA_CTRL) & 0x8000))
        i--;
    ...
}

Will HEVC decoding also fail consistently on modern CPUs due to false DMA
timeouts? And could multiple HEVC decoding sessions corrupt state by sharing
these static variables?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713120840.17427-1-linux.amoon@gmail.com?part=15

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic