[PATCH v2] drm/msm/snapshot: fix dumping of the unaligned regions

[PATCH v2] drm/msm/snapshot: fix dumping of the unaligned regions

[Dmitry Baryshkov]

The snapshotting code internally aligns data segment to 16 bytes. This
works fine for DPU code (where most of the regions are aligned), but
fails for snapshotting of the DSI data (because DSI data region is
shifted by 4 bytes). Fix the code by removing length alignment and by
accurately printing last registers in the region. While reworking the
code also fix the 16x memory overallocation in
msm_disp_state_dump_regs().

Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reported-by: Salendarsingh Gaud <sgaud@qti.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Changes in v2:
- Drop the debugging clause, limiting dumping to DSI control regions.
- Link to v1: https://patch.msgid.link/20260516-msm-fix-dsi-dump-2-v1-1-43928094d25b@oss.qualcomm.com
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 24 +++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index 5e151952dea8..c8a10b1232c2 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -9,7 +9,7 @@
 
 #include "msm_disp_snapshot.h"
 
-static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *base_addr)
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr)
 {
 	u32 len_padded;
 	u32 num_rows;
@@ -19,11 +19,11 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	void __iomem *end_addr;
 	int i;
 
-	len_padded = aligned_len * REG_DUMP_ALIGN;
-	num_rows = aligned_len / REG_DUMP_ALIGN;
+	len_padded = round_up(len, REG_DUMP_ALIGN);
+	num_rows = DIV_ROUND_UP(len, REG_DUMP_ALIGN);
 
 	addr = base_addr;
-	end_addr = base_addr + aligned_len;
+	end_addr = base_addr + len;
 
 	*reg = kvzalloc(len_padded, GFP_KERNEL);
 	if (!*reg)
@@ -48,8 +48,8 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 		void __iomem *base_addr, struct drm_printer *p)
 {
+	void __iomem *addr, *end_addr;
 	int i;
-	void __iomem *addr;
 	u32 num_rows;
 
 	if (!dump_addr) {
@@ -58,6 +58,7 @@ static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 	}
 
 	addr = base_addr;
+	end_addr = base_addr + len;
 	num_rows = len / REG_DUMP_ALIGN;
 
 	for (i = 0; i < num_rows; i++) {
@@ -67,6 +68,17 @@ static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 				dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
 		addr += REG_DUMP_ALIGN;
 	}
+
+	if (addr != end_addr) {
+		drm_printf(p, "0x%lx : %08x",
+			   (unsigned long)(addr - base_addr),
+			   dump_addr[i * 4]);
+		if (addr + 0x4 < end_addr)
+			drm_printf(p, " %08x", dump_addr[i * 4 + 1]);
+		if (addr + 0x8 < end_addr)
+			drm_printf(p, " %08x", dump_addr[i * 4 + 2]);
+		drm_printf(p, "\n");
+	}
 }
 
 void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
@@ -185,7 +197,7 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
 	va_end(va);
 
 	INIT_LIST_HEAD(&new_blk->node);
-	new_blk->size = ALIGN(len, REG_DUMP_ALIGN);
+	new_blk->size = len;
 	new_blk->base_addr = base_addr;
 
 	msm_disp_state_dump_regs(&new_blk->state, new_blk->size, base_addr);

---
base-commit: 11ff30385c8ad7de9862f4f1cec424fca15a4f13
change-id: 20260515-msm-fix-dsi-dump-2-64a3bc160da6

Best regards,
--  
With best wishes
Dmitry

Claude review: drm/msm/snapshot: fix dumping of the unaligned regions

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/snapshot: fix dumping of the unaligned regions
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-05-18T16:49:03.943027

---

This is a single-patch fix for the MSM display snapshot code that handles dumping hardware registers for devcoredump. The patch addresses two related bugs:

1. **Memory overallocation**: The old code treated the `aligned_len` parameter as if it were already in units of `REG_DUMP_ALIGN` (16 bytes), then multiplied it again (`aligned_len * REG_DUMP_ALIGN`), causing 16x overallocation.
2. **Unaligned region truncation**: Regions not aligned to 16 bytes (like DSI ctrl regions, whose size comes from the DT resource size) had their trailing registers silently dropped during printing.

