[PATCH] drm/imagination: simplify pvr_fw_trace_seq_data

[PATCH] drm/imagination: simplify pvr_fw_trace_seq_data

[Rosen Penev]

A flexible array member allows one allocation instead of two. Which
means one less kfree to worry about.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index e154cb35f604..caf504f2bf85 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
@@ -241,9 +241,6 @@ update_logtype(struct pvr_device *pvr_dev, u32 group_mask)
 }
 
 struct pvr_fw_trace_seq_data {
-	/** @buffer: Pointer to copy of trace data. */
-	u32 *buffer;
-
 	/** @start_offset: Starting offset in trace data, as reported by FW. */
 	u32 start_offset;
 
@@ -252,6 +249,9 @@ struct pvr_fw_trace_seq_data {
 
 	/** @assert_buf: Trace assert buffer, as reported by FW. */
 	struct rogue_fwif_file_info_buf assert_buf;
+
+	/** @buffer: Pointer to copy of trace data. */
+	u32 buffer[];
 };
 
 static u32 find_sfid(u32 id)
@@ -455,17 +455,10 @@ static int fw_trace_open(struct inode *inode, struct file *file)
 	struct pvr_fw_trace_seq_data *trace_seq_data;
 	int err;
 
-	trace_seq_data = kzalloc_obj(*trace_seq_data);
+	trace_seq_data = kzalloc_flex(*trace_seq_data, buffer, ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS);
 	if (!trace_seq_data)
 		return -ENOMEM;
 
-	trace_seq_data->buffer = kcalloc(ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS,
-					 sizeof(*trace_seq_data->buffer), GFP_KERNEL);
-	if (!trace_seq_data->buffer) {
-		err = -ENOMEM;
-		goto err_free_data;
-	}
-
 	/*
 	 * Take a local copy of the trace buffer, as firmware may still be
 	 * writing to it. This will exist as long as this file is open.
@@ -478,15 +471,12 @@ static int fw_trace_open(struct inode *inode, struct file *file)
 
 	err = seq_open(file, &pvr_fw_trace_seq_ops);
 	if (err)
-		goto err_free_buffer;
+		goto err_free_data;
 
 	((struct seq_file *)file->private_data)->private = trace_seq_data;
 
 	return 0;
 
-err_free_buffer:
-	kfree(trace_seq_data->buffer);
-
 err_free_data:
 	kfree(trace_seq_data);
 
@@ -499,7 +489,6 @@ static int fw_trace_release(struct inode *inode, struct file *file)
 		((struct seq_file *)file->private_data)->private;
 
 	seq_release(inode, file);
-	kfree(trace_seq_data->buffer);
 	kfree(trace_seq_data);
 
 	return 0;
-- 
2.53.0

Claude review: drm/imagination: simplify pvr_fw_trace_seq_data

[Claude Code Review Bot]

Patch Review

**Correctness: Good**

The conversion is mechanically correct:

- The `u32 *buffer` pointer is replaced with `u32 buffer[]` flexible array member, properly placed at the end of the struct.
- `kzalloc_obj(*trace_seq_data)` + `kcalloc(ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS, ...)` is replaced with a single `kzalloc_flex(*trace_seq_data, buffer, ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS)`, which will compute the correct total size via `struct_size`.
- The `err_free_buffer` goto label and `kfree(trace_seq_data->buffer)` in both the error path and `fw_trace_release()` are correctly removed, since one `kfree(trace_seq_data)` now frees everything.

**All existing users of `trace_seq_data->buffer` remain valid** — the FAM is accessed with the same `trace_seq_data->buffer[idx]` syntax (confirmed at line 276 of the patched file in `read_fw_trace()`).

**No issues found.** This is a clean, correct simplification.

**Reviewed-by worthy:** Yes, this patch is ready to apply.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: simplify pvr_fw_trace_seq_data

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: simplify pvr_fw_trace_seq_data
Author: Rosen Penev <rosenp@gmail.com>
Patches: 1
Reviewed: 2026-03-11T12:56:11.528185

---

This is a single-patch series that converts `pvr_fw_trace_seq_data` from using a separate heap allocation for its `buffer` member to using a C flexible array member (FAM). This is a straightforward and correct cleanup that:

1. Reduces the number of allocations from two to one
2. Removes the associated error-handling path and `kfree` call
3. Uses the kernel's `kzalloc_flex` helper correctly

The patch is clean and correct. It follows an established kernel pattern for converting pointer+allocation pairs to flexible array members.

---
Generated by Claude Code Patch Reviewer

[PATCH] drm/imagination: simplify pvr_fw_trace_seq_data

[Rosen Penev]

A flexible array member allows one allocation instead of two. Which
means one less kfree to worry about.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index e154cb35f604..99d002b481a9 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
@@ -241,9 +241,6 @@ update_logtype(struct pvr_device *pvr_dev, u32 group_mask)
 }
 
 struct pvr_fw_trace_seq_data {
-	/** @buffer: Pointer to copy of trace data. */
-	u32 *buffer;
-
 	/** @start_offset: Starting offset in trace data, as reported by FW. */
 	u32 start_offset;
 
@@ -252,6 +249,9 @@ struct pvr_fw_trace_seq_data {
 
 	/** @assert_buf: Trace assert buffer, as reported by FW. */
 	struct rogue_fwif_file_info_buf assert_buf;
+
+	/** @buffer: Copy of trace data. */
+	u32 buffer[];
 };
 
 static u32 find_sfid(u32 id)
@@ -455,17 +455,10 @@ static int fw_trace_open(struct inode *inode, struct file *file)
 	struct pvr_fw_trace_seq_data *trace_seq_data;
 	int err;
 
-	trace_seq_data = kzalloc_obj(*trace_seq_data);
+	trace_seq_data = kzalloc_flex(*trace_seq_data, buffer, ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS);
 	if (!trace_seq_data)
 		return -ENOMEM;
 
-	trace_seq_data->buffer = kcalloc(ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS,
-					 sizeof(*trace_seq_data->buffer), GFP_KERNEL);
-	if (!trace_seq_data->buffer) {
-		err = -ENOMEM;
-		goto err_free_data;
-	}
-
 	/*
 	 * Take a local copy of the trace buffer, as firmware may still be
 	 * writing to it. This will exist as long as this file is open.
@@ -478,15 +471,12 @@ static int fw_trace_open(struct inode *inode, struct file *file)
 
 	err = seq_open(file, &pvr_fw_trace_seq_ops);
 	if (err)
-		goto err_free_buffer;
+		goto err_free_data;
 
 	((struct seq_file *)file->private_data)->private = trace_seq_data;
 
 	return 0;
 
-err_free_buffer:
-	kfree(trace_seq_data->buffer);
-
 err_free_data:
 	kfree(trace_seq_data);
 
@@ -499,7 +489,6 @@ static int fw_trace_release(struct inode *inode, struct file *file)
 		((struct seq_file *)file->private_data)->private;
 
 	seq_release(inode, file);
-	kfree(trace_seq_data->buffer);
 	kfree(trace_seq_data);
 
 	return 0;
-- 
2.53.0

Claude review: drm/imagination: simplify pvr_fw_trace_seq_data

[Claude Code Review Bot]

Patch Review

**Summary:** Moves the `u32 *buffer` pointer in `struct pvr_fw_trace_seq_data` 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 struct — this is required by C for flexible array members. ✅
- `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 — matching the original `kcalloc(..., GFP_KERNEL)`. ✅
- The `kzalloc` variant zeros the memory, matching the original `kcalloc` behavior. ✅
- The removal of `kfree(trace_seq_data->buffer)` in both the error path (`err_free_buffer` label) and `fw_trace_release()` is correct since the buffer is now part of the struct allocation. ✅
- The error path cleanup is simplified from two labels (`err_free_buffer` → `err_free_data`) to one (`err_free_data`), which correctly just frees the single allocation. ✅

**All existing accesses to `trace_seq_data->buffer[idx]`** (e.g., in `read_fw_trace()` at line 276) continue to work identically — array subscript 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"` — appropriate since it's no longer a pointer. ✅
- The patch correctly uses `kzalloc_flex` which is the kernel's preferred macro for allocating structs with flexible array members, consistent with other callers in the tree.

**No issues found.** This is a clean, correct simplification.

Reviewed-by: AI Review <ai@review>

---
Generated by Claude Code Patch Reviewer

Claude review: drm/imagination: simplify pvr_fw_trace_seq_data

[Claude Code Review Bot]

Overall Series Review

Subject: drm/imagination: simplify pvr_fw_trace_seq_data
Author: Rosen Penev <rosenp@gmail.com>
Patches: 1
Reviewed: 2026-03-13T13:53:03.575176

---

This is a single, straightforward cleanup patch that converts two separate allocations (one for the struct, one for the trace buffer) into a single allocation using a flexible array member (FAM). The transformation is correct and follows established kernel patterns. The patch reduces code complexity by eliminating one allocation, one error path, and one `kfree` call.

**Verdict: Looks good.**

---
Generated by Claude Code Patch Reviewer