Re: [PATCH 4/6] accel/qaic: Use DRM_SCHED to handle FIFO submission

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] accel/qaic: Aborted jobs leave unsignaled hardware fences in dma_resv
- [Critical] accel/qaic: Resetting sequence numbers across slices overwrites fences
- [Critical] accel/qaic: Deadlock in decode_deactivate due to ignored interrupts
- [High] accel/qaic: Error rollback with mutex_lock_interruptible hangs BO
- [High] accel/qaic: Job credit overflow triggers DRM scheduler WARN_ON_ONCE
--

commit 188a88e259ba9f726c46ba938ac70127cfee6105
Author: Carl Vanderlip <carl.vanderlip@oss.qualcomm.com>

accel/qaic: Use DRM_SCHED to handle FIFO submission

This commit replaces the custom FIFO submission process with the DRM
scheduler, introducing a scheduler entity credit system to prevent overflow.
It calculates a scaling ratio to map variable FIFO sizes to a fixed credit
limit.

> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
[ ... ]
> @@ -919,7 +919,12 @@ static int decode_deactivate(struct qaic_device *qdev, void *trans, u32 *msg_len
>  		 * Releasing resources failed on the device side, which puts
>  		 * us in a bind since they may still be in use, so enable the
>  		 * dbc. User is expected to retry deactivation.
> +		 *
> +		 * Scheduler entity must be destroyed before it's reinitialized
> +		 * in enable_dbc, otherwise entity list element points to itself
> +		 * and causes a cycle in any list it was a node of.
>  		 */
> +		drm_sched_entity_destroy(&qdev->dbc[dbc_id].sched_entity);
>  		enable_dbc(qdev, dbc_id, usr);
>  		return -ECANCELED;

[Severity: Critical]
Can this synchronous destroy call cause a deadlock?

When shutting down a user context, the driver first calls disable_dbc(),
which sets dbc->usr = NULL. If the device replies with an error status here,
we call drm_sched_entity_destroy() which blocks waiting for all pending jobs
to complete.

However, since dbc->usr is already NULL, the hardware interrupt handler
dbc_irq_threaded_fn() will skip processing and ignore all completions, which
deadlocks the control workqueue forever.

> diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> --- a/drivers/accel/qaic/qaic_data.c
> +++ b/drivers/accel/qaic/qaic_data.c
[ ... ]
> -static inline int copy_partial_exec_reqs(struct bo_slice *slice, u64 resize)
> +static int create_slice_jobs(struct drm_file *file_priv, struct bo_slice *slice, bool partial,
> +			     u64 resize, struct list_head *tmp_list)
>  {
[ ... ]
> -	ret = qaic_submit_reqs_to_hw(slice, nents_xfer, true, last_bytes);
> +	if (partial)
> +		nents_xfer = first_n + 1;
> +	else
> +		nents_xfer = slice->nents;
>  
> -	return ret;
> +	job = qaic_create_job(file_priv, slice, nents_xfer, 0, tmp_list);

[Severity: Critical]
Is it safe to hardcode 0 for the sequence number across multiple slices?

Because all slices of a BO share the exact same bo->fence_context,
dma_resv_add_fence() sees subsequent slice fences as having the same context
and an overlapping sequence number (e.g., 0 >= 0). The dma_resv logic
therefore assumes they are on the same timeline and overwrites the earlier
slice's fence, which could allow userspace or other drivers to access the
buffer while earlier slices are still undergoing DMA.

[ ... ]
> @@ -1290,13 +1232,7 @@ static int send_bo_list_to_sched(...)
[ ... ]
>  unlock_bo:
> -	drm_gem_object_put(&bo->base);
>  	mutex_unlock(&bo->lock);
> -failed_to_send_bo:
> -	for (j = 0; j < i; j++) {
> -		drm_gem_object_put(&exec->bo_arr[j]->base);
> -		spin_lock_irqsave(&dbc->xfer_lock, flags);
> -		bo = list_last_entry(&dbc->xfer_list, struct qaic_bo, xfer_list);
> -		list_del_init(&bo->xfer_list);
> -		spin_unlock_irqrestore(&dbc->xfer_lock, flags);
> -		dma_sync_sgtable_for_cpu(&qdev->pdev->dev, bo->sgt, bo->dir);
> +free_jobs:
> +	for (j = i - 1; i > 0 && j >= 0; j--) {
> +		bo = exec->bo_arr[j];
> +		ret = mutex_lock_interruptible(&bo->lock);
> +		if (!ret) {
> +			bo->need_dev_sync = false;
> +			dma_fence_put(bo->fence);
> +			bo->fence = NULL;
> +			mutex_unlock(&bo->lock);
> +		}

[Severity: High]
Does using an interruptible lock in this error rollback path risk permanently
deadlocking the buffer object?

If a thread has a pending signal, the lock acquisition fails and this
critical cleanup block is skipped. Failing to perform this rollback leaves
the BO permanently waiting on jobs that will never be scheduled.

>  	}
> +	list_for_each_entry_safe_reverse(job_i, tmp, &tmp_list, queue)
> +		qaic_cleanup_job(job_i);
>  unlock_resv:
>  	drm_gem_unlock_reservations((struct drm_gem_object **)exec->bo_arr,
>  				    exec->count, &acquire_ctx);

[Severity: Critical]
Does this error path leak unsignaled hardware fences into the global
dma_resv object?

The internal hardware irq_fence is extracted from these jobs and added
directly to the BO's dma_resv during qaic_create_bo_fence(). If we jump to
free_jobs and drop these jobs here using qaic_cleanup_job(), their
irq_fence is never signaled, which could permanently deadlock the buffer
object.

Similarly, if the DRM scheduler fails to execute the job, it signals its
software s_fence->finished but leaves the hardware irq_fence unsignaled.

> diff --git a/drivers/accel/qaic/qaic_sched.c b/drivers/accel/qaic/qaic_sched.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_sched.c
[ ... ]
> +void set_dbc_scaling_ratio(struct dma_bridge_chan *dbc, u32 nelem)
> +{
> +	if (!nelem) {
[ ... ]
> +		dbc->credit_ratio = QAIC_CREDITS;
> +		return;
> +	}
> +	if (QAIC_CREDITS % nelem != 0)
> +		pr_debug("Credits not evenly divisible by queue size: size = %d\n", nelem);
> +	dbc->credit_ratio = DIV_ROUND_UP(QAIC_CREDITS, nelem);
> +}
[ ... ]
> +struct qaic_job *qaic_create_job(struct drm_file *file_priv, struct bo_slice *slice,
> +				 unsigned int num_req, u64 seq_no, struct list_head *tmp_list)
> +{
[ ... ]
> +	u32 credits;
[ ... ]
> +
> +	if (check_mul_overflow((u32)num_req, dbc->credit_ratio, &credits))
> +		return ERR_PTR(-EINVAL);

[Severity: High]
Can this math result in credits exceeding the DRM scheduler's limit?

By rounding up the scaling ratio, (nelem - 1) * credit_ratio can exceed
QAIC_CREDITS - 1. For example, if nelem = 5456, ratio = 13, and num_req =
5455, the required credits evaluate to 70915, exceeding the 65535 limit.

While check_mul_overflow() prevents 32-bit integer overflow, it does not
verify if credits exceeds the DRM scheduler's static credit_limit.
Exceeding this limit will trip the WARN_ON_ONCE in drm_sched_can_queue()
and permanently stall the entity queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003023818.1161666-1-carl.vanderlip@oss.qualcomm.com?part=4