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 <[email protected]> > --- > 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;
