Re: [PATCH 1/2] media: qcom: camss: vfe-17x: dispatch wm_done per write master

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The VFE ISR mistakenly checks the `STATUS_1_RDI_SOF(i)` bitmask against the `status0` register instead of `status1`, missing actual SOF interrupts and ignoring `status1` entirely.
- [High] The loop reading `VFE_BUS_IRQ_STATUS` registers is incorrectly bounded by `vfe->res->line_num`, leading to out-of-bounds bus IRQ register reads and writes.
- [High] Bit collision occurs between `STATUS_0_RDI_REG_UPDATE(VFE_LINE_PIX)` and `STATUS_0_IMAGE_MASTER_PING_PONG(0)` as both evaluate to `BIT(8)`.
--

commit 53bcc62d295e9993945dc677a919dd6bb05592da
Author: Herman van Hazendonk <github.com@herrie.org>
Subject: media: qcom: camss: vfe-17x: dispatch wm_done per write master

This commit updates the wm_done dispatch loop to correctly use the
STATUS_0_IMAGE_MASTER_PING_PONG macro with the loop iterator. Previously,
it checked a constant BIT(9) which caused interrupts for other write
masters to be dropped and stalled their video streams.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> index e5ee7e717b3bc..3ad0c3be005f3 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> @@ -364,7 +364,7 @@ static irqreturn_t vfe_isr(int irq, void *dev)
>  			vfe->isr_ops.comp_done(vfe, i);
>  
>  	for (wm = 0; wm < MSM_VFE_IMAGE_MASTERS_NUM; wm++)
> -		if (status0 & BIT(9))

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the ISR,
is status1 read but subsequently ignored?

In vfe_isr():

	status1 = readl_relaxed(vfe->base + VFE_IRQ_STATUS_1);
	...
	for (i = VFE_LINE_RDI0; i < vfe->res->line_num; i++)
		if (status0 & STATUS_1_RDI_SOF(i))
			vfe->isr_ops.sof(vfe, i);

The STATUS_1_RDI_SOF(i) bitmask is checked against status0 instead of
status1. Does this missed evaluation of status1 cause genuine SOF
events and critical system interrupts to be permanently ignored?


[Severity: High]
This is a pre-existing issue, but does the loop reading
VFE_BUS_IRQ_STATUS registers go out of bounds?

In vfe_isr():

	for (i = VFE_LINE_RDI0; i < vfe->res->line_num; i++) {
		vfe_bus_status[i] = readl_relaxed(vfe->base + VFE_BUS_IRQ_STATUS(i));
		writel_relaxed(vfe_bus_status[i], vfe->base + VFE_BUS_IRQ_CLEAR(i));
	}

For VFE 170 configurations where line_num can be 4, does this loop exceed
the 3 available bus IRQ status registers?

If i = 3, VFE_BUS_IRQ_STATUS(3) calculates to offset 0x2068
(VFE_BUS_IRQ_CLEAR_GLOBAL), and VFE_BUS_IRQ_CLEAR(3) maps back to
VFE_BUS_IRQ_STATUS(0).

Can this lead to reading the global clear register and illegally writing
its value directly into the hardware status register STATUS(0)?


[Severity: High]
This is a pre-existing issue, but could we have a bit collision between
STATUS_0_RDI_REG_UPDATE() and STATUS_0_IMAGE_MASTER_PING_PONG()?

Looking at the macro definitions:

#define		STATUS_0_RDI_REG_UPDATE(n)		BIT((n) + 5)
#define		STATUS_0_IMAGE_MASTER_PING_PONG(n)	BIT((n) + 8)

For VFE_LINE_PIX (which maps to 3), STATUS_0_RDI_REG_UPDATE(3) evaluates
to BIT(8). Concurrently, STATUS_0_IMAGE_MASTER_PING_PONG(0) also maps to
BIT(8) for Write Master 0.

When a WM0 ping-pong interrupt asserts BIT(8) in status0, does the ISR
misinterpret it as a register update for line 3 and trigger a spurious
vfe->isr_ops.reg_update(vfe, 3) call?

> +		if (status0 & STATUS_0_IMAGE_MASTER_PING_PONG(wm))
>  			if (vfe_bus_status[1] & STATUS1_WM_CLIENT_BUF_DONE(wm))
>  				vfe->isr_ops.wm_done(vfe, wm);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603054238.562902-1-github.com@herrie.org?part=1