[PATCH 0/2] media: qcom: camss: vfe-17x: fix wm_done dispatch and silence unmapped-WM noise

[PATCH 0/2] media: qcom: camss: vfe-17x: fix wm_done dispatch and silence unmapped-WM noise

[Herman van Hazendonk]

The vfe-17x wm_done IRQ path has two independent bugs fixed here:

PATCH 1/2 fixes a gate condition in the wm_done dispatch loop that
used a constant BIT(9) — STATUS_0_IMAGE_MASTER_PING_PONG(1) — instead
of the per-WM macro STATUS_0_IMAGE_MASTER_PING_PONG(wm). As a result
only write master 1 ever had its wm_done callback fired; every other
write master's interrupt was silently dropped, stalling any video
stream not routed through WM 1.

PATCH 2/2 moves the VFE_LINE_NONE check ahead of the output spinlock
in vfe_isr_wm_done(). Write masters allocated to statistics or other
secondary paths are not mapped to an output line; the existing code
took the spinlock before noticing this and printed a rate-limited
error each time. The fix returns silently before acquiring the lock,
eliminating harmless but noisy dmesg spam on such configurations.

Herman van Hazendonk (2):
  media: qcom: camss: vfe-17x: dispatch wm_done per write master
  media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

 .../media/platform/qcom/camss/camss-vfe-17x.c | 31 ++++++++++++++-----
 1 file changed, 23 insertions(+), 8 deletions(-)

-- 
2.43.0

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

[Herman van Hazendonk]

The wm_done dispatch loop checks a constant BIT(9) inside the
per-write-master iteration:

  for (wm = 0; wm < MSM_VFE_IMAGE_MASTERS_NUM; wm++)
          if (status0 & BIT(9))
                  if (vfe_bus_status[1] & STATUS1_WM_CLIENT_BUF_DONE(wm))
                          vfe->isr_ops.wm_done(vfe, wm);

BIT(9) is STATUS_0_IMAGE_MASTER_PING_PONG(1), so the outer check only
fires for write master 1. Any wm_done interrupt raised for a different
write master is dropped: the per-WM bit in vfe_bus_status[1] may be
set, but the gating status0 read does not match for that index and
isr_ops.wm_done() is never called. That stalls every video stream not
routed through WM 1.

Use the per-wm STATUS_0_IMAGE_MASTER_PING_PONG(wm) macro so the gate
tracks the loop iterator.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/media/platform/qcom/camss/camss-vfe-17x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
index e5ee7e717b3b..3ad0c3be005f 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))
+		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);
 
-- 
2.43.0

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

[sashiko-bot]

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

[PATCH 2/2] media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

[Herman van Hazendonk]

vfe_isr_wm_done() handles the per-write-master "frame complete" IRQ.
On VFE 1.7x configurations where a Write Master is allocated to a
secondary purpose (e.g. statistics or DMI buffers) it is not mapped
to any output line, so vfe->wm_output_map[wm] is VFE_LINE_NONE.

Today the function takes the output spinlock first and only then
checks for VFE_LINE_NONE, emitting a rate-limited "Received wm done
for unmapped index" error each time. The error is harmless but it
spams dmesg when a stats path is configured.

Move the VFE_LINE_NONE test in front of the spinlock and return
silently: an unmapped WM simply has nothing to deliver to userspace.
While at it, hoist the wm_output_map[] dereference into a local for
readability.

No functional change for mapped Write Masters.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 .../media/platform/qcom/camss/camss-vfe-17x.c | 29 ++++++++++++++-----
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
index 3ad0c3be005f..f639ac02410b 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
@@ -507,21 +507,36 @@ static void vfe_isr_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
  */
 static void vfe_isr_wm_done(struct vfe_device *vfe, u8 wm)
 {
-	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];
+	struct vfe_line *line;
 	struct camss_buffer *ready_buf;
 	struct vfe_output *output;
 	unsigned long flags;
+	int wm_output;
 	u32 index;
 	u64 ts = ktime_get_ns();
 
+	/*
+	 * Some VFE modes route data to secondary WMs that aren't mapped
+	 * to output lines. Silently ignore their IRQs.
+	 *
+	 * vfe->wm_output_map[wm] is written from the stream stop/start
+	 * paths under output_lock held as a mutex; this ISR runs in
+	 * atomic context and cannot take it. Snapshot the value once
+	 * with READ_ONCE() so the check below and the array index that
+	 * follows operate on the same value: otherwise a concurrent
+	 * write of VFE_LINE_NONE (-1) between the two loads would let
+	 * the function pass the check and then dereference
+	 * &vfe->line[-1].
+	 */
+	wm_output = READ_ONCE(vfe->wm_output_map[wm]);
+	if (wm_output == VFE_LINE_NONE)
+		return;
+
+	line = &vfe->line[wm_output];
+
 	spin_lock_irqsave(&vfe->output_lock, flags);
 
