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/imagination: simplify pvr_fw_trace_seq_data Date: Fri, 13 Mar 2026 13:53:03 +1000 Message-ID: In-Reply-To: <20260312192128.4133-1-rosenp@gmail.com> References: <20260312192128.4133-1-rosenp@gmail.com> <20260312192128.4133-1-rosenp@gmail.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 **Summary:** Moves the `u32 *buffer` pointer in `struct pvr_fw_trace_seq_da= ta` to a `u32 buffer[]` flexible array member, then replaces the separate `= kzalloc_obj` + `kcalloc` pair with a single `kzalloc_flex` call. **Correctness:** - The FAM `u32 buffer[]` is correctly placed as the last member of the stru= ct =E2=80=94 this is required by C for flexible array members. =E2=9C=85 - `kzalloc_flex(*trace_seq_data, buffer, ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN= _DWORDS)` correctly omits the GFP flag, which defaults to `GFP_KERNEL` via = the `default_gfp()` macro =E2=80=94 matching the original `kcalloc(..., GFP= _KERNEL)`. =E2=9C=85 - The `kzalloc` variant zeros the memory, matching the original `kcalloc` b= ehavior. =E2=9C=85 - The removal of `kfree(trace_seq_data->buffer)` in both the error path (`e= rr_free_buffer` label) and `fw_trace_release()` is correct since the buffer= is now part of the struct allocation. =E2=9C=85 - The error path cleanup is simplified from two labels (`err_free_buffer` = =E2=86=92 `err_free_data`) to one (`err_free_data`), which correctly just f= rees the single allocation. =E2=9C=85 **All existing accesses to `trace_seq_data->buffer[idx]`** (e.g., in `read_= fw_trace()` at line 276) continue to work identically =E2=80=94 array subsc= ript on a flexible array member works the same as on a pointer. **Minor observations:** - The comment was updated from `"Pointer to copy of trace data"` to `"Copy = of trace data"` =E2=80=94 appropriate since it's no longer a pointer. =E2= =9C=85 - The patch correctly uses `kzalloc_flex` which is the kernel's preferred m= acro for allocating structs with flexible array members, consistent with ot= her callers in the tree. **No issues found.** This is a clean, correct simplification. Reviewed-by: AI Review --- Generated by Claude Code Patch Reviewer