Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] LPAC-specific registers and commands are programmed unconditionally for all A6XX-family GPUs, breaking hardware initialization on older chips. - [High] Missing error check for LPAC ringbuffer allocation leads to out-of-bounds array access and silent failures. - [High] Unconditional creation of LPAC submitqueues allows unprivileged users to cause unrecoverable GPU hangs on hardware without LPAC support. - [High] The GPU hangcheck mechanism ignores the LPAC ring, meaning LPAC-exclusive hangs will never be recovered. - [High] In `msm_iommu_new`, failing to attach the LPAC domain causes resource leaks and use-after-free bugs by freeing attached domains. - [Medium] Performance counter statistics for LPAC submissions will be corrupted due to mismatched start and end register reads. - [Medium] When error interrupts are disabled, LPAC completion interrupts are inadvertently dropped, causing job timeouts. --
commit f42a6839003ad94fdc9d88ca88077e9fcbcda345 Author: Anna Maniscalco <[email protected]> drm/msm: Add LPAC submitqueue Add UAPI and infostructure to support sending submission to the LPAC ring. > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 9f69aada9b7b7..4417a9d04d7c2 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c [ ... ] > @@ -493,7 +498,10 @@ 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)); > + if (is_lpac) > + get_stats_counter(ring, cp_always_on_counter, > rbmemptr_stats(ring, index, alwayson_start)); > + else > + get_stats_counter(ring, cp_always_on_context, > rbmemptr_stats(ring, index, alwayson_start)); [Severity: Medium] Does this cause a mismatch for LPAC submissions? The start counter conditionally reads from cp_always_on_counter for LPAC, but the end counter unconditionally reads from cp_always_on_context later in the function: get_stats_counter(ring, rbbm_perfctr_cp0, rbmemptr_stats(ring, index, cpcycles_end)); get_stats_counter(ring, cp_always_on_context, rbmemptr_stats(ring, index, alwayson_end)); This will cause the duration of LPAC jobs to be incorrectly calculated since the start and end values are read from different registers. > OUT_PKT7(ring, CP_THREAD_CONTROL, 1); > OUT_RING(ring, CP_SET_THREAD_BOTH); [ ... ] > @@ -2030,6 +2048,9 @@ static irqreturn_t a6xx_irq(struct msm_gpu *gpu) > a6xx_preempt_trigger(gpu); > } > > + if (status & A6XX_RBBM_INT_0_MASK_CP_CACHE_FLUSH_TS_LPAC) > + msm_gpu_retire(gpu); > + [Severity: Medium] Are LPAC completion interrupts inadvertently dropped when error interrupts are disabled? Earlier in a6xx_irq(): if (priv->disable_err_irq) status &= A6XX_RBBM_INT_0_MASK_CP_CACHE_FLUSH_TS; In testing or debug modes where disable_err_irq is set, the interrupt status mask preserves the main ring's completion bit but drops the newly added LPAC completion bit (CP_CACHE_FLUSH_TS_LPAC). This causes LPAC jobs to artificially time out during these tests. Should the mask be updated to include the LPAC bit? > if (status & A6XX_RBBM_INT_0_MASK_CP_SW) [ ... ] > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c > b/drivers/gpu/drm/msm/msm_submitqueue.c > index 1a5a77b280163..042b4f90fb73f 100644 > --- a/drivers/gpu/drm/msm/msm_submitqueue.c > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c [ ... ] > @@ -211,6 +212,13 @@ int msm_submitqueue_create(struct drm_device *drm, > struct msm_context *ctx, > queue = kzalloc_obj(*queue); > } > > + if (flags & MSM_SUBMITQUEUE_LPAC) { > + ring_nr = priv->gpu->nr_rings; > + ring = priv->gpu->lpac_rb; > + } else { > + ring = priv->gpu->rb[ring_nr]; > + } > + [Severity: High] Can an unprivileged user cause a GPU hang regression on hardware without LPAC support? Because lpac_rb is exposed unconditionally here, userspace can request an LPAC submitqueue and push commands to it even on hardware without an LPAC processor. The hardware will ignore the software ringbuffer, causing the jobs to time out and the GPU to enter a recovery loop. Should this verify if the hardware actually supports LPAC first? > if (!queue) > return -ENOMEM; [ ... ] I also noticed a few regressions in the broader LPAC enablement changes outside of this specific diff: [Severity: Critical] Is it safe to unconditionally program LPAC registers for all A6xx-family GPUs? drivers/gpu/drm/msm/adreno/a6xx_gpu.c:hw_init() { ... /* Set the ringbuffer address for lpac */ gpu_write64(gpu, REG_A7XX_CP_LPAC_RB_BASE, gpu->lpac_rb->iova); ... ret = lpac_cp_init(gpu); if (ret) goto out; ... } Writing to A7xx-only registers and submitting initialization commands to the non-existent LPAC ring on older A6xx GPUs appears to cause the hardware to timeout during a6xx_idle() and potentially trigger AHB bus errors. This introduces a regression in GPU driver initialization on non-LPAC platforms. [Severity: High] Does this error check inspect the wrong variable? drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_init() { ... gpu->lpac_rb = msm_ringbuffer_new(gpu, nr_rings, memptrs, memptrs_iova); if (IS_ERR(gpu->rb[i])) { ret = PTR_ERR(gpu->rb[i]); ... } This checks gpu->rb[i] instead of gpu->lpac_rb. This accesses memory past the end of the initialized gpu->rb array and ignores lpac_rb allocation failures. If the allocation fails, gpu->lpac_rb remains an error pointer and can cause a panic when accessed later. [Severity: High] Will the GPU hangcheck mechanism fail to detect LPAC-exclusive hangs? drivers/gpu/drm/msm/msm_gpu.c:hangcheck_handler() { ... struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu); ... } The handler only inspects progress on the main ring returned by active_ring(gpu), skipping the concurrent lpac_rb. If a job hangs on the LPAC ring while the main ring is idle or progressing normally, the timer will never detect the lockup and the GPU will remain stuck. [Severity: High] Could this error path leak the primary domain or cause a use-after-free? drivers/gpu/drm/msm/msm_iommu.c:msm_iommu_new() { ... if (lpac_dev) { if (!device_iommu_mapped(lpac_dev)) return ERR_PTR(-ENODEV); lpac_domain = iommu_paging_domain_alloc(lpac_dev); if (IS_ERR(lpac_domain)) return ERR_CAST(lpac_domain); ... ret = iommu_attach_device(lpac_domain, lpac_dev); if (ret) { iommu_domain_free(domain); iommu_domain_free(lpac_domain); kfree(iommu); return ERR_PTR(ret); } ... } If device_iommu_mapped(lpac_dev) is false or domain allocation fails, it returns an error without freeing the primary domain, causing a memory leak. Additionally, if iommu_attach_device() fails, it calls iommu_domain_free() on the primary domain while it is still actively attached to dev. Freeing an attached domain without first calling iommu_detach_device() can lead to use-after-free corruption in the IOMMU subsystem. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0...@gmail.com?part=11
