Re: [PATCH v6 6/9] media: chips-media: wave6: Improve debugging capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Kernel panic due to division by zero in debugfs FPS calculation.
- [High] Lockless access to `vb2_queue` internals causes Use-After-Free during concurrent `REQBUFS(0)`.
- [Medium] Checking `IS_ERR_OR_NULL` on an optional debugfs dentry inappropriately triggers `WARN_ON` when debugfs is disabled.
- [Medium] 32-bit integer multiplication overflow when calculating FPS statistics.
- [Medium] Data truncation of 64-bit DMA addresses in trace events.
- [Low] Use of 64-bit division and modulo operators breaks compilation on 32-bit architectures.
- [Low] Incorrect chaining of assignments with comma operators in `TP_fast_assign`.
--

commit 59a3991812bdf6b7f45f197b804c2beea6a0e9aa
Author: Nas Chung <nas.chung@chipsnmedia.com>
Subject: media: chips-media: wave6: Improve debugging capabilities

Add debugfs entries and trace events to provide detailed
debugging information.
These enhancements help diagnose issues and improve debugging
capabilities for the Wave6 core driver.

Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Tested-by: Ming Qian <ming.qian@oss.nxp.com>
Tested-by: Marek Vasut <marek.vasut@mailbox.org>

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-trace.h b/drivers/media/platform/chips-media/wave6/wave6-trace.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-trace.h
[ ... ]
> +TRACE_EVENT(wave6_vpu_source_change,
> +	    TP_PROTO(struct vpu_instance *inst, struct dec_seq_info *info),
> +	    TP_ARGS(inst, info),
[ ... ]
> +	    TP_fast_assign(__assign_str(name);
> +			   __entry->id = inst->id;
> +			   __entry->width = info->pic_width,
> +			   __entry->height = info->pic_height,

[Severity: Low]
Is there a typo in these assignments? The width and height fields are
terminated with commas instead of semicolons inside TP_fast_assign.

> +			   __entry->profile = info->profile,
> +			   __entry->level = info->level;
[ ... ]
> +TRACE_EVENT(wave6_vpu_enc_pic,
> +	    TP_PROTO(struct vpu_instance *inst, struct enc_param *param),
> +	    TP_ARGS(inst, param),
> +	    TP_STRUCT__entry(__string(name, dev_name(inst->dev->dev))
> +			     __field(u32, id)
> +			     __field(u32, srcidx)
> +			     __field(u32, buf_y)

[Severity: Medium]
Does this truncate 64-bit physical addresses? 

Variables like buf_y and buf_cb are dma_addr_t, which can be 64-bit on 
platforms like arm64. Declaring these fields as u32 will lose the upper 
32 bits of the address in the trace output.

> +			     __field(u32, buf_cb)
> +			     __field(u32, buf_cr)
> +			     __field(u32, stride)
[ ... ]
> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c b/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu-dbg.c
[ ... ]
> +static int wave6_vpu_dbg_instance(struct seq_file *s, void *data)
> +{
> +	struct vpu_instance *inst = s->private;
> +	struct vpu_performance_info *perf = &inst->performance;
> +	struct vb2_queue *vq;
[ ... ]
> +	vq = v4l2_m2m_get_src_vq(inst->v4l2_fh.m2m_ctx);
> +	seq_printf(s, "output (%2d, %2d): fmt = %c%c%c%c %d x %d, %d;\n",
> +		   vb2_is_streaming(vq),
> +		   vb2_get_num_buffers(vq),

[Severity: High]
Can this lead to a use-after-free? 

This accesses the queue state locklessly in wave6_vpu_dbg_instance(). If a
concurrent userspace thread calls VIDIOC_REQBUFS with count=0, the buffers
bitmap is freed.

If the lockless read evaluates vb2_get_num_buffers() at the same time, it 
can race against the release and cause bitmap_weight() to read uninitialized
or freed memory.

> +		   inst->src_fmt.pixelformat,
[ ... ]
> +	seq_puts(s, "fps");
> +	tmp = MSEC_PER_SEC * inst->processed_buf_num;

[Severity: Medium]
Can this multiplication overflow on 32-bit architectures? 

MSEC_PER_SEC and inst->processed_buf_num are both 32-bit integers. The result
will wrap around after about 4.29 million frames before the implicit cast to 
the 64-bit tmp variable.

> +	if (perf->ts_last > perf->ts_first + NSEC_PER_MSEC) {
> +		fps = DIV_ROUND_CLOSEST(tmp, (perf->ts_last - perf->ts_first) / NSEC_PER_MSEC);

[Severity: Low]
Will this break the build on 32-bit platforms? 

The calculation performs direct division using the / operator on 64-bit types, 
which results in undefined references to __divdi3. Should this use a kernel 
math helper like div64_s64() instead?

> +		seq_printf(s, " actual: %lld;", fps);
> +	}
> +	if (perf->total_sw_time) {
> +		fps = DIV_ROUND_CLOSEST(tmp, perf->total_sw_time / NSEC_PER_MSEC);

[Severity: High]
What happens if total_sw_time or total_hw_time is non-zero but less than
1,000,000 nanoseconds?

The division perf->total_sw_time / NSEC_PER_MSEC will evaluate to zero.
Passing zero as the divisor to DIV_ROUND_CLOSEST causes an immediate
division by zero exception and a kernel panic.

> +		seq_printf(s, " sw: %lld;", fps);
> +	}
> +	if (perf->total_hw_time) {
> +		fps = DIV_ROUND_CLOSEST(tmp, perf->total_hw_time / NSEC_PER_MSEC);
> +		seq_printf(s, " hw: %lld", fps);
> +	}
[ ... ]
> +int wave6_vpu_create_dbgfs_file(struct vpu_instance *inst)
> +{
> +	char name[64];
> +
> +	if (WARN_ON(!inst || !inst->dev || IS_ERR_OR_NULL(inst->dev->debugfs)))
> +		return -EINVAL;

[Severity: Medium]
Is it intended to trigger a warning when debugfs is disabled?

If CONFIG_DEBUG_FS is disabled, debugfs_create_dir() returns ERR_PTR(-ENODEV).
Using IS_ERR_OR_NULL() here will trigger the WARN_ON() every time a new
encoder or decoder instance is created, dumping a stack trace into the kernel
log.

> +
> +	scnprintf(name, sizeof(name), "instance.%d", inst->id);

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