Re: [PATCH v6 5/9] media: chips-media: wave6: Add Wave6 core driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Allocating `struct video_device` with `devm_kzalloc()` and an empty release callback causes a Use-After-Free when the driver is unbound while a file descriptor is open.
- [High] Freeing `core->irq_fifo` in `remove()` while the devm-managed IRQ handler is still active creates a use-after-free window.
- [High] Returning early for `W6_INT_BIT_REQ_WORK_BUF` in `wave6_vpu_core_irq` drops other concurrent hardware interrupts.
- [High] The interrupt thread drops completion events because `wave6_vpu_core_get_instance()` only returns the first matching instance.
- [High] Writing 0 to `W6_RET_INT_INSTANCE_INFO` creates a race condition that can drop newly raised interrupts.
- [High] The hardware interrupt handler is registered before the kfifo is allocated, leading to a potential NULL pointer dereference on early interrupts.
- [High] Hardware initialization failures during PM runtime resume are silently ignored, falsely reporting a successful resume.
- [High] Video devices are unregistered after PM runtime is disabled, allowing active file operations to access hardware without power management.
- [High] The IRQ handler unconditionally accesses hardware registers without checking if the device is active.
- [High] `wave6_vpu_disable_instance()` ignores timeouts while waiting for refcount to drop, potentially leading to Use-After-Free and ABA problems with stale events.
- [Medium] The driver creates a debugfs directory but never removes it, causing a memory leak on module unload.
- [Medium] Clocks are left permanently enabled if the driver is removed while the device is runtime-active.
- [Medium] Ignoring the return value of `kfifo_in()` allows interrupts to be silently dropped if the queue fills up.
--

commit dac8dbfd6064814484fc40f423abb75bd9d68a7c
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Add Wave6 core driver

