Re: [PATCH] drm/imagination: clamp freelist reconstruction requests

[Alessio Belle <Alessio.Belle@imgtec.com>]

Hi Pengpeng,

Thanks for your patch and sorry the delay. A couple minor comments below.

On Thu, 2026-08-13 at 23:27 +0800, Pengpeng Hou wrote:
> The firmware reconstruction request contains a count followed by a
> fixed array of ROGUE_FWIF_MAX_FREELISTS_TO_RECONSTRUCT IDs. The host
> uses the count to walk the request array and to copy IDs into an equally
> sized response array without checking the protocol maximum.
> 
> A firmware count above that maximum therefore makes both operations
> access beyond their protocol objects. Clamp the count to the array
> capacity, reconstruct only that bounded subset, and report the same
> bounded count to firmware. This follows the recovery contract confirmed
> by the firmware maintainers: an incomplete bounded update is preferable
> to an unrepresentable response and gives firmware a chance to recover.
> 
> Warn once because an oversized count is a firmware bug, while avoiding
> repeated log flooding if firmware retries the malformed request.
> 
> Fixes: 6eedddab733b ("drm/imagination: Implement free list and HWRT create and destroy ioctls")
> Assisted-by: Codex:gpt-5
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since the RFC: https://lore.kernel.org/all/20260722040942.85459-1-pengpeng@iscas.ac.cn/
> - implement the maintainer-confirmed bounded recovery policy
> - warn once when firmware exceeds the protocol array capacity
> - disclose the use of Codex
> 
> The protocol extents and recovery path were reviewed statically. The
> change was not tested on PowerVR hardware.
> 
>  drivers/gpu/drm/imagination/pvr_free_list.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_free_list.c b/drivers/gpu/drm/imagination/pvr_free_list.c
> index e85cac83834c..23dbc227b081 100644
> --- a/drivers/gpu/drm/imagination/pvr_free_list.c
> +++ b/drivers/gpu/drm/imagination/pvr_free_list.c
> @@ -612,13 +612,20 @@ pvr_free_list_process_reconstruct_req(struct pvr_device *pvr_dev,
>  	};
>  	struct rogue_fwif_freelists_reconstruction_data *resp =
>  		&resp_cmd.cmd_data.free_lists_reconstruction_data;
> +	u32 count = min_t(u32, req->freelist_count,
> +			  ARRAY_SIZE(req->freelist_ids));
>  
> -	for (u32 i = 0; i < req->freelist_count; i++)
> +	if (count != req->freelist_count)
> +		drm_warn_once(from_pvr_device(pvr_dev),
> +			      "Firmware requested reconstruction of %u freelists, limiting to %u\n",
> +			      req->freelist_count, count);

This triggers a build error because drm_warn_once() is not defined here.
Could you include drm_print.h at the top, after drm_gem.h?

Also could you wrap the "if" block in brackets since it spans multiple lines?

Lastly, up to you, but if you remove "Firmware", the log would still make sense
and the line would stay below 100 columns.

Thanks,
Alessio

> +
> +	for (u32 i = 0; i < count; i++)
>  		pvr_free_list_reconstruct(pvr_dev, req->freelist_ids[i]);
>  
> -	resp->freelist_count = req->freelist_count;
> +	resp->freelist_count = count;
>  	memcpy(resp->freelist_ids, req->freelist_ids,
> -	       req->freelist_count * sizeof(resp->freelist_ids[0]));
> +	       count * sizeof(resp->freelist_ids[0]));
>  
>  	WARN_ON(pvr_kccb_send_cmd(pvr_dev, &resp_cmd, NULL));
>  }