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

[PATCH] 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>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 27 ++++++++++++++++++-----
 1 file changed, 21 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..a86c7fc46f68 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)
@@ -172,6 +184,9 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
 	struct va_format vaf;
 	va_list va;
 
+	if (strcmp(fmt, "dsi%d_ctrl"))
+		return;
+
 	new_blk = kzalloc_obj(struct msm_disp_state_block);
 	if (!new_blk)
 		return;
@@ -185,7 +200,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-16T08:44:40.796210

---

This is a single patch fixing the MSM display snapshot code for unaligned register regions (specifically DSI, which has a 4-byte offset). The core fix is correct and well-reasoned: it removes premature alignment in the caller, fixes a 16x memory overallocation bug (`aligned_len * REG_DUMP_ALIGN` where multiplication was used instead of rounding), and adds proper handling for partial trailing rows during both register dumping and printing.

**However, there is a critical debug filter accidentally left in the patch** that makes it unsuitable for merging as-is. The `strcmp(fmt, "dsi%d_ctrl")` check at line 187 will silently skip ALL non-DSI snapshot blocks (DPU, DP, etc.), completely breaking devcoredump for anything other than DSI controllers.

**Verdict: Needs revision** — the debug filter must be removed before merging. The rest of the logic is correct.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Debug filter left in — NAK as-is**

At line 187-188 of the patched file:
```c
if (strcmp(fmt, "dsi%d_ctrl"))
    return;
```
This is clearly debug scaffolding used during development to limit snapshot capture to only DSI controller blocks. `strcmp` returns non-zero (true) for any string that does NOT match `"dsi%d_ctrl"`, so this causes `msm_disp_snapshot_add_block()` to silently return without capturing any DPU, DP, or other display hardware blocks. This is a serious regression that must be removed.

**16x overallocation fix — correct**

The original code had:
```c
len_padded = aligned_len * REG_DUMP_ALIGN;
```
where `aligned_len` was already aligned to 16 bytes. This multiplied by 16 again, allocating 16x the needed memory. The fix correctly uses:
```c
len_padded = round_up(len, REG_DUMP_ALIGN);
```

**`DIV_ROUND_UP` for num_rows — correct**

Changing from `aligned_len / REG_DUMP_ALIGN` (which relied on pre-alignment) to `DIV_ROUND_UP(len, REG_DUMP_ALIGN)` correctly handles unaligned lengths by producing enough rows to cover the full region.

**`end_addr` tracking — correct**

Setting `end_addr = base_addr + len` (using the raw, unaligned length) ensures the per-register bounds checks in the dump loop:
```c
x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
```
correctly avoid MMIO reads past the valid register region. This was already present in the pre-patch code but only works correctly now that `end_addr` uses the true length.

**Print tail handling — correct but minor style nit**

The new tail section in `msm_disp_state_print_regs` (lines 72-81):
```c
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");
}
```
The logic is correct: after printing full 4-register rows via `num_rows = len / REG_DUMP_ALIGN` (floor division), the tail handles the 1-3 remaining registers. The buffer accesses are safe because the dump function allocated `round_up(len, REG_DUMP_ALIGN)` bytes. One minor observation: the first register in the tail (`dump_addr[i * 4]`) is printed unconditionally — this is safe because the `addr != end_addr` guard ensures at least one register remains.

**Removing alignment from the caller — correct**

Changing `new_blk->size = ALIGN(len, REG_DUMP_ALIGN)` to `new_blk->size = len` moves alignment responsibility into the dump/print functions where it belongs. The raw length is needed to correctly bound MMIO reads and partial-row printing.

**Summary**: Remove the `strcmp(fmt, "dsi%d_ctrl")` debug filter and this patch is ready. The alignment fixes and tail-printing logic are sound.

---
Generated by Claude Code Patch Reviewer

[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