Re: [PATCH] drm/imagination: simplify pvr_fw_trace_seq_data

[Matt Coster]

On 11/03/2026 00:40, Rosen Penev wrote:
> A flexible array member allows one allocation instead of two. Which
> means one less kfree to worry about.

Hi Rosen,

Thanks for the patch! I agree, doing this as two kzalloc()s makes zero
sense (no pun intended).

> 
> Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> ---
>  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. */

Can you update this comment?

> +	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);

Given the buffer is 48KB, I think it's safe to say kcalloc() was the
wrong choice in the first place.

The choice of 12,000 for the DEFAULT_SIZE define appears arbitrary, and
although other sizes haven't been tested I wonder if a more "efficient"
replacement here would be to make it a power-of-2 size and use
folio_alloc()/folio_address()/folio_put() for the buffer instead of
making it a flexible array member.

Or maybe I'm overthinking it and we should just stick with a simple
vzalloc(struct_size(...))/vfree().

I'll leave it up to you.

Cheers,
Matt

>  	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;


-- 
Matt Coster
E: matt.coster@xxxxxxxxxx

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature