From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/snapshot: fix dumping of the unaligned regions
Date: Sat, 16 May 2026 08:44:41 +1000
Message-ID: <review-patch1-20260516-msm-fix-dsi-dump-2-v1-1-43928094d25b@oss.qualcomm.com>
In-Reply-To: <20260516-msm-fix-dsi-dump-2-v1-1-43928094d25b@oss.qualcomm.com>
References: <20260516-msm-fix-dsi-dump-2-v1-1-43928094d25b@oss.qualcomm.com> <20260516-msm-fix-dsi-dump-2-v1-1-43928094d25b@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Debug filter left in =E2=80=94 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) fo=
r any string that does NOT match `"dsi%d_ctrl"`, so this causes `msm_disp_s=
napshot_add_block()` to silently return without capturing any DPU, DP, or o=
ther display hardware blocks. This is a serious regression that must be rem=
oved.

**16x overallocation fix =E2=80=94 correct**

The original code had:
```c
len_padded =3D 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 =3D round_up(len, REG_DUMP_ALIGN);
```

**`DIV_ROUND_UP` for num_rows =E2=80=94 correct**

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

**`end_addr` tracking =E2=80=94 correct**

Setting `end_addr =3D base_addr + len` (using the raw, unaligned length) en=
sures the per-register bounds checks in the dump loop:
```c
x0 =3D (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
x4 =3D (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 =E2=80=94 correct but minor style nit**

The new tail section in `msm_disp_state_print_regs` (lines 72-81):
```c
if (addr !=3D 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 =3D=
 len / REG_DUMP_ALIGN` (floor division), the tail handles the 1-3 remaining=
 registers. The buffer accesses are safe because the dump function allocate=
d `round_up(len, REG_DUMP_ALIGN)` bytes. One minor observation: the first r=
egister in the tail (`dump_addr[i * 4]`) is printed unconditionally =E2=80=
=94 this is safe because the `addr !=3D end_addr` guard ensures at least on=
e register remains.

**Removing alignment from the caller =E2=80=94 correct**

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

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

---
Generated by Claude Code Patch Reviewer