[PATCH v2 5/9] media: chips-media: wave5: finish a job only once

[Jackson.lee]

From: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>

v4l2_m2m_job_finish() checks that the context owns the current job, but
not which job it is, so a second finish from the same context clears
m2m_dev->curr_ctx all the same. v4l2_m2m_try_run() picks the next job
under job_spinlock, drops it, and only then reads curr_ctx again to hand
its priv to device_run(). A stray finish landing in that window makes
that read return NULL and the decoder oopses:

  Unable to handle kernel NULL pointer dereference at virtual address
  0000000000000358
  Workqueue: events v4l2_m2m_device_run_work [v4l2_mem2mem]
  pc : v4l2_m2m_try_run+0x78/0x140 [v4l2_mem2mem]

The decoder finishes jobs from seven places: device_run() itself, the
completion IRQ, and job_abort(). None of them tracked whether a job was
still outstanding, so the same job was finished twice whenever the IRQ
and device_run() both reached the end. Instrumenting the call sites over
150 s of a three-stream run counted 63 finishes with no job to finish,
two of them a plain double finish.

Take ownership of the slot once per device_run() and let only the winner
call in; whoever loses the race now returns without touching the shared
state. Also compare v4l2_m2m_get_curr_priv() against this instance rather
than testing it for NULL, which let one instance finish another's job.

job_abort() must not end a job that device_run() is still running or is
about to run. v4l2_m2m_try_run() sets TRANS_RUNNING before it calls
device_run(), so v4l2_m2m_cancel_job() can get there first, and a finish
from job_abort() then releases cancel_job()'s wait early: try_run()
reads a curr_ctx that is already NULL, or release() frees the instance
under device_run(). Hold a per-instance run_lock across device_run() and
take it in job_abort(), which then finishes through the same ownership
check. The only job left for it to end is one device_run() handed over
to the DEC_PIC completion after EOS; device_run() finishes every other
one itself. job_abort() no longer returns early on a failed state
switch, so that held job is still ended and streamoff cannot hang on it.

start_decode() no longer finishes the job when it fails: device_run()
does that on its way out, as for any run that queued no DEC_PIC.

Fixes: a176ac5e701f ("media: chips-media: wave5: Improve performance of decoder")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
---
 .../chips-media/wave5/wave5-vpu-dec.c         | 93 ++++++++++++++-----
 .../platform/chips-media/wave5/wave5-vpuapi.c |  1 +
 .../platform/chips-media/wave5/wave5-vpuapi.h |  3 +
 3 files changed, 73 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index a314813a6b4d..3ecf535f5622 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -102,6 +102,29 @@ static const struct vpu_format dec_fmt_list[FMT_TYPES][MAX_FMTS] = {
 	}
 };
 
+/* bit in vpu_instance.job_flags */
+#define WAVE5_JOB_RUNNING	0
+
+/*
+ * Hand the shared job slot back, exactly once per device_run().
+ *
+ * v4l2_m2m_job_finish() only checks that the *context* owns the current job,
+ * not which job it is, so a second finish from the same context clears
+ * m2m_dev->curr_ctx -- and it may do so for the job v4l2_m2m_try_run() has just
+ * put there and is about to hand to device_run(), which then dereferences NULL.
+ *
+ * device_run(), the completion IRQ and job_abort() all reach for this and any
+ * of them may get there first, so ownership is taken atomically and only the
+ * winner calls in.
+ */
+static void wave5_dec_finish_job(struct vpu_instance *inst)
+{
+	if (!test_and_clear_bit(WAVE5_JOB_RUNNING, &inst->job_flags))
+		return;
+
+	v4l2_m2m_job_finish(inst->v4l2_m2m_dev, inst->v4l2_fh.m2m_ctx);
+}
+
 /*
  * Make sure that the state switch is allowed and add logging for debugging
  * purposes
@@ -245,8 +268,7 @@ static int start_decode(struct vpu_instance *inst, u32 *fail_res)
 		mutex_unlock(&inst->feed_lock);
 		set_instance_state(inst, VPU_INST_STATE_STOP);
 
-		dev_dbg(inst->dev->dev, "%s: pic run failed / finish job", __func__);
-		v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		dev_dbg(inst->dev->dev, "%s: pic run failed", __func__);
 	}
 
 	return ret;
@@ -375,7 +397,7 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
 	ret = wave5_vpu_dec_get_output_info(inst, &dec_info);
 	if (ret) {
 		dev_dbg(inst->dev->dev, "%s: could not get output info.", __func__);
-		v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		wave5_dec_finish_job(inst);
 		return;
 	}
 
@@ -388,7 +410,7 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
 
 	if (!vb2_is_streaming(dst_vq)) {
 		dev_dbg(inst->dev->dev, "%s: capture is not streaming..", __func__);
-		v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		wave5_dec_finish_job(inst);
 		return;
 	}
 
@@ -469,13 +491,13 @@ static void wave5_vpu_dec_finish_decode(struct vpu_instance *inst)
 	}
 
 	if (inst->sent_eos &&
-	    v4l2_m2m_get_curr_priv(inst->v4l2_m2m_dev)) {
+	    v4l2_m2m_get_curr_priv(inst->v4l2_m2m_dev) == inst) {
 		struct queue_status_info q_status;
 
 		wave5_vpu_dec_give_command(inst, DEC_GET_QUEUE_STATUS, &q_status);
 		if (q_status.report_queue_count == 0 &&
 		    q_status.instance_queue_count == 0)
-			v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+			wave5_dec_finish_job(inst);
 	}
 
 	if (inst->queuing_fail) {
@@ -1685,6 +1707,9 @@ static void wave5_vpu_dec_device_run(void *priv)
 	int ret = 0;
 	bool cmd_issued = false;
 
+	mutex_lock(&inst->run_lock);
+	set_bit(WAVE5_JOB_RUNNING, &inst->job_flags);
+
 	dev_dbg(inst->dev->dev, "%s: Fill the ring buffer with new bitstream data", __func__);
 	pm_runtime_resume_and_get(inst->dev->dev);
 	if (!inst->retry) {
@@ -1809,36 +1834,55 @@ static void wave5_vpu_dec_device_run(void *priv)
 	 * stalling every instance sharing the VPU.
 	 */
 	if (!inst->sent_eos || !cmd_issued)
