Hi Janusz, On 2026-07-15 at 17:21:39 +0200, Janusz Krzysztofik wrote: > 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. Yeah, you are right, it would make more sense to validate it before assignment. I'll do that in the next version.
> > 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; -- Best Regards, Krzysztof
