From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/snapshot: fix dumping of the unaligned regions Date: Mon, 18 May 2026 16:49:04 +1000 Message-ID: In-Reply-To: <20260516-msm-fix-dsi-dump-2-v2-1-9e49fb2d240e@oss.qualcomm.com> References: <20260516-msm-fix-dsi-dump-2-v2-1-9e49fb2d240e@oss.qualcomm.com> <20260516-msm-fix-dsi-dump-2-v2-1-9e49fb2d240e@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 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