-		v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		wave5_dec_finish_job(inst);
+	mutex_unlock(&inst->run_lock);
 }
 
 static void wave5_vpu_dec_job_abort(void *priv)
 {
 	struct vpu_instance *inst = priv;
-	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
 	int ret;
 
-	ret = set_instance_state(inst, VPU_INST_STATE_STOP);
-	if (ret)
-		return;
-
 	/*
-	 * job_abort() runs from the STREAMOFF path and may be called while the
-	 * device is runtime suspended. Setting the EOS flag talks to the
-	 * firmware (send_firmware_command() accesses VPU registers), so the
-	 * device must be resumed first; otherwise the register access faults
-	 * with an asynchronous SError.
+	 * v4l2_m2m_cancel_job() calls this while the context owns the running
+	 * job and then sleeps until a finish clears TRANS_RUNNING. Ending the
+	 * job from here while device_run() is still executing -- or is about
+	 * to be entered, v4l2_m2m_try_run() having set TRANS_RUNNING already --
+	 * releases that wait early: try_run() then reads a curr_ctx that is
+	 * already NULL, or release() frees the instance under device_run().
+	 * So wait for device_run() to return first.
 	 */
-	pm_runtime_resume_and_get(inst->dev->dev);
+	mutex_lock(&inst->run_lock);
 
-	ret = wave5_vpu_dec_set_eos_on_firmware(inst);
-	if (ret)
-		dev_warn(inst->dev->dev,
-			 "Setting EOS for the bitstream, fail: %d\n", ret);
+	ret = set_instance_state(inst, VPU_INST_STATE_STOP);
 
-	pm_runtime_put_autosuspend(inst->dev->dev);
+	if (!ret) {
+		/*
+		 * job_abort() runs from the STREAMOFF path and may be called while the
+		 * device is runtime suspended. Setting the EOS flag talks to the
+		 * firmware (send_firmware_command() accesses VPU registers), so the
+		 * device must be resumed first; otherwise the register access faults
+		 * with an asynchronous SError.
+		 */
+		pm_runtime_resume_and_get(inst->dev->dev);
 
-	v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
+		ret = wave5_vpu_dec_set_eos_on_firmware(inst);
+		if (ret)
+			dev_warn(inst->dev->dev,
+				 "Setting EOS for the bitstream, fail: %d\n", ret);
+
+		pm_runtime_put_autosuspend(inst->dev->dev);
+	}
+
+	/*
+	 * With device_run() out of the way, the job is either already finished,
+	 * still to be run -- device_run() then finds the instance stopped,
+	 * issues nothing and finishes it itself -- or held for a DEC_PIC
+	 * completion after EOS. Only the last one is left for us: end it now
+	 * rather than leave streamoff or release() waiting on that IRQ.
+	 */
+	wave5_dec_finish_job(inst);
+	mutex_unlock(&inst->run_lock);
 }
 
 static int wave5_vpu_dec_job_ready(void *priv)
@@ -1920,6 +1964,7 @@ static int wave5_vpu_open_dec(struct file *filp)
 
 	spin_lock_init(&inst->state_spinlock);
 	mutex_init(&inst->feed_lock);
+	mutex_init(&inst->run_lock);
 	INIT_LIST_HEAD(&inst->avail_src_bufs);
 
 	inst->codec_info = kzalloc_obj(*inst->codec_info);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index 397da200a9ed..c7e61e2b78eb 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -286,6 +286,7 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
 	wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);
 
 	mutex_destroy(&inst->feed_lock);
+	mutex_destroy(&inst->run_lock);
 
 unlock_and_return:
 	mutex_unlock(&vpu_dev->hw_lock);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 7b08fef58217..2d515c9028a5 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -826,6 +826,9 @@ struct vpu_instance {
 	struct mutex feed_lock; /* lock for feeding bitstream buffers */
 	bool queuing_fail; /* if there is the queuing failure */
 	bool empty_queue;
+	unsigned long job_flags; /* bit 0: a device_run() job is ours to finish */
+	struct mutex run_lock; /* held across device_run(); job_abort() waits on it */
+	struct delayed_work unstall_work; /* releases a stale empty_queue park */
 	struct vpu_buf bitstream_vbuf;
 	dma_addr_t last_rd_ptr;
 	size_t remaining_consumed_bytes;
-- 
2.43.0