Re: [PATCH v7 8/9] media: chips-media: wave6: Add Wave6 control driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Data race on shared work_buffers_avail counter leads to DMA memory corruption.
- [High] Incompatible pointer type passed to `gen_pool_dma_alloc` causes structure memory corruption and invalid DMA mappings.
- [High] Use-after-free of the driver device structure in the async firmware load callback on unbind.
- [High] System deadlock caused by holding the parent's mutex while synchronously unregistering child devices.
- [High] Failure in work buffer initialization leaves the VPU state machine permanently corrupted.
- [High] Auxiliary power domains are never powered on because they lack device links.
- [Medium] Leaked runtime PM usage counter due to an improper conditional check on device suspend state.
- [Medium] Using a value of 0 to validate a DMA mapping leads to an IOMMU resource leak.
- [Medium] Mixing scoped guards with goto-based error handling violates kernel cleanup API rules.
--

commit 373df31fcd64a0ed065b0d792d9b1dac72e53fd5
Author: Nas Chung <nas.chung@chipsnmedia.com>
Subject: media: chips-media: wave6: Add Wave6 control driver

This commit adds the control driver for the Chips&Media Wave6 video codec IP.
It manages shared resources, firmware memory allocation, and power states for
the child core devices.

> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu.c
[ ... ]
> +static int wave6_vpu_init_vpu(struct wave6_vpu_device *vpu,
> +			      struct vpu_core_device *core)
> +{
[ ... ]
> +	wave6_vpu_set_state(vpu, WAVE6_VPU_STATE_PREPARE);
[ ... ]
> +init_done:
> +	ret = wave6_vpu_init_work_buf(vpu, core);
> +	if (ret)
> +		return ret;

[Severity: High]
If wave6_vpu_init_work_buf() fails here, the function returns immediately
without resetting vpu->state. Will this leave the driver permanently in
WAVE6_VPU_STATE_PREPARE, causing subsequent attempts to boot to skip the init
sequence?

> +
> +	wave6_vpu_set_state(vpu, WAVE6_VPU_STATE_ON);
> +
> +	return 0;
> +}
[ ... ]
> +static int wave6_vpu_get(struct wave6_vpu_device *vpu,
> +			 struct vpu_core_device *core)
> +{
> +	int ret;
> +
> +	if (WARN_ON(!vpu || !core))
> +		return -EINVAL;
> +
> +	guard(mutex)(&vpu->lock);
> +
> +	if (!vpu->fw_available)
> +		return -EINVAL;
[ ... ]
> +	ret = wave6_vpu_try_boot(vpu, core);
> +	if (ret)
> +		goto error_boot;
> +
> +	return 0;
> +
> +error_boot:
> +	pm_runtime_put_sync(vpu->dev);
> +error_pm:

[Severity: Medium]
Does this mix goto-based error handling with guard() cleanup in the same
function? The kernel cleanup API expects that a function either uses scoped
guards entirely or goto entirely to prevent confusing ownership and cleanup
ordering.

> +	atomic_dec(&vpu->core_count);
> +
> +	return ret;
> +}
> +
> +static void wave6_vpu_put(struct wave6_vpu_device *vpu,
> +			  struct vpu_core_device *core)
> +{
[ ... ]
> +	guard(mutex)(&vpu->lock);

[Severity: High]
Will this cause a system deadlock when the parent unregisters child devices?
During wave6_vpu_release(), the parent takes vpu->lock and triggers core
destruction. The child's remove callback will then try to put the VPU, leading
back into this function which attempts to acquire the exact same lock.

> +
> +	if (!vpu->fw_available)
> +		return;
[ ... ]
> +	wave6_vpu_sleep(vpu, core);
> +
> +	if (!pm_runtime_suspended(vpu->dev))
> +		pm_runtime_put_sync(vpu->dev);

[Severity: Medium]
Does conditionally skipping the put operation cause a usage counter leak?
If the device was suspended by other system events, skipping this symmetrical
put prevents the device from ever entering lower power states in the future.

> +}
> +
> +static void wave6_vpu_require_work_buffer(struct wave6_vpu_device *vpu,
> +					  struct vpu_core_device *core)
> +{
> +	struct vpu_buf *vb;
> +	u32 size;
[ ... ]
> +	if (WARN_ON(vpu->work_buffers_avail == 0))
> +		goto exit;
> +
> +	vpu->work_buffers_avail--;
> +	vb = &vpu->work_buffers[vpu->work_buffers_avail];

[Severity: High]
Is there a data race on the work_buffers_avail counter here? If multiple
VPU cores trigger requests concurrently without holding vpu->lock, could the
counter underflow and lead to an out-of-bounds array access that corrupts DMA
memory?

> +
> +	wave6_vdi_writel(core->reg_base, W6_CMD_SET_WORK_BUF_ADDR, vb->daddr);
> +
> +exit:
> +	wave6_vdi_writel(core->reg_base, W6_CMD_SET_WORK_BUF_SIZE, SET_WORK_BUF_SIZE_ACK);
> +}
[ ... ]
> +static void wave6_vpu_release(struct wave6_vpu_device *vpu)
> +{
> +	guard(mutex)(&vpu->lock);
> +
> +	vpu->fw_available = false;
> +	wave6_vpu_destroy_cores(vpu);
> +	wave6_vpu_free_work_buffers(vpu);
> +	if (vpu->sram_pool && vpu->sram_buf.vaddr) {
[ ... ]
> +	}
> +	if (vpu->code_buf.dma_addr)
> +		dma_unmap_resource(vpu->dev,

[Severity: Medium]
Could this condition leak valid IOMMU mappings if the DMA allocator returns 0?
Since 0 is a valid DMA address on many platforms, skipping the unmap when the
address evaluates to false can lead to a resource leak.

> +				   vpu->code_buf.dma_addr,
> +				   vpu->code_buf.size,
> +				   DMA_BIDIRECTIONAL,
> +				   0);
> +}
> +
> +static void wave6_vpu_load_firmware(const struct firmware *fw, void *context)
> +{
> +	struct wave6_vpu_device *vpu = context;
> +
> +	guard(mutex)(&vpu->lock);

[Severity: High]
Can this cause a use-after-free if the device unbinds while firmware loading
is in progress? If the driver is removed and devres frees the vpu memory,
this async callback might try to acquire a mutex that resides in freed memory.

> +
> +	if (!fw || !fw->data) {
[ ... ]
> +	if (fw->size + W6_EXTRA_CODE_BUF_SIZE > wave6_vpu_get_code_buf_size(vpu)) {
> +		dev_err(vpu->dev, "firmware size (%zd > %zd) is too big\n",
> +			fw->size, vpu->code_buf.size);
> +		vpu->fw_available = false;
> +		goto exit;

[Severity: Medium]
Is this mixing goto-based error handling and scoped guard cleanup again? This
violates the cleanup API guidelines.

> +	}
[ ... ]
> +static int wave6_vpu_attach_pm_domains(struct wave6_vpu_device *vpu)
> +{
> +	int ret;
[ ... ]
> +	if (vpu->num_pm_domains == 1) {
> +		/* genpd_dev_pm_attach() attach automatically if count is 1 */
> +		vpu->num_pm_domains = 0;
> +		return 0;
> +	}
> +
> +	ret = dev_pm_domain_attach_list(vpu->dev, NULL, &vpu->pd_list);

[Severity: High]
By passing NULL for the data parameter, does this skip creating device links
for the auxiliary domains? Without DL_FLAG_PM_RUNTIME links, the PM core won't
power on these domains during runtime, which might lead to exceptions when
accessing unpowered hardware registers.

> +	if (ret < 0) {
[ ... ]
> +static int wave6_vpu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	vpu->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> +	if (vpu->sram_pool) {
> +		vpu->sram_buf.size = vpu->res->sram_size;
> +		vpu->sram_buf.vaddr = gen_pool_dma_alloc(vpu->sram_pool,
> +							 vpu->sram_buf.size,
> +							 &vpu->sram_buf.phys_addr);
> +		if (!vpu->sram_buf.vaddr) {
> +			vpu->sram_buf.size = 0;
> +		} else {
> +			vpu->sram_buf.dma_addr = dma_map_resource(&pdev->dev,
> +								  vpu->sram_buf.phys_addr,

[Severity: High]
gen_pool_dma_alloc() expects a dma_addr_t pointer for its third argument to
write back the DMA address, but a phys_addr_t pointer is being passed here.
Could this cause out-of-bounds memory corruption on architectures where the
sizes differ? Additionally, is the returned DMA address being incorrectly
passed to dma_map_resource() as if it were a physical address?

> +								  vpu->sram_buf.size,
> +								  DMA_BIDIRECTIONAL,
> +								  0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788496816.git.nas.chung@chipsnmedia.com?part=8