Re: [PATCH v3 3/5] drm/i915/gem: Pull out size validation into a separate function

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Krzysztof,

On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote:
> shmem_sg_alloc_table is a very large and hard to read function,
> so reduce the number of operations it is responsible for by
> placing "size" validation in a new helper.
> 
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> v3:
>  * Split refactoring and put it after the fix in shmem folio
>   counting suggested by Andi.
> 
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 29 ++++++++++++++++-------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 0011d76f5b8c..4a61b012fb6f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -62,6 +62,22 @@ void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
>  	sg_free_table(st);
>  }
>  
> +static int validate_size(size_t size, unsigned int page_count,
> +			 struct intel_memory_region *mr)
> +{
> +	if (overflows_type(size / PAGE_SIZE, page_count))
> +		return -E2BIG;
> +
> +	/*
> +	 * If there's no chance of allocating enough pages for the whole
> +	 * object, bail early.
> +	 */
> +	if (size > resource_size(&mr->region))
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			 size_t size, struct intel_memory_region *mr,
>  			 struct address_space *mapping,
> @@ -77,16 +93,11 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	unsigned long i;
>  	int ret;
>  
> -	if (overflows_type(size / PAGE_SIZE, page_count))
> -		return -E2BIG;
> -
>  	page_count = size / PAGE_SIZE;

Verifying if page_count can accommodate a result before assigning that
result to it looked more correctly to me.  Since overflows_type() doesn't
look at the variable's value, only its type, I think you could postpone 
page_count initialization and pass its pointer to your helper to preserve 
that more reasonable order of operations.

Thanks,
Janusz

> -	/*
> -	 * If there's no chance of allocating enough pages for the whole
> -	 * object, bail early.
> -	 */
> -	if (size > resource_size(&mr->region))
> -		return -ENOMEM;
> +
> +	ret = validate_size(size, page_count, mr);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
>  		return -ENOMEM;