-	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
-		dev_err_ratelimited(vfe->camss->dev,
-				    "Received wm done for unmapped index\n");
-		goto out_unlock;
-	}
-	output = &vfe->line[vfe->wm_output_map[wm]].output;
+	output = &line->output;
 
 	ready_buf = output->buf[0];
 	if (!ready_buf) {
-- 
2.43.0

Re: [PATCH 2/2] media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

[sashiko-bot]

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

New issues:
- [High] Moving the `wm_output_map` validation outside the `output_lock` spinlock introduces a TOCTOU race condition against stream stopping, leading to Use-After-Free and double vb2 buffer completions.

Pre-existing issues:
- [High] Pre-existing bug in `vfe_buf_done` where an out-of-bounds line pointer is computed locklessly and later dereferenced.
--

commit 98cdb54eb2a601721b4658e4c2704449a2aa63dd
Author: Herman van Hazendonk <github.com@herrie.org>

media: qcom: camss: vfe-17x: silence wm-done IRQ on unmapped WMs

This commit aims to silence a rate-limited error message when handling
Write Master IRQs for unmapped indices. It moves the check for VFE_LINE_NONE
outside of the output_lock spinlock, intending to return early and avoid log
spam.

> vfe_isr_wm_done() handles the per-write-master "frame complete" IRQ.
> On VFE 1.7x configurations where a Write Master is allocated to a
> secondary purpose (e.g. statistics or DMI buffers) it is not mapped
> to any output line, so vfe->wm_output_map[wm] is VFE_LINE_NONE.
> 
> Today the function takes the output spinlock first and only then
> checks for VFE_LINE_NONE, emitting a rate-limited "Received wm done
> for unmapped index" error each time. The error is harmless but it
> spams dmesg when a stats path is configured.
> 
> Move the VFE_LINE_NONE test in front of the spinlock and return
> silently: an unmapped WM simply has nothing to deliver to userspace.
> While at it, hoist the wm_output_map[] dereference into a local for
> readability.
> 
> No functional change for mapped Write Masters.
> 
> Signed-off-by: Herman van Hazendonk <github.com@herrie.org>

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> --- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> @@ -507,21 +507,36 @@ static void vfe_isr_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
>   */
>  static void vfe_isr_wm_done(struct vfe_device *vfe, u8 wm)
>  {
> -	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];

[Severity: High]
This isn't a bug introduced by this patch, but a similar pattern exists in
vfe_buf_done() in drivers/media/platform/qcom/camss/camss-vfe.c:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_buf_done()
	struct vfe_line *line = &vfe->line[vfe->wm_output_map[wm]];
	...
	spin_lock_irqsave(&vfe->output_lock, flags);

	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
        ...

If wm_output_map[wm] is initially VFE_LINE_NONE (-1), line is assigned an
out-of-bounds pointer locklessly. If a concurrent thread maps the WM to a
valid index before vfe_buf_done() acquires output_lock, the internal check
passes. The function will then pass the stale, out-of-bounds line pointer
to ops->vfe_wm_update(), which dereferences it. Should this also be updated
to prevent reading from an out-of-bounds pointer?

> +	struct vfe_line *line;
>  	struct camss_buffer *ready_buf;
>  	struct vfe_output *output;
>  	unsigned long flags;
> +	int wm_output;
>  	u32 index;
>  	u64 ts = ktime_get_ns();
>  
> +	/*
> +	 * Some VFE modes route data to secondary WMs that aren't mapped
> +	 * to output lines. Silently ignore their IRQs.
> +	 *
> +	 * vfe->wm_output_map[wm] is written from the stream stop/start
> +	 * paths under output_lock held as a mutex; this ISR runs in
> +	 * atomic context and cannot take it. Snapshot the value once
> +	 * with READ_ONCE() so the check below and the array index that
> +	 * follows operate on the same value: otherwise a concurrent
> +	 * write of VFE_LINE_NONE (-1) between the two loads would let
> +	 * the function pass the check and then dereference
> +	 * &vfe->line[-1].
> +	 */
> +	wm_output = READ_ONCE(vfe->wm_output_map[wm]);
> +	if (wm_output == VFE_LINE_NONE)
> +		return;
> +
> +	line = &vfe->line[wm_output];
> +
>  	spin_lock_irqsave(&vfe->output_lock, flags);
>  
> -	if (vfe->wm_output_map[wm] == VFE_LINE_NONE) {
> -		dev_err_ratelimited(vfe->camss->dev,
> -				    "Received wm done for unmapped index\n");
> -		goto out_unlock;
> -	}
> -	output = &vfe->line[vfe->wm_output_map[wm]].output;
> +	output = &line->output;

[Severity: High]
Does moving this check outside the output_lock introduce a race condition with
stream stopping?

The new comment mentions the ISR runs in atomic context and cannot take the
lock, but the ISR proceeds to acquire output_lock immediately after this check.

If the ISR locklessly reads a valid wm_output here, but stalls before acquiring
the output_lock:

CPU1
vfe_isr_wm_done()
   wm_output = READ_ONCE(vfe->wm_output_map[wm]);

CPU2 (stream teardown)
vfe_put_output()
   spin_lock_irqsave(&vfe->output_lock, flags);
   vfe->wm_output_map[wm] = VFE_LINE_NONE;
   vfe_flush_buffers(vfe, &line->output);
   spin_unlock_irqrestore(&vfe->output_lock, flags);

CPU1
   spin_lock_irqsave(&vfe->output_lock, flags);
   output = &line->output;
   ready_buf = output->buf[0];

vfe_flush_buffers() completes all buffers via vb2_buffer_done() but leaves
output->buf[0] non-NULL. When CPU1 resumes and acquires the lock, it will no
longer re-verify wm_output_map. It then modifies the already completed buffer
(ready_buf->vb.vb2_buf.timestamp = ts) and double-completes it via
vb2_buffer_done(), which would corrupt the vb2 state.

>  	ready_buf = output->buf[0];
>  	if (!ready_buf) {

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