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 <[email protected]> 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/[email protected]?part=4
