Re: [PATCH] drm/msm/adreno: Improve AHB error messages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] drm/msm/a8xx: status clobbering in a8xx_cp_hw_err_irq() [1]
--

commit 036d732e4c5bd970decdf064cde4d6ff621ef0a9
Author: Rob Clark <robin.clark@oss.qualcomm.com>

drm/msm/adreno: Improve AHB error messages

This commit adds A7XX register definitions and decodes CP_RL_ERROR_DETAILS
registers on A7XX and A8XX GPUs to log detailed AHB bus error messages.

> diff --git a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
> index f892731cd3d3a..f2dcf62be861e 100644
> --- a/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a8xx_gpu.c
> @@ -1232,12 +1232,18 @@ irqreturn_t a8xx_irq(struct msm_gpu *gpu)
>  		a8xx_fault_detect_irq(gpu);
>  
>  	if (status & A6XX_RBBM_INT_0_MASK_CP_AHB_ERROR) {
> -		u32 rl0, rl1;
> +		u32 rl0, rl1, addr;
> +		const char *mode;
>  
>  		rl0 = gpu_read(gpu, REG_A8XX_CP_RL_ERROR_DETAILS_0);
>  		rl1 = gpu_read(gpu, REG_A8XX_CP_RL_ERROR_DETAILS_1);
> +
> +		addr = FIELD(rl0, A8XX_CP_RL_ERROR_DETAILS_0_ADDR);
> +		mode = (rl0 & A8XX_CP_RL_ERROR_DETAILS_0_WRITE) ? "write" : "read";
> +
>  		dev_err_ratelimited(&gpu->pdev->dev,
> -				"CP | AHB bus error RL_ERROR_0: %x, RL_ERROR_1: %x\n", rl0, rl1);
> +			"CP | AHB bus error RL_ERROR_0: %x (addr: 0x%05x, %s), RL_ERROR_1: %x\n",
> +			rl0, addr, mode, rl1);
>  	}
>  
>  	if (status & A6XX_RBBM_INT_0_MASK_CP_HW_ERROR)

[Severity: Medium]
This problem wasn't introduced by this patch, but does a8xx_cp_hw_err_irq()
clobber status while iterating over pipes [1]?

drivers/gpu/drm/msm/adreno/a8xx_gpu.c:a8xx_cp_hw_err_irq() {
	u32 status = gpu_read(gpu, REG_A8XX_CP_INTERRUPT_STATUS_GLOBAL);
    ...
	for (unsigned int pipe_id = PIPE_NONE; pipe_id <= PIPE_DDE_BV; pipe_id++) {
		if (!(BIT(pipe_id) & pipe))
			continue;

		if (hw_fault_mask & status) {
			status = a8xx_read_pipe_slice(gpu, pipe_id, slice,
					REG_A8XX_CP_HW_FAULT_STATUS_PIPE);
    ...
		}

		if (sw_fault_mask & status) {
			status = a8xx_read_pipe_slice(gpu, pipe_id, slice,
					REG_A8XX_CP_INTERRUPT_STATUS_PIPE);
    ...
			if (status & BIT(8)) {
				a8xx_write_pipe(gpu, pipe_id, REG_A8XX_CP_SQE_STAT_ADDR_PIPE, 1);
				status = a8xx_read_pipe_slice(gpu, pipe_id, slice,
						REG_A8XX_CP_SQE_STAT_DATA_PIPE);
    ...
			}

			if (status & BIT(10)) {
				status = a8xx_read_pipe_slice(gpu, pipe_id, slice,
						REG_A8XX_CP_PROTECT_STATUS_PIPE);
    ...
			}
		}
	}
}

Overwriting status with per-pipe fault and opcode register values causes the
subsequent sw_fault_mask check, the BIT(10) register protection check, and
later pipe iterations to test unrelated values instead of the interrupt
status registers, resulting in missed or bogus fault diagnostics.

[1] https://sashiko.dev/#/bug/linux-69f62dbe-1123-4e6d-8506-4fc99ce6cd9b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111171417.249152-1-robin.clark@oss.qualcomm.com?part=1