[PATCH v2] drm/imagination: simplify pvr_fw_trace_seq_data

[PATCH v2] drm/imagination: simplify pvr_fw_trace_seq_data

[Rosen Penev]

The trace buffer copy always has ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS
entries, so embed it in struct pvr_fw_trace_seq_data as a fixed-size
array instead of allocating it separately. This removes one allocation
and its error handling, and lets FORTIFY_SOURCE and UBSAN_BOUNDS see the
real size of the buffer.

At about 48KB, the struct needs an order-4 page allocation, which can
fail under memory fragmentation. Allocate it with kvzalloc_obj() so it
falls back to vmalloc in that case, and free it with kvfree().

Assisted-by: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: use kvzalloc_obj()
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 25 ++++++----------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index 6fe478f64f02..27bbc62114b1 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
@@ -240,9 +240,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;
 
@@ -251,6 +248,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[ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS];
 };
 
 static u32 find_sfid(u32 id)
@@ -454,17 +454,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 = kvzalloc_obj(*trace_seq_data);
 	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.
@@ -477,17 +470,14 @@ 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);
+	kvfree(trace_seq_data);
 
 	return err;
 }
@@ -498,8 +488,7 @@ 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);
+	kvfree(trace_seq_data);
 
 	return 0;
 }
-- 
2.56.0

Re: [PATCH v2] drm/imagination: simplify pvr_fw_trace_seq_data

[Alexandru Dadu]

Hi Rosen,

On Mon, 2026-10-05 at 12:41 -0700, Rosen Penev wrote:
> The trace buffer copy always has
> ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS
> entries, so embed it in struct pvr_fw_trace_seq_data as a fixed-size
> array instead of allocating it separately. This removes one
> allocation
> and its error handling, and lets FORTIFY_SOURCE and UBSAN_BOUNDS see
> the
> real size of the buffer.
> 
> At about 48KB, the struct needs an order-4 page allocation, which can
> fail under memory fragmentation. Allocate it with kvzalloc_obj() so
> it
> falls back to vmalloc in that case, and free it with kvfree().
> 
> Assisted-by: LLM
> Signed-off-by: Rosen Penev <rosenp@gmail.com>

Reviewed-by: Alexandru Dadu <alexandru.dadu@imgtec.com>

Thanks,
Alexandru Dadu
> ---
>  v2: use kvzalloc_obj()
>  drivers/gpu/drm/imagination/pvr_fw_trace.c | 25 ++++++--------------
> --
>  1 file changed, 7 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c
> b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> index 6fe478f64f02..27bbc62114b1 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> @@ -240,9 +240,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;
>  
> @@ -251,6 +248,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[ROGUE_FW_TRACE_BUF_DEFAULT_SIZE_IN_DWORDS];
>  };
>  
>  static u32 find_sfid(u32 id)
> @@ -454,17 +454,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 = kvzalloc_obj(*trace_seq_data);
>  	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.
> @@ -477,17 +470,14 @@ 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);
> +	kvfree(trace_seq_data);
>  
>  	return err;
>  }
> @@ -498,8 +488,7 @@ 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);
> +	kvfree(trace_seq_data);
>  
>  	return 0;
>  }