Add the core driver for the Chips&Media Wave6 video codec IP.
It configures the V4L2 mem2mem devices and communicates with the Wave6
hardware to perform video processing tasks.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c b/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c
> new file mode 100644
> index 0000000000000..e54a2a39ff9a3
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c
[ ... ]
> +static irqreturn_t wave6_vpu_core_irq(int irq, void *dev_id)
> +{
> +	struct vpu_core_device *core = dev_id;
> +	struct vpu_irq irq_info;
> +
> +	if (!vpu_read_reg(core, W6_VPU_VPU_INT_STS))
> +		return IRQ_NONE;

[Severity: High]
Does this unconditionally access hardware registers without checking if the
device is active?

If a spurious interrupt occurs while the device is runtime-suspended and its
clocks are disabled, this unclocked register read could cause an asynchronous
bus fault on platforms like i.MX95. Should the handler use
pm_runtime_get_if_active() before reading MMIO registers?

> +
> +	irq_info.status = vpu_read_reg(core, W6_VPU_VINT_REASON);
> +	irq_info.inst_idc = vpu_read_reg(core, W6_RET_INT_INSTANCE_INFO);
> +
> +	vpu_write_reg(core, W6_RET_INT_INSTANCE_INFO, INT_INSTANCE_INFO_CLEAR);

[Severity: High]
Does unconditionally writing 0 to W6_RET_INT_INSTANCE_INFO create a race
condition?

If the hardware finishes another operation and sets a new bit in this register
between the read and the write, the write of 0 will clear the newly set bit
before it can be read, permanently dropping the interrupt for that instance.

> +	vpu_write_reg(core, W6_VPU_VINT_REASON_CLEAR, irq_info.status);
> +	vpu_write_reg(core, W6_VPU_VINT_CLEAR, VINT_CLEAR);
> +
> +	trace_wave6_vpu_irq(core, irq_info.status, irq_info.inst_idc);
> +
> +	if (irq_info.status & BIT(W6_INT_BIT_REQ_WORK_BUF)) {
> +		if (core->vpu)
> +			core->vpu->req_work_buffer(core->vpu, core);
> +
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Will returning early for W6_INT_BIT_REQ_WORK_BUF drop other concurrent
hardware interrupts?

Because the entire irq_info.status mask was already cleared from the hardware,
returning here bypasses the kfifo_in() call. Any other interrupt events (like
DEC_PIC or BSBUF_ERROR) present in the status are never queued to the
interrupt thread and will be permanently lost, potentially hanging the
affected instances.

> +
> +	kfifo_in(&core->irq_fifo, &irq_info, sizeof(struct vpu_irq));

[Severity: Medium]
What happens if the FIFO fills up?

kfifo_in() returns the number of bytes queued (or 0 if full). Ignoring the
return value allows interrupts to be silently dropped if a burst occurs before
the threaded handler can process them, leaving VPU instances waiting
indefinitely. Should this at least log an error on overflow?

> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static struct vpu_instance *wave6_vpu_core_get_instance(struct vpu_core_device *core,
> +							u32 inst_idc)
> +{
> +	struct vpu_instance *inst;
> +
> +	guard(spinlock)(&core->inst_lock);
> +
> +	list_for_each_entry(inst, &core->instances, list) {
> +		if ((BIT(inst->id) & inst_idc) && inst->enable) {
> +			atomic_inc(&inst->refcount);
> +			return inst;
> +		}
> +	}
> +
> +	return NULL;
> +}

[Severity: High]
Does this only return the first matching instance?

The hardware provides a bitmask of interrupting instances in inst_idc. By
iterating and returning only the first matched instance, any other instances
that had their bits set in the same inst_idc mask are completely ignored,
causing them to miss their interrupts and hang indefinitely.

[ ... ]
> +void wave6_vpu_disable_instance(struct vpu_instance *inst)
> +{
> +	int count;
> +
> +	scoped_guard(spinlock, &inst->dev->inst_lock)
> +		inst->enable = false;
> +
> +	if (read_poll_timeout(atomic_read, count, !count,
> +			      W6_VPU_POLL_DELAY_US,
> +			      W6_VPU_POLL_TIMEOUT,
> +			      true, &inst->refcount))
> +		dev_dbg(inst->dev->dev, "[%d] disable timeout\n", inst->id);
> +}

[Severity: High]
Does ignoring the timeout here lead to a potential use-after-free and ABA
problem?

If read_poll_timeout() times out, the function merely logs a debug message and
returns, allowing the caller to proceed to destroy the instance. If the
delayed IRQ thread is still processing the instance, it will access freed
memory.

Additionally, refcount only tracks events actively being processed by the
thread, not un-popped events still in the kfifo. Thus, disable_instance() can
return while events are still in the FIFO. If a new instance is created
reusing the same ID, the IRQ thread could pop the stale event and incorrectly
match it to the new instance.

[ ... ]
> +static void wave6_vpu_core_check_state(struct vpu_core_device *core)
> +{
> +	u32 val;
> +	int ret;
> +
> +	guard(mutex)(&core->hw_lock);
> +
> +	ret = read_poll_timeout(vpu_read_reg, val, val != 0,
> +				W6_VPU_POLL_DELAY_US, W6_VPU_POLL_TIMEOUT,
> +				false, core, W6_VPU_VCPU_CUR_PC);
> +	if (ret)
> +		return;

[Severity: High]
Are hardware initialization failures swallowed here?

This function returns void and drops the error code if read_poll_timeout()
or wave6_vpu_get_version() fails.

[ ... ]
> +static int wave6_vpu_core_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_request_threaded_irq(&pdev->dev, irq,
> +					wave6_vpu_core_irq,
> +					wave6_vpu_core_irq_thread,
> +					0, "vpu_irq", core);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request IRQ: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = v4l2_device_register(&pdev->dev, &core->v4l2_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register v4l2_dev: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = wave6_vpu_init_m2m_dev(core);
> +	if (ret)
> +		goto err_v4l2_unregister;
> +
> +	ret = kfifo_alloc(&core->irq_fifo,
> +			  MAX_NUM_INSTANCE * sizeof(struct vpu_irq),
> +			  GFP_KERNEL);

[Severity: High]
Could an early interrupt cause a NULL pointer dereference?

devm_request_threaded_irq() is called before kfifo_alloc(). If an interrupt
fires immediately upon registration, wave6_vpu_core_irq() will execute and
attempt to push the event into the unallocated FIFO using kfifo_in(),
leading to a crash.

[ ... ]
> +	core->debugfs = debugfs_lookup(WAVE6_VPU_DEBUGFS_DIR, NULL);
> +	if (!IS_ERR_OR_NULL(core->debugfs))
> +		dput(core->debugfs);
> +	else
> +		core->debugfs = debugfs_create_dir(WAVE6_VPU_DEBUGFS_DIR, NULL);

[Severity: Medium]
Is there a missing debugfs_remove() call for this directory in
wave6_vpu_core_remove()? This leaks the directory on module unload.

> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	if (core->res->codec_types & WAVE6_IS_DEC) {
> +		ret = wave6_vpu_dec_register_device(core);

[Severity: High]
Does allocating the video_device with devm_kzalloc() inside
wave6_vpu_dec_register_device() cause a use-after-free?

wave6_vpu_dec_register_device() and wave6_vpu_enc_register_device() allocate
the video device via devm_kzalloc() and assign an empty release callback. If
a user holds a file descriptor open and the driver is unbound, devres frees
the video_device memory. When the user later closes the file descriptor, the
V4L2 core accesses the now-freed video_device struct.

[ ... ]
> +static void wave6_vpu_core_remove(struct platform_device *pdev)
> +{
> +	struct vpu_core_device *core = dev_get_drvdata(&pdev->dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	wave6_vpu_enc_unregister_device(core);
> +	wave6_vpu_dec_unregister_device(core);

[Severity: High]
Is PM runtime disabled before the user-facing video devices are unregistered?

Unregistering video devices after pm_runtime_disable() allows active file
operations to access hardware without power management. If a user application
issues an ioctl during this window, the driver may attempt to wake the device,
failing and leading to unexpected errors or unpowered hardware access.

[Severity: Medium]
Are clocks left permanently enabled if the driver is removed while the device
is active?

Since this function does not explicitly suspend the device or disable clocks,
and devm_clk_bulk_get_all() only releases the clock references without
disabling them, the device clocks will be left enabled if the driver is
unbound while active (e.g., streaming).

> +	wave6_vdi_free_dma(&core->temp_vbuf);
> +	kfifo_free(&core->irq_fifo);

[Severity: High]
Does manually freeing core->irq_fifo create a use-after-free window?

The IRQ was requested using devm_request_threaded_irq(), meaning it will be
freed during the devres cleanup phase after remove() returns. Manually calling
kfifo_free() here creates a window where the devm-managed IRQ remains active
but the kfifo and DMA memory are already freed. A late or spurious interrupt
during this window will trigger kfifo_in() on freed memory.

> +	wave6_vpu_release_m2m_dev(core);
> +	v4l2_device_unregister(&core->v4l2_dev);
> +}
[ ... ]
> +static int wave6_vpu_core_runtime_resume(struct device *dev)
> +{
[ ... ]
> +	/*
> +	 * Only call parent VPU get_vpu if the core has a parent and is active.
> +	 * - core->vpu: prevent access in core without parent VPU.
> +	 * - core->active: execute boot only after m2m streaming is started.
> +	 */
> +	if (core->vpu && core->active)
> +		ret = core->vpu->get_vpu(core->vpu, core);
> +
> +	if (!ret)
> +		wave6_vpu_core_wait_activated(core);
> +	else if (core->num_clks)
> +		clk_bulk_disable_unprepare(core->num_clks, core->clks);
> +
> +	return ret;
> +}

[Severity: High]
Because wave6_vpu_core_check_state() is void and cannot propagate failures,
wave6_vpu_core_wait_activated() will return without an error if it times out
or fails to get the version.

This means wave6_vpu_core_runtime_resume() will return 0 (success) to the PM
framework even if hardware initialization failed, leaving the device unable to
generate interrupts and hanging subsequent V4L2 operations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624072043.238-1-nas.chung@chipsnmedia.com?part=5