Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [High] drm/msm/a6xx: IFPC vote leak on early return in a6xx_get_ctx_timestamp() - [High] drm/msm/a6xx: wrong ring and skipped sysprof update in a6xx_preempt_trigger() - [High] drm/msm/adreno: wrong CP_ALWAYS_ON_CONTEXT register for A8xx in a7xx_submit() - [High] drm/msm/a6xx: missing last_job_start_ctx update in a6xx_submit() - [High] drm/msm/a6xx: uninitialized eval_lock in a6xx_get_ctx_timestamp() - [High] drm/msm/a8xx: uninitialized gpu->hangcheck_timer used in A8xx IRQ handlers - [High] drm/msm: missing hangcheck_timer and hangcheck_work teardown on cleanup - [High] drm/msm: stale context timestamps in check_ring_timeout() cause false hangs - [High] drm/msm: unclocked register read in a6xx_get_ctx_timestamp() when suspended - [Medium] drm/msm/a6xx: uninitialized variable in preempt_prepare_postamble() - [Medium] drm/msm: wrong clock multiplier and 32-bit overflow in hangcheck_worker() - [Low] drm/msm: unused variables in a6xx_preempt_timer() and check_ring_timeout() --
commit ca7e03133676b7e2984c48917b8dc917a117cdfe Author: Anna Maniscalco <[email protected]> drm/msm: preemption aware hangcheck This commit reworks the hangcheck logic to be preemption-aware by moving the hangcheck timer and worker to each ringbuffer and tracking the per-ring time spent on the GPU using the CP_ALWAYS_ON_CONTEXT register. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 92e8c8abb86da..bcb177a99c677 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -519,6 +519,9 @@ static void a7xx_submit(struct msm_gpu *gpu, struct > msm_gem_submit *submit) > get_stats_counter(ring, rbbm_perfctr_cp0, rbmemptr_stats(ring, index, > cpcycles_start)); > get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, > index, alwayson_start)); > > + get_stats_counter(ring, REG_A6XX_CP_ALWAYS_ON_CONTEXT, > + rbmemptr(ring, last_job_start_ctx)); [Severity: High] Since a7xx_submit() is shared between A7xx and A8xx GPUs and selects cp_always_on_context just above: if (adreno_is_a8xx(adreno_gpu)) { rbbm_perfctr_cp0 = REG_A8XX_RBBM_PERFCTR_CP(0); cp_always_on_context = REG_A8XX_CP_ALWAYS_ON_CONTEXT; } else { rbbm_perfctr_cp0 = REG_A7XX_RBBM_PERFCTR_CP(0); cp_always_on_context = REG_A6XX_CP_ALWAYS_ON_CONTEXT; } will passing REG_A6XX_CP_ALWAYS_ON_CONTEXT (0x0982) instead of cp_always_on_context here trigger a CP register protection fault on A8xx, where 0x0982 falls inside the NORDWR protected range (0x08ec..0x09af) in a830_protect_regs and a850_protect_regs? [ ... ] > @@ -2297,6 +2294,43 @@ static u64 a6xx_get_timestamp(struct msm_gpu *gpu) > return gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER); > } > > +static int a6xx_get_ctx_timestamp(struct msm_ringbuffer *ring, uint64_t > *value) > +{ > + struct msm_gpu *gpu = ring->gpu; > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > + int ret = 0; > + > + /* Inhibit IFPC while accessing registers: */ > + if (gpu->funcs->sysprof_setup) > + gpu->funcs->sysprof_setup(gpu, true); > + > + /* > + * a6xx_gmu_set_oob cannot be called while holding a spinlock. > + * Limit the scope. > + */ > + { > + guard(spinlock_irqsave)(&a6xx_gpu->eval_lock); > + > + /* > + * Make sure the GMU keeps the GPU on while we read the register > + * > + * This should normally not be waking up the gpu since the > + * hangcheck timer shouldn't fire when the gpu is idle. > + */ > + if (a6xx_in_preempt(a6xx_gpu) || ring != a6xx_gpu->cur_ring) > + return -EBUSY; > + > + *value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_CONTEXT); > + } > + > + /* Re-enable IFPC: */ > + if (gpu->funcs->sysprof_setup) > + gpu->funcs->sysprof_setup(gpu, false); > + > + return ret; > +} [Severity: High] Does returning -EBUSY directly from inside the guarded block leak the GMU_OOB_PERFCOUNTER_SET vote and GMU_STATUS_OOB_PERF_SET status bit? When a6xx_in_preempt(a6xx_gpu) is true or ring != a6xx_gpu->cur_ring, the early return skips the paired gpu->funcs->sysprof_setup(gpu, false) call at the end of a6xx_get_ctx_timestamp(), leaving IFPC permanently inhibited. Also, is a6xx_gpu->eval_lock guaranteed to be initialized when a6xx_get_ctx_timestamp() acquires it? In a6xx_preempt_hw_init(): /* No preemption if we only have one ring */ if (gpu->nr_rings == 1) return; ... spin_lock_init(&a6xx_gpu->eval_lock); eval_lock is never initialized when gpu->nr_rings == 1 (such as on A6xx GPUs or single-ring A7xx GPUs), and when gpu->nr_rings > 1, a6xx_preempt_hw_init() re-initializes eval_lock on every hw_init while hangcheck_worker() can acquire it without holding gpu->lock. Finally, can a6xx_get_ctx_timestamp() perform an unclocked register read of REG_A6XX_CP_ALWAYS_ON_CONTEXT when the GPU is runtime-suspended or mid-resume? hangcheck_worker() runs without holding gpu->lock or a runtime PM reference. When a job is retired, ring->hangcheck_timer remains armed, and in msm_job_run(), msm_fence_init() increments fctx->last_fence before acquiring gpu->lock and before msm_gpu_submit() calls pm_runtime_get_sync(). In a6xx_gmu_sysprof_setup(): if (!pm_runtime_get_if_active(&gpu->pdev->dev)) return; ... pm_runtime_put(&gpu->pdev->dev); if the GPU is suspended, sysprof_setup() silently returns without waking the hardware (and on a6xx_gmuwrapper_funcs, sysprof_setup is NULL), allowing gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_CONTEXT) to execute while GX is powered off. [ ... ] > @@ -2825,6 +2859,7 @@ const struct adreno_gpu_funcs a6xx_gpu_funcs = { > .progress = a6xx_progress, > .sysprof_setup = a6xx_gmu_sysprof_setup, > .perfcntr_configure = a6xx_perfcntr_configure, > + .get_ctx_timestamp = a6xx_get_ctx_timestamp, > }, > .init = a6xx_gpu_init, > .get_timestamp = a6xx_gmu_get_timestamp, > @@ -2859,6 +2894,7 @@ const struct adreno_gpu_funcs a6xx_gmuwrapper_funcs = { > .get_rptr = a6xx_get_rptr, > .progress = a6xx_progress, > .perfcntr_configure = a6xx_perfcntr_configure, > + .get_ctx_timestamp = a6xx_get_ctx_timestamp, > }, [Severity: High] Both a6xx_gpu_funcs and a6xx_gmuwrapper_funcs wire up .get_ctx_timestamp = a6xx_get_ctx_timestamp while using .submit = a6xx_submit. Since a6xx_submit() was not updated to emit get_stats_counter() for last_job_start_ctx, won't ring->memptrs->last_job_start_ctx remain 0 on all A6xx GPUs, causing check_ring_timeout() to compare cumulative context cycles since power-on against 0 and falsely trigger hang recovery? > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > index 1e599d4ddea1c..263fec8e6e9bf 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_preempt.c > @@ -20,15 +20,18 @@ static void a6xx_preempt_timer(struct timer_list *t) > if (!try_preempt_state(a6xx_gpu, PREEMPT_TRIGGERED, PREEMPT_FAULTED)) > return; > > - dev_err(dev->dev, "%s: preemption timed out\n", gpu->name); > - kthread_queue_work(gpu->worker, &gpu->recover_work); > + msm_trigger_recovery(gpu); > } [Severity: Low] Removing dev_err() from a6xx_preempt_timer() leaves local variable dev unused: struct msm_gpu *gpu = &a6xx_gpu->base.base; struct drm_device *dev = gpu->dev; Was the preemption timeout error log removed intentionally, or should dev_err() be kept (or dev removed to avoid an unused-variable warning)? > -static void preempt_prepare_postamble(struct a6xx_gpu *a6xx_gpu) > +static void preempt_prepare_postamble(struct a6xx_gpu *a6xx_gpu, struct > msm_ringbuffer *ring) > { > u32 *postamble = a6xx_gpu->preempt_postamble_ptr; > + uint64_t last_active_ctxcycles; > u32 count = 0; > > + if (ring) > + last_active_ctxcycles = rbmemptr(ring, last_active_ctxcycles); > + > postamble[count++] = PKT7(CP_REG_RMW, 3); > postamble[count++] = REG_A6XX_RBBM_PERFCTR_SRAM_INIT_CMD; > postamble[count++] = 0; > @@ -43,6 +46,15 @@ static void preempt_prepare_postamble(struct a6xx_gpu > *a6xx_gpu) > postamble[count++] = CP_WAIT_REG_MEM_4_MASK(0x1); > postamble[count++] = CP_WAIT_REG_MEM_5_DELAY_LOOP_CYCLES(0); > > + a6xx_gpu->preempt_postamble_cntreset_end = count; > + > + postamble[count++] = PKT7(ring ? CP_REG_TO_MEM : CP_NOP, 3); > + postamble[count++] = CP_REG_TO_MEM_0_REG(REG_A6XX_CP_ALWAYS_ON_CONTEXT) > | > + CP_REG_TO_MEM_0_CNT(2) | > + CP_REG_TO_MEM_0_IS_64B; > + postamble[count++] = lower_32_bits(last_active_ctxcycles); > + postamble[count++] = upper_32_bits(last_active_ctxcycles); [Severity: Medium] When a6xx_preempt_init() calls preempt_prepare_postamble(a6xx_gpu, NULL), ring is NULL, so last_active_ctxcycles is never initialized before lower_32_bits(last_active_ctxcycles) and upper_32_bits(last_active_ctxcycles) are evaluated and written to postamble[]. Should last_active_ctxcycles be initialized to 0? > a6xx_gpu->preempt_postamble_len = count; > > a6xx_gpu->postamble_enabled = true; > @@ -54,9 +66,9 @@ static void preempt_disable_postamble(struct a6xx_gpu > *a6xx_gpu) > > /* > * Disable the postamble by replacing the first packet header with a NOP > - * that covers the whole buffer. > + * that skips the counters reset part. > */ > - *postamble = PKT7(CP_NOP, (a6xx_gpu->preempt_postamble_len - 1)); > + *postamble = PKT7(CP_NOP, (a6xx_gpu->preempt_postamble_cntreset_end - > 1)); > > a6xx_gpu->postamble_enabled = false; > } > @@ -263,8 +275,8 @@ void a6xx_preempt_trigger(struct msm_gpu *gpu) > /* Enable or disable postamble as needed */ > sysprof = msm_gpu_sysprof_no_perfcntr_zap(gpu); > > - if (!sysprof && !a6xx_gpu->postamble_enabled) > - preempt_prepare_postamble(a6xx_gpu); > + if (!sysprof) > + preempt_prepare_postamble(a6xx_gpu, ring); > > if (sysprof && a6xx_gpu->postamble_enabled) > preempt_disable_postamble(a6xx_gpu); [Severity: High] In a6xx_preempt_trigger(), ring is the incoming ring returned by get_next_ring(gpu) (assigned to a6xx_gpu->next_ring), while a6xx_gpu->cur_ring is the outgoing ring being preempted out. Since the postamble runs before switching away from the outgoing ring, doesn't passing ring instead of a6xx_gpu->cur_ring cause the postamble's CP_REG_TO_MEM packet to write the outgoing ring's CP_ALWAYS_ON_CONTEXT value into the incoming ring's last_active_ctxcycles, leaving the outgoing ring's last_active_ctxcycles unupdated? Furthermore, when sysprof is true, preempt_disable_postamble() only NOPs out up to preempt_postamble_cntreset_end so that the CP_REG_TO_MEM packet still runs, but preempt_prepare_postamble() is skipped when sysprof is true. Won't that leave the postamble writing to a stale ring's last_active_ctxcycles (or executing the initial CP_NOP from a6xx_preempt_init()) while sysprof is active? > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index 0c2c356362518..6417bef187b5e 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c [ ... ] > @@ -651,11 +653,32 @@ static bool made_progress(struct msm_gpu *gpu, struct > msm_ringbuffer *ring) > return true; > } > > -static void hangcheck_handler(struct timer_list *t) > +static bool check_ring_timeout(struct msm_ringbuffer *ring, unsigned long > timeout) > { > - struct msm_gpu *gpu = timer_container_of(gpu, t, hangcheck_timer); > + struct msm_gpu *gpu = ring->gpu; > + u64 start, end; > + int ret; [Severity: Low] In check_ring_timeout(), local variable ret is declared but never used. Can it be removed? > + > + if (!gpu->funcs->get_ctx_timestamp) > + return !made_progress(gpu, ring); > + > + start = ring->memptrs->last_job_start_ctx; > + > + if (gpu->funcs->get_ctx_timestamp(ring, &end)) > + end = ring->memptrs->last_active_ctxcycles; > + > + if (end >= start) > + return (end - start) > timeout; > + else > + return false; > +} > + > +static void hangcheck_worker(struct kthread_work *work) > +{ > + struct msm_ringbuffer *ring = container_of(work, struct msm_ringbuffer, > hangcheck_work); > + struct msm_gpu *gpu = ring->gpu; > + struct msm_drm_private *priv = gpu->dev->dev_private; > struct drm_device *dev = gpu->dev; > - struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu); > uint32_t fence = ring->memptrs->fence; > > if (fence != ring->hangcheck_fence) { > @@ -663,7 +686,7 @@ static void hangcheck_handler(struct timer_list *t) > ring->hangcheck_fence = fence; > ring->hangcheck_progress_retries = 0; > } else if (fence_before(fence, ring->fctx->last_fence) && > - !made_progress(gpu, ring)) { > + check_ring_timeout(ring, priv->hangcheck_period * > 192000)) { [Severity: Medium] Since the always-on context counter ticks at 19.2 MHz (19,200 ticks per millisecond), doesn't multiplying priv->hangcheck_period (in ms) by 192000 instead of 19200 make the timeout 10x longer than intended? Also, since priv->hangcheck_period is an unsigned int and timeout is an unsigned long, can priv->hangcheck_period * 192000 overflow 32-bit arithmetic when hangcheck_period is large or on 32-bit architectures? > /* no progress and not done.. hung! */ > ring->hangcheck_fence = fence; > ring->hangcheck_progress_retries = 0; > @@ -674,17 +697,26 @@ static void hangcheck_handler(struct timer_list *t) > DRM_DEV_ERROR(dev->dev, "%s: submitted fence: %u\n", > gpu->name, ring->fctx->last_fence); > > + gpu->hung_ring = ring; > kthread_queue_work(gpu->worker, &gpu->recover_work); > } > > /* if still more pending work, reset the hangcheck timer: */ > if (fence_after(ring->fctx->last_fence, ring->hangcheck_fence)) > - hangcheck_timer_reset(gpu); > + hangcheck_ring_timer_reset(gpu, ring); [Severity: High] Can check_ring_timeout() falsely declare a newly submitted job on a background ring hung before the GPU has switched to that ring? msm_gpu_submit() arms ring->hangcheck_timer immediately on the CPU, whereas ring->memptrs->last_job_start_ctx is only updated by the GPU when it executes the ring's command stream. If a higher-priority ring keeps the GPU busy while a new submit is queued on a lower-priority ring, ring->hangcheck_timer will fire before the new submit's CP_REG_TO_MEM executes. check_ring_timeout() will then read the stale last_job_start_ctx and last_active_ctxcycles left over from previous work on that ring (or across a GPU suspend/reset where last_active_ctxcycles was not cleared), and if their difference exceeded timeout, trigger a false recovery. In addition, when a hang is detected above, ring->hangcheck_fence is set to fence (which is before ring->fctx->last_fence), so fence_after(ring->fctx->last_fence, ring->hangcheck_fence) is true and immediately re-arms ring->hangcheck_timer while recover_work is pending, and a single gpu->hung_ring pointer can be overwritten if another ring's hangcheck fires before recover_worker() runs. [ ... ] > @@ -923,8 +955,6 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > if (funcs->progress) > priv->hangcheck_period /= 2; > > - timer_setup(&gpu->hangcheck_timer, hangcheck_handler, 0); > - [Severity: High] Removing timer_setup(&gpu->hangcheck_timer, ...) from msm_gpu_init() leaves gpu->hangcheck_timer uninitialized in struct msm_gpu, but a8xx_fault_detect_irq() and a8xx_sw_fuse_violation_irq() in drivers/gpu/drm/msm/adreno/a8xx_gpu.c still call: timer_delete(&gpu->hangcheck_timer); kthread_queue_work(gpu->worker, &gpu->recover_work); Won't triggering a fault or SW fuse violation on A8xx call timer_delete() on an uninitialized timer and fail to stop the per-ring hangcheck timers? Should those A8xx handlers call msm_trigger_recovery(gpu) and struct msm_gpu drop the unused hangcheck_timer field? [ ... ] > @@ -1006,6 +1036,9 @@ int msm_gpu_init(struct drm_device *drm, struct > platform_device *pdev, > goto fail; > } > > + timer_setup(&gpu->rb[i]->hangcheck_timer, hangcheck_handler, 0); > + kthread_init_work(&gpu->rb[i]->hangcheck_work, > hangcheck_worker); > + > memptrs += sizeof(struct msm_rbmemptrs); > memptrs_iova += sizeof(struct msm_rbmemptrs); > } [Severity: High] Where are gpu->rb[i]->hangcheck_timer and gpu->rb[i]->hangcheck_work cancelled during teardown? In msm_gpu_cleanup() (and the fail path of msm_gpu_init()), msm_ringbuffer_destroy(gpu->rb[i]) frees ring->fctx and schedules kfree_rcu(ring, rcu), and msm_gem_kernel_put(gpu->memptrs_bo, gpu->vm) unmaps and frees ring->memptrs before kthread_destroy_worker(gpu->worker) is called: for (i = 0; i < ARRAY_SIZE(gpu->rb); i++) { msm_ringbuffer_destroy(gpu->rb[i]); gpu->rb[i] = NULL; } msm_gem_kernel_put(gpu->memptrs_bo, gpu->vm); ... if (gpu->worker) { kthread_destroy_worker(gpu->worker); } If a ring's hangcheck_timer is still armed when the driver is unbound or cleaned up, can hangcheck_handler() and hangcheck_worker() fire during or after msm_ringbuffer_destroy() and cause a use-after-free on ring, ring->fctx, and ring->memptrs (as well as a NULL pointer dereference on gpu->rb[i] in msm_gpu_retire())? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
