Re: [PATCH v2] media: rkvdec: Handle interrupts without a current context

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 6329 bytes --]

Hi,

Le lundi 26 octobre 2026 à 16:48 +0400, Alex Kazinsky a écrit :
> The watchdog can finish the current job before the threaded IRQ handler
> retrieves its context. In that case, v4l2_m2m_get_curr_priv() returns NULL,
> but the variant IRQ handlers unconditionally dereference ctx->dev.
> 
> Pass the device pointer to the variant IRQ handlers independently of the
> context. Clear the interrupt status and, for VDPU383, disable interrupts
> before returning if no current context exists. This avoids dereferencing
> NULL while still acknowledging the interrupt.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: f9c7b7deeffd ("media: rkvdec: Support per-variant interrupt handler")
> Signed-off-by: Alex Kazinsky <a.kazinsky@ideco.ru>
> ---
> Link to v1: https://lore.kernel.org/all/20261023131124.94116-1-a.kazinsky@ideco.ru/
> 
> This issue has not been reproduced on hardware. The watchdog/IRQ race
> described above is based on code analysis.
> 
> A possible test would be to submit a single decoding job and temporarily
> delay the IRQ thread before v4l2_m2m_get_curr_priv() for longer than the
> watchdog timeout. This would artificially widen the suspected race window.
> This experiment has not been performed.

Ack. I've studied Hantro and why it does not need all your suggested defensive
coding, details delow.

> 
> Changes in v2:
> - Fix continuation-line alignment in the IRQ handler declarations.
> - Clarify that the issue has not been reproduced on hardware and
>   describe a possible test.
> 
>  .../media/platform/rockchip/rkvdec/rkvdec.c   | 23 +++++++++++++------
>  .../media/platform/rockchip/rkvdec/rkvdec.h   |  4 +++-
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index 061281f903f3..da4ed7bde05c 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -1434,15 +1434,18 @@ static void rkvdec_iommu_restore(struct rkvdec_dev *rkvdec)
>  	}
>  }
>  
> -static irqreturn_t rk3399_irq_handler(struct rkvdec_ctx *ctx)
> +static irqreturn_t rk3399_irq_handler(struct rkvdec_dev *rkvdec,
> +				      struct rkvdec_ctx *ctx)
>  {
> -	struct rkvdec_dev *rkvdec = ctx->dev;
>  	enum vb2_buffer_state state;
>  	u32 status;
>  
>  	status = readl(rkvdec->regs + RKVDEC_REG_INTERRUPT);
>  	writel(0, rkvdec->regs + RKVDEC_REG_INTERRUPT);
>  
> +	if (!ctx)
> +		return IRQ_HANDLED;
> +

The IRQ lifetime is bound to the dev, so it fine to continue further.

>  	if (status & RKVDEC_RDY_STA) {
>  		state = VB2_BUF_STATE_DONE;
>  	} else {

What is not in the patch context here is:

	if (cancel_delayed_work(&rkvdec->watchdog_work))
		rkvdec_job_finish(ctx, state);

ctx will exist if the watchdog was cancelled, this is pretty much guarantied, so
the null-check-return is not needed. It does not invalidate your report though,
since we get the dev from the ctx, which can be dead. More instructions below
....

> @@ -1457,9 +1460,9 @@ static irqreturn_t rk3399_irq_handler(struct rkvdec_ctx *ctx)
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t vdpu381_irq_handler(struct rkvdec_ctx *ctx)
> +static irqreturn_t vdpu381_irq_handler(struct rkvdec_dev *rkvdec,
> +				       struct rkvdec_ctx *ctx)
>  {
> -	struct rkvdec_dev *rkvdec = ctx->dev;
>  	enum vb2_buffer_state state;
>  	bool need_reset = 0;
>  	u32 status;
> @@ -1467,6 +1470,9 @@ static irqreturn_t vdpu381_irq_handler(struct rkvdec_ctx *ctx)
>  	status = readl(rkvdec->regs + VDPU381_REG_STA_INT);
>  	writel(0, rkvdec->regs + VDPU381_REG_STA_INT);
>  
> +	if (!ctx)
> +		return IRQ_HANDLED;
> +
>  	if (status & VDPU381_STA_INT_DEC_RDY_STA) {
>  		state = VB2_BUF_STATE_DONE;
>  	} else {
> @@ -1486,9 +1492,9 @@ static irqreturn_t vdpu381_irq_handler(struct rkvdec_ctx *ctx)
>  	return IRQ_HANDLED;
>  }
>  
> -static irqreturn_t vdpu383_irq_handler(struct rkvdec_ctx *ctx)
> +static irqreturn_t vdpu383_irq_handler(struct rkvdec_dev *rkvdec,
> +				       struct rkvdec_ctx *ctx)
>  {
> -	struct rkvdec_dev *rkvdec = ctx->dev;
>  	enum vb2_buffer_state state;
>  	bool need_reset = 0;
>  	u32 status;
> @@ -1499,6 +1505,9 @@ static irqreturn_t vdpu383_irq_handler(struct rkvdec_ctx *ctx)
>  	writel(FIELD_PREP_WM16(VDPU383_INT_EN_IRQ | VDPU383_INT_EN_LINE_IRQ, 0),
>  	       rkvdec->link + VDPU383_LINK_INT_EN);
>  
> +	if (!ctx)
> +		return IRQ_HANDLED;
> +
>  	if (status & VDPU383_STA_INT_DEC_RDY_STA) {
>  		state = VB2_BUF_STATE_DONE;
>  	} else {
> @@ -1521,7 +1530,7 @@ static irqreturn_t rkvdec_irq_handler(int irq, void *priv)
>  	struct rkvdec_ctx *ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);
>  	const struct rkvdec_variant *variant = rkvdec->variant;
>  
> -	return variant->ops->irq_handler(ctx);
> +	return variant->ops->irq_handler(rkvdec, ctx);
>  }
>  
>  /*
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.h b/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> index a24be6638b6b..8bcb51db5105 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.h
> @@ -30,6 +30,7 @@
>  #define RKVDEC_8K_PIXELS		(7680 * 4320)
>  
>  struct rkvdec_ctx;
> +struct rkvdec_dev;
>  struct rkvdec_rcb_config;
>  
>  struct rkvdec_ctrl_desc {
> @@ -72,7 +73,8 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>  }
>  
>  struct rkvdec_variant_ops {
> -	irqreturn_t (*irq_handler)(struct rkvdec_ctx *ctx);
> +	irqreturn_t (*irq_handler)(struct rkvdec_dev *rkvdec,
> +				   struct rkvdec_ctx *ctx);

What I'd do is only pass rkvdec_dev, which has a lifetime bound to the IRQ
handler. Then inside rkvdec_job_finish(), retrieve the CTX with:

	struct rkvdec_ctx *ctx = v4l2_m2m_get_curr_priv(rkvdec->m2m_dev);

This will match what we do in Hantro driver, and should be safe, with less
defensive coding. In short, the ctx lifetime is bound to the m2m job, at least
it should, if not, we have another bug to fix.

Nicolas

>  	u32 (*colmv_size)(u16 width, u16 height);
>  	void (*flatten_matrices)(u8 *output, const u8 *input, int matrices, int row_length);
>  };

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]