Re: [PATCH v5] drm/imagination: Propagate all errors from KCCB command submission code

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unrecoverable queue state, CCCB corruption, and Use-After-Free when a KCCB kick fails in pvr_queue_run_job().
- [High] KCCB slot reservation leak in pvr_kccb_send_cmd_powered() on error.
- [Medium] WARN_ON(err) triggers continuously when the device is lost, flooding dmesg.

Pre-existing issues:
- [High] Paired fragment job hangs indefinitely when the geometry job fails to acquire PM reference or fails the KCCB kick.
- [High] Kernel's CCCB `write_offset` is not reset during a GPU hard reset, causing immediate firmware faults on subsequent submissions.
--

commit 71ee1590b98c30900f70c9456403e8740176ca27
Author: Alessio Belle <alessio.belle@imgtec.com>

drm/imagination: Propagate all errors from KCCB command submission code

This commit updates the KCCB kick command variants to propagate errors
up to the DRM scheduler's run_job callback. It introduces error checking
in pvr_queue_run_job and early returns for lost devices.

Link: https://patch.msgid.link/20260907-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v4-1-b684f58a06ce@imgtec.com

> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index b702d122d7918..dfd0682705425 100644
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
> @@ -268,19 +273,25 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
>  	struct rogue_fwif_ccb_ctl *ctrl = pvr_ccb->ctrl;
>  	u32 old_write_offset;
>  	u32 new_write_offset;
> +	int err;
>  
> -	WARN_ON(pvr_dev->lost);
> +	if (pvr_dev->lost)
> +		return -EIO;

[Severity: High]
Does this early return cause a permanent resource leak of KCCB slot
reservations?

When the device is lost, this returns -EIO directly and bypasses the success
path where pvr_dev->kccb.reserved_count-- is decremented.

Is a call to pvr_kccb_release_slot() needed here or in the caller to undo
the reservation?

[ ... ]
> @@ -378,8 +396,7 @@ pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev, struct rogue_fwif_kccb_cmd
>  	if (err)
>  		return err;
>  
> -	pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
> -	return 0;
> +	return pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);

[Severity: High]
Similarly, if pvr_kccb_send_cmd_reserved_powered() fails, does this direct
return propagate the error without releasing the slot reserved earlier in this
function by pvr_kccb_reserve_slot_sync()?

[ ... ]
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
> index 09993e858df8b..8aba8fbfccfa4 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
[ ... ]
>  	if (job->paired_job) {
>  		err = pvr_job_get_pm_ref(job->paired_job);
>  		if (WARN_ON(err))
> -			return ERR_PTR(err);
> +			goto err_release;

[Severity: High]
This is a pre-existing issue, but can an early failure here (like failing to
acquire the PM ref) or during a KCCB kick failure cause a paired fragment job
to hang indefinitely? 

The geometry job would abort and be completed with an error by the DRM
scheduler, but it doesn't appear to clean up or error out the paired fragment
job. 

Since the combined firmware submission never occurred, the fragment queue
might stall indefinitely, eventually triggering a full GPU reset.

[ ... ]
> @@ -793,25 +795,37 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
>  
>  		/* Submit the fragment job along the geometry job and send a combined kick. */
>  		pvr_queue_submit_job_to_cccb(frag_job);
> -		pvr_cccb_send_kccb_combined_kick(pvr_dev,
> -						 &geom_queue->cccb, &frag_queue->cccb,
> -						 pvr_context_get_fw_addr(geom_job->ctx) +
> -						 geom_queue->ctx_offset,
> -						 pvr_context_get_fw_addr(frag_job->ctx) +
> -						 frag_queue->ctx_offset,
> -						 job->hwrt,
> -						 frag_job->fw_ccb_cmd_type ==
> -						 ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
> +		err = pvr_cccb_send_kccb_combined_kick(pvr_dev,
> +						       &geom_queue->cccb, &frag_queue->cccb,
> +						       pvr_context_get_fw_addr(geom_job->ctx) +
> +						       geom_queue->ctx_offset,
> +						       pvr_context_get_fw_addr(frag_job->ctx) +
> +						       frag_queue->ctx_offset,
> +						       job->hwrt,
> +						       frag_job->fw_ccb_cmd_type ==
> +						       ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
>  	} else {
>  		struct pvr_queue *queue = container_of(job->base.sched,
>  						       struct pvr_queue, scheduler);
>  
> -		pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> -					pvr_context_get_fw_addr(job->ctx) + queue->ctx_offset,
> -					job->hwrt);
> +		err = pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> +					      pvr_context_get_fw_addr(job->ctx) +
> +					      queue->ctx_offset,
> +					      job->hwrt);
> +	}
> +
> +	if (WARN_ON(err)) {

[Severity: Critical]
If a KCCB kick fails (e.g., due to a lost device), does taking this error path
leave the queue in an unrecoverable state?

Before sending the kick, pvr_queue_submit_job_to_cccb() is called, which writes
the job commands to the CCCB and advances the write offset. 

If the kick fails and the error is propagated to the DRM scheduler, the job's
memory backing will be freed. However, because the CCCB write_offset and
in_flight_job_count are not rolled back, the aborted commands remain pending in
the hardware queue. 

Can this lead to CCCB corruption and a use-after-free by the firmware?

[Severity: Medium]
Additionally, does WARN_ON(err) cause excessive dmesg spam?

When the device is lost, pvr_kccb_send_cmd_reserved_powered() legitimately
returns -EIO. If the DRM scheduler subsequently attempts to process queued jobs,
each one will fail with -EIO and trigger a full stack trace warning.

[Severity: High]
This is a pre-existing issue, but does a GPU hard reset break future submissions
because the kernel's CCCB write_offset is not reset?

During a hard reset, the firmware's control offsets are reset to 0 (e.g., in
cccb_ctrl_init(), WRITE_ONCE(ctrl->write_offset, 0)). However, the kernel's
local pvr_cccb->write_offset is never synchronized to match. 

On the next submission, the kernel writes at the old offset, but the firmware
expects commands at offset 0, which might contain zero-filled memory, leading
to immediate firmware faults.

> +		pvr_job_release_pm_ref(job);
> +		if (job->paired_job)
> +			pvr_job_release_pm_ref(job->paired_job);
> +		goto err_release;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v5-1-5e262721454b@imgtec.com?part=1