The fix is correct, well-scoped, and the commit message clearly explains the problem. The Fixes tag is appropriate.

**Verdict: Looks good to merge.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/snapshot: fix dumping of the unaligned regions

[Claude Code Review Bot]

Patch Review

**Understanding the old bug:**

Before this patch, `msm_disp_snapshot_add_block()` pre-aligned the size:
```c
new_blk->size = ALIGN(len, REG_DUMP_ALIGN);
```
Then `msm_disp_state_dump_regs()` received this already-aligned value as `aligned_len` and treated it as a count in units of `REG_DUMP_ALIGN`:
```c
len_padded = aligned_len * REG_DUMP_ALIGN;  // 16x overallocation!
num_rows = aligned_len / REG_DUMP_ALIGN;
end_addr = base_addr + aligned_len;
```
So if the caller passed `len = 0x200` (512 bytes), it was aligned to `0x200` (no-op since already aligned), then the dump function computed `len_padded = 0x200 * 16 = 0x2000` -- a 16x overallocation. Meanwhile `num_rows = 0x200 / 16 = 32` and `end_addr = base_addr + 0x200` were coincidentally correct for aligned inputs, masking the arithmetic confusion.

For unaligned DSI regions (e.g., a ctrl_size that isn't a multiple of 16), the `ALIGN()` in `msm_disp_snapshot_add_block` would round up, but the dump function's `end_addr = base_addr + aligned_len` would then read past the actual MMIO region, which could cause bus errors or read garbage registers.

**Fix analysis:**

The fix correctly:

1. **Stores the raw length** in `new_blk->size = len` -- no premature alignment.

2. **Aligns internally in `msm_disp_state_dump_regs()`**:
   ```c
   len_padded = round_up(len, REG_DUMP_ALIGN);  // for kvzalloc size
   num_rows = DIV_ROUND_UP(len, REG_DUMP_ALIGN); // iterate all rows including partial
   end_addr = base_addr + len;                    // true end of MMIO region
   ```
   The dump function already had per-register `addr < end_addr` guards, so the partial last row is handled correctly -- registers past the true end read as 0 (the guard returns 0, and kvzalloc zeroed the buffer).

3. **Handles partial rows in printing**: The new tail code in `msm_disp_state_print_regs()` prints only the registers that were actually within the region:
   ```c
   if (addr != end_addr) {
       drm_printf(p, "0x%lx : %08x", ...);
       if (addr + 0x4 < end_addr) ...
       if (addr + 0x8 < end_addr) ...
       drm_printf(p, "\n");
   }
   ```

**Minor observations:**

- **`num_rows` in print function not updated**: In `msm_disp_state_print_regs()`, line 62 still uses integer division `num_rows = len / REG_DUMP_ALIGN` (truncating). This is intentional and correct -- the for-loop handles only complete rows, and the new tail block handles the remainder. Good.

- **`block->size` now stores raw len**: `msm_disp_state_print_regs` is called with `block->size` at line 101. Since `size` now stores the unaligned value, both the dump and print functions receive the same raw length, and the `kvzalloc` allocation in the dump function uses `round_up(len, REG_DUMP_ALIGN)`, so the buffer is large enough for the print function to read `dump_addr[i * 4 + N]` indices up to the last partial row without going out of bounds. Correct.

- **First register unconditionally printed in tail**: The tail code unconditionally prints `dump_addr[i * 4]` when `addr != end_addr`. Since `addr` only reaches this point if `addr < end_addr` (because `num_rows = len / REG_DUMP_ALIGN` means `addr = base_addr + num_rows * REG_DUMP_ALIGN`, and if `addr != end_addr` then there are remaining bytes), there is always at least one register to print. Correct.

- **Declaration order change**: Moving `end_addr` declaration and putting `int i` after it is a minor style cleanup (reverse Christmas tree). Fine.

**No issues found. Patch is correct.**

---
Generated by Claude Code Patch Reviewer