Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm/msm/a8xx: wrong GX power check and unpowered read in a8xx_gpu_state_get() - [High] drm/msm/a8xx: NULL dereference in __snapshot_sect_fini() on alloc failure - [Medium] drm/msm/a8xx: inverted return check in gen8_snapshot_mempool() - [Medium] drm/msm/a8xx: unrestored DBGC config after gen8_debugbus_internal_read_setup() - [Medium] drm/msm/a8xx: missing aperture clear and unlocked sel write in state dump - [Low] drm/msm/a8xx: 32-bit build failure in CD_WRITE() and CD_READ() with GENMASK() --
commit 219b73628f6b0835c639eddc9f3de8e4b31b60a5 Author: Rob Clark <[email protected]> drm/msm/a8xx: devcoredump state capture support This commit adds devcoredump GPU state capture and crashdumper support for Adreno Gen8 (A8xx) GPUs, along with snapshot section builder helpers. > diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > new file mode 100644 > index 0000000000000..b0b2624da5e08 > --- /dev/null > +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c > @@ -0,0 +1,1278 @@ [ ... ] > +static void gen8_legacy_snapshot_registers(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + const struct gen8_reg_list *regs, > + u32 slice_id) > +{ > + int count; > + > + /* Figure out how many registers we are going to dump */ > + count = adreno_snapshot_regs_count(regs->regs); > + > + snapshot_sect (state, MVC_V3, 4 * count, { > + .ctxt_id = 0, > + .cluster_id = CLUSTER_NONE, > + .pipe_id = PIPE_NONE, > + .location_id = UINT_MAX, > + .sp_id = UINT_MAX, > + .usptp_id = UINT_MAX, > + .slice_id = HEADER_SLICE_ID(regs->slice_region, slice_id), > + }) { > + unsigned long flags; > + > + if (regs->sel) > + gpu_write(gpu, regs->sel->host_reg, regs->sel->val); > + > + if (regs->slice_region) { > + u32 val = GEN8_CP_APERTURE_REG_VAL(slice_id, 0, 0, 0); > + a8xx_aperture_acquire_raw(gpu, val, &flags); > + } > + > + /* Make sure the previous writes are posted before reading */ > + mb(); > + > + for (const u32 *ptr = regs->regs; ptr[0] != UINT_MAX; ptr += 2) > { > + snapshot_mvc_regs_v3_subheader(§, ptr); > + > + for (int k = ptr[0]; k <= ptr[1]; k++) { > + snapshot_sect_u32(§, reg_read(gpu, k)); > + } > + } > + > + if (regs->slice_region) > + a8xx_aperture_release(gpu, flags); > + } > +} [Severity: Medium] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, gen8_legacy_snapshot_registers() writes regs->sel->host_reg before calling a8xx_aperture_acquire_raw(), unlike gen8_legacy_snapshot_mvc() which acquires the aperture first. Should the selector write happen after acquiring the slice aperture so that it is serialized under a6xx_gpu->aperture_lock and routed to the target slice? Also, a8xx_aperture_acquire_raw() programs REG_A8XX_CP_APERTURE_CNTL_HOST and updates a6xx_gpu->cached_aperture, while a8xx_aperture_release() only drops a6xx_gpu->aperture_lock without clearing the register. When state is captured through the host AHB path (such as for b->pre_crashdumper_regs, or when dumper is NULL during an SMMU stall-on-fault via adreno_fault_handler() -> msm_gpu_fault_crashstate_capture() -> msm_gpu_crashstate_capture() -> a8xx_gpu_state_get()), can leaving the aperture programmed after SLICE register lists, gen8_snapshot_cp_indexed_regs(), or gen8_legacy_snapshot_mvc() misroute subsequent UNSLICE reads and later un-apertured host MMIO accesses? drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c:gen8_legacy_snapshot_mvc() { ... u32 val = GEN8_CP_APERTURE_REG_VAL( slice_id, cluster->pipe_id, cluster->cluster_id, cluster->context_id); a8xx_aperture_acquire_raw(gpu, val, &flags); ... a8xx_aperture_release(gpu, flags); } } drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c:a8xx_gpu_state_get() { ... out: gpu_write(gpu, REG_A8XX_RBBM_SNAPSHOT_STATUS, 0x0); ... return &state->base; } Should gen8_legacy_snapshot_registers(), gen8_snapshot_mvc_regs(), and the out path of a8xx_gpu_state_get() call a8xx_aperture_clear(gpu) to reset the host aperture back to PIPE_NONE? [ ... ] > +static bool gen8_snapshot_mempool(struct msm_gpu *gpu, > + struct adreno_gpu_state *state, > + struct adreno_crashdumper *dumper) > +{ > + const struct gen8_snapshot_block_list *b = block_list(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(to_adreno_gpu(gpu)); > + u32 slice_mask = a6xx_gpu->slice_mask; > + u32 first_slice = a8xx_get_first_slice(a6xx_gpu); > + bool ret = true; > + > + for (int i = 0; i < b->mempool_index_registers_len; i++) { > + const struct gen8_cp_indexed_reg *cp_indexed_reg = > + &b->mempool_index_registers[i]; > + unsigned long flags; > + u32 j; > + > + FOR_EACH_SLICE (j, cp_indexed_reg->slice_region, slice_mask) { > + u32 pipe_id = cp_indexed_reg->pipe_id; > + > + /* Set CP_CHICKEN_DBG[StabilizeMVC] to stabilize it > while dumping */ > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice, > pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), > BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), BIT(2)); > + a8xx_aperture_release(gpu, flags); > + > + ret = gen8_snapshot_indexed_registers(gpu, state, > dumper, > + cp_indexed_reg->addr, > + cp_indexed_reg->data, 0, > + cp_indexed_reg->size, > + cp_indexed_reg->pipe_id, > + HEADER_SLICE_ID(cp_indexed_reg->slice_region, > j)); > + > + /* Reset CP_CHICKEN_DBG[StabilizeMVC] once we are done > */ > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL_NS(first_slice, > pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_CHICKEN_DBG_PIPE, BIT(2), 0); > + a8xx_aperture_release(gpu, flags); > + > + a8xx_aperture_acquire_raw(gpu, > + GEN8_CP_APERTURE_REG_VAL(j, pipe_id, 0, 0), > + &flags); > + gpu_rmw(gpu, REG_A8XX_CP_SLICE_CHICKEN_DBG_PIPE, > BIT(2), 0); > + a8xx_aperture_release(gpu, flags); > + > + if (ret) > + break; > + } > + } > + > + /* Clear aperture register */ > + a8xx_aperture_clear(gpu); > + > + return ret; > +} [Severity: Medium] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, gen8_snapshot_indexed_registers() returns true on success and false when CD_RUN() fails or times out: drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c:gen8_snapshot_indexed_registers() { ... if (CD_RUN(&p, gpu)) return false; snapshot_sect (state, INDEXED_REGS_V2, count * sizeof(u32), hdr) { const u32 *src = dumper->ptr + A8XX_CD_DATA_OFFSET; snapshot_sect_data(§, src, count * sizeof(u32)); } return true; } In a8xx_gpu_state_get() -> gen8_snapshot_mempool(), doesn't checking if (ret) break; invert the return value check? When gen8_snapshot_indexed_registers() succeeds (ret is true), the inner FOR_EACH_SLICE loop breaks after the first slice, skipping all remaining slices on multi-slice GPUs. Conversely, when CD_RUN() times out (ret is false), the loop continues to the next slice and the next entry in b->mempool_index_registers, issuing further hardware accesses on a wedged crashdumper and potentially overwriting ret with true. [ ... ] > +static void gen8_debugbus_internal_read_setup(struct msm_gpu *gpu) > +{ > + /* Clear the DBGC logic/events */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x70000000); > + > + /* Enable the internal buffer mode and program to tail mode */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_OPL, 0x28000000); > + > + /* Program the logic unit 0 to high when the side data is 1 */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, 0x0000000c); > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0, 0x000000f0); > + > + /* Enable the DBGC logic/events */ > + gpu_write(gpu, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, 0x07000000); > +} [ ... ] > +static void gen8_snapshot_cx_dbgc_debugbus_block( > + struct msm_gpu *gpu, uint32_t block_id, size_t count, > + struct adreno_snapshot_sect *sect) > +{ > + /* This block requires an out of order access due to a HW limitation. > + * (QCTDD12240539) > + */ > + if (block_id == DEBUGBUS_CX_GC_US_I_0) { > + static u32 data[GEN8_DEBUGBUS_BLOCK_SIZE * 2]; > + gen8_debugbus_internal_read_setup(gpu); > + for (int i = 0; i < GEN8_DEBUGBUS_BLOCK_IDX_1; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + for (int i = GEN8_DEBUGBUS_BLOCK_IDX_2; i < > GEN8_DEBUGBUS_BLOCK_SIZE; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + for (int i = GEN8_DEBUGBUS_BLOCK_IDX_1; i < > GEN8_DEBUGBUS_BLOCK_IDX_2; i++) > + gen8_cx_gc_us_i_0_debugbus_read(gpu, block_id, i, > &data[i * 2]); > + snapshot_sect_arr(sect, data); > + } else { > + for (int i = 0; i < count; i++) { > + u32 data[2]; > + gen8_cx_debug_bus_read(gpu, block_id, i, data); > + snapshot_sect_arr(sect, data); > + } > + } > +} [Severity: Medium] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, gen8_get_debugbus() programs REG_A6XX_DBGC_CFG_DBGBUS_CNTLT, REG_A6XX_DBGC_CFG_DBGBUS_CNTLM, REG_A6XX_DBGC_CFG_DBGBUS_IVTL_0..3, REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0..1, and REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0..3 once before iterating through b->debugbus_blocks and b->gbif_debugbus_blocks. When gen8_snapshot_cx_dbgc_debugbus_block() encounters DEBUGBUS_CX_GC_US_I_0, it calls gen8_debugbus_internal_read_setup(), which overwrites REG_A6XX_DBGC_CFG_DBGBUS_OPL (enabling internal buffer mode), REG_A6XX_DBGC_CFG_DBGBUS_BYTEL_0, REG_A6XX_DBGC_CFG_DBGBUS_MASKL_0, and REG_A6XX_DBGC_CFG_DBGBUS_CNTLM. Because those registers are not restored after reading DEBUGBUS_CX_GC_US_I_0, won't all subsequent blocks in b->debugbus_blocks and b->gbif_debugbus_blocks read corrupted debugbus data through gen8_cx_debug_bus_read() while DBGC remains in internal buffer mode? [ ... ] > +struct msm_gpu_state *a8xx_gpu_state_get(struct msm_gpu *gpu) > +{ > + const struct gen8_snapshot_block_list *b = block_list(gpu); > + struct adreno_crashdumper _dumper = { 0 }, *dumper = NULL; > + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); > + struct adreno_gpu_state *state; > + // TODO need to get status from SMMU on some gen8.. > + bool stalled = !!(gpu_read(gpu, REG_A8XX_RBBM_MISC_STATUS) & > + A8XX_RBBM_MISC_STATUS_SMMU_STALLED_ON_FAULT); > + > + if (!b) > + return NULL; > + > + state = kzalloc_obj(*state); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + INIT_LIST_HEAD(&state->objs); > + > + /* Get the generic state from the adreno core */ > + adreno_gpu_state_get(gpu, &state->base); > + > + adreno_snapshot_registers_v2(gpu, state, b->gmu_cx_unsliced_regs); > + > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.log, SNAPSHOT_GMU_MEM_LOG); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.hfi, SNAPSHOT_GMU_MEM_HFI); > + a8xx_snapshot_gmu_bo(state, &a6xx_gpu->gmu.debug, > SNAPSHOT_GMU_MEM_DEBUG); > + > + a8xx_snapshot_gmu_hfi_history(gpu, state); > + > + /* If GX isn't on the rest of the data isn't going to be accessible */ > + if (!a6xx_gmu_gx_is_on(adreno_gpu)) > + return &state->base; [Severity: High] In drivers/gpu/drm/msm/adreno/a8xx_gpu_state.c, a8xx_gpu_state_get() reads REG_A8XX_RBBM_MISC_STATUS at the top of the function before checking whether the GX power domain is on, and then checks GX power by calling a6xx_gmu_gx_is_on(adreno_gpu) instead of a8xx_gmu_gx_is_on(adreno_gpu) (or adreno_gpu->funcs->gx_is_on(adreno_gpu)). In drivers/gpu/drm/msm/adreno/a6xx_gmu.c, a6xx_gmu_gx_is_on() reads REG_A6XX_GMU_SPTPRAC_PWR_CLK_STATUS and checks the A6xx bit definitions, whereas A8xx reports GX power and clock status via REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS (on the A850 family, avoiding GMUCX reads when GMU MX is off) or REG_A8XX_GMU_PWR_CLK_STATUS in a8xx_gmu_gx_is_on(): drivers/gpu/drm/msm/adreno/a6xx_gmu.c:a8xx_gmu_gx_is_on() { ... if (adreno_is_a850_family(adreno_gpu)) { val = a6xx_cx_misc_read(a6xx_gpu, REG_A8XX_CX_MISC_GFX_PWR_CLK_STATUS); return !(val & (A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_GDSC_POWER_OFF | A8XX_CX_MISC_GFX_PWR_CLK_STATUS_GX_CLK_OFF)); } val = gmu_read(gmu, REG_A8XX_GMU_PWR_CLK_STATUS); return !(val & (A8XX_GMU_PWR_CLK_STATUS_GX_HM_GDSC_POWER_OFF | A8XX_GMU_PWR_CLK_STATUS_GX_HM_CLK_OFF)); } When a8xx_gpu_state_get() is called while GX is power-collapsed (for example during IFPC or a GMU fault), can reading REG_A8XX_RBBM_MISC_STATUS before the power check and calling a6xx_gmu_gx_is_on() cause unpowered GX or GMU MMIO accesses and trigger a bus error or SError panic? [ ... ] > diff --git a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > index f9956e0bc1d5f..1847bca38fe4a 100644 > --- a/drivers/gpu/drm/msm/adreno/adreno_snapshot.h > +++ b/drivers/gpu/drm/msm/adreno/adreno_snapshot.h [ ... ] > @@ -319,3 +329,199 @@ struct adreno_snapshot_gmu_version { [ ... ] > +static void __snapshot_sect_fini(struct adreno_snapshot_sect *s) > +{ > + struct adreno_snapshot_section_header *hdr = (void *)s->obj->ptr; > + hdr->size = s->obj->size = ((u8 *)s->cur - (u8 *)s->obj->ptr); > +} > + > +DEFINE_FREE(sect_fini, struct adreno_snapshot_sect, > __snapshot_sect_fini(&_T)); > + > +static struct adreno_snapshot_sect > +__snapshot_sect_init(struct adreno_gpu_state *state, uint16_t id, size_t > extra_sz, > + void *sect_hdr, size_t sect_hdr_sz, const char *func, int > line) > +{ > + /* A few section types have no section header: */ > + switch (id) { > + case ADRENO_SNAPSHOT_SECTION_REGS_V2: > + sect_hdr_sz = 0; > + default: > + break; > + } > + > + pr_debug("%s:%d: snapshot_sect(id=%04x, extra_sz=%zu, > sect_hdr_sz=%zu)\n", > + func, line, id, extra_sz, sect_hdr_sz); > + > + size_t sz = sizeof(struct adreno_snapshot_section_header) + sect_hdr_sz > + extra_sz; > + struct adreno_state_memobj *obj = kzalloc_obj(*obj); > + void *ptr = kvzalloc(sz, GFP_KERNEL); > + > + if (!obj || !ptr) { > + kfree(obj); > + kvfree(ptr); > + return (struct adreno_snapshot_sect){}; > + } [ ... ] > +#define __snapshot_sect(_label, _state, _id, _extra_sz, ...) \ > + for (struct adreno_snapshot_sect sect __free(sect_fini) = ({ \ > + struct STRUCT_ ## _id sect_hdr = __VA_ARGS__; \ > + __snapshot_sect_init(_state, ADRENO_SNAPSHOT_SECTION_ ## _id, > \ > + _extra_sz, §_hdr, \ > + sizeof(sect_hdr), \ > + __func__, __LINE__); }); \ > + sect.obj; ({ goto _label; })) \ > + if (false) { \ > +_label: > \ > + break; \ > + } else [Severity: High] In drivers/gpu/drm/msm/adreno/adreno_snapshot.h, if kzalloc_obj() or kvzalloc() fails in __snapshot_sect_init() (which is particularly likely when a8xx_gpu_state_get() is invoked from recover_worker() or msm_gpu_fault_crashstate_capture() under memalloc_noreclaim_save()), __snapshot_sect_init() returns a zeroed struct adreno_snapshot_sect with sect.obj set to NULL. When the for loop in __snapshot_sect() checks sect.obj, sees NULL, and exits its scope, the __free(sect_fini) cleanup handler still runs __snapshot_sect_fini(§). Since neither sect_fini nor __snapshot_sect_fini() checks if (s->obj), won't dereferencing s->obj->ptr cause a NULL pointer dereference? [ ... ] > +static inline void CD_WRITE(struct cd_script *p, u32 reg, u32 val) > +{ > + *(p->in++) = val; > + *(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | BIT(21) | BIT(0); > + __cd_check_overflow(p); > +} > + > +static inline void CD_READ(struct cd_script *p, u32 reg, u32 dwords) > +{ > + *(p->in++) = p->out; > + *(p->in++) = FIELD_PREP(GENMASK(63, 44), reg) | dwords; > + p->out += sizeof(u32) * dwords; > + __cd_check_overflow(p); > +} [Severity: Low] In drivers/gpu/drm/msm/adreno/adreno_snapshot.h, CD_WRITE() and CD_READ() use GENMASK(63, 44) with FIELD_PREP() to build 64-bit crashdumper script entries. Because drivers/gpu/drm/msm/adreno/a8xx_gpu_state.o is built whenever CONFIG_DRM_MSM_GPU_STATE is enabled, including on 32-bit ARM builds where unsigned long is 32 bits wide, won't GENMASK(63, 44) fail to compile on 32-bit architectures unless GENMASK_ULL(63, 44) is used? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
