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/ttm: Support 52-bit PAs in ttm_place Date: Sat, 16 May 2026 11:53:55 +1000 Message-ID: In-Reply-To: <20260513141253.20410-1-felix.kuehling@amd.com> References: <20260513141253.20410-1-felix.kuehling@amd.com> <20260513141253.20410-1-felix.kuehling@amd.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 **Commit message vs. code mismatch:** The commit message says: > Grow these to **unsigned long** to support larger physical addresses. But the actual diff changes the type to `uint64_t`, not `unsigned long`: ```c - unsigned fpfn; - unsigned lpfn; + uint64_t fpfn; + uint64_t lpfn; ``` The code change (`uint64_t`) is actually the better choice =E2=80=94 `unsig= ned long` would only be 32 bits on 32-bit architectures, which wouldn't ach= ieve the stated goal. The commit message should be updated to say `uint64_t= ` to match the code. **Title says "52-bit" but no justification:** The subject line says "52-bit PAs" but neither the commit message nor the c= ode explains why 52 bits specifically. With `uint64_t` page numbers and 4KB= pages, the theoretical limit is 64+12 =3D 76-bit addressing. The title sho= uld either say "larger PAs" generically or explain why 52-bit is the releva= nt target (likely ARM64's 52-bit PA extension or x86's 5-level paging). **Missing driver-side audit:** The patch only changes the struct definition but doesn't audit the 21+ driv= er files that use `fpfn`/`lpfn`. Potential issues include: 1. **Truncation on assignment:** Drivers that assign `fpfn`/`lpfn` from 32-= bit variables are fine (implicit widening), but any driver storing the resu= lt of these fields back into a 32-bit variable could truncate silently. 2. **Format strings:** Any `printk`/`drm_dbg` calls that print fpfn/lpfn wi= th `%u` or `%x` will now produce warnings or incorrect output =E2=80=94 the= y need `%llu` or `PRIu64`. 3. **`struct ttm_resource.start`** is `unsigned long`, which is 32-bit on 3= 2-bit platforms. If fpfn/lpfn values >32 bits flow into `start`, there's a = truncation path. On 64-bit platforms `unsigned long` is 64-bit so this isn'= t an issue in practice, but it's worth noting. 4. **Structure size change:** `struct ttm_place` grows from 16 bytes to 24 = bytes (with padding). Since placements are typically small static/stack arr= ays, this shouldn't be a performance concern, but it's worth noting for any= driver that allocates large arrays of `ttm_place`. **Verdict:** The code change itself is correct and desirable =E2=80=94 `uint64_t` is the= right type for page frame numbers that need to exceed 32-bit range. Howeve= r: - Fix the commit message to say `uint64_t` instead of `unsigned long`. - Consider explaining *why* 52-bit in the subject, or generalize the title. - An audit of callers for truncation and format string issues would strengt= hen this patch (or could be a follow-up). --- Generated by Claude Code Patch Reviewer