Hi Krzysztof,
your patch looks good, but it needs to be polished a bit.
...
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 06543ae60706..f338dc39fad1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -68,10 +68,13 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> unsigned int max_segment)
> {
> unsigned int page_count; /* restricted by sg_alloc_table */
> - unsigned long i;
> + unsigned long next_pfn = 0; /* suppress gcc warning */
> + unsigned long folio_start = 0;
> + unsigned long folio_end = 0;
Can these go inside the for loop? Keep variables in the innermost
section needed.
Besides
> + struct folio *folio = NULL;
> struct scatterlist *sg;
> - unsigned long next_pfn = 0; /* suppress gcc warning */
> gfp_t noreclaim;
> + unsigned long i;
Please, don't reorder variables in this patch, it's out of the
scope.
> int ret;
>
> if (overflows_type(size / PAGE_SIZE, page_count))
> @@ -85,6 +88,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> if (size > resource_size(&mr->region))
> return -ENOMEM;
>
> + if (max_segment < PAGE_SIZE)
> + return -EINVAL;
> +
the max_segment validation is out of the scope.
> if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
> return -ENOMEM;
>
> @@ -101,7 +107,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> sg = st->sgl;
> st->nents = 0;
> for (i = 0; i < page_count; i++) {
> - struct folio *folio;
Why are you moving this out of the for loop?
> + unsigned long folio_page_index = 0;
This initialization is not necessary.
> unsigned long nr_pages;
> const unsigned int shrink[] = {
> I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
...
> + }
> + } while (1);
> +
> + folio_start = folio_pgoff(folio);
> + folio_end = folio_start + folio_nr_pages(folio) - 1;
> + }
> +
> + folio_page_index = i - folio_start;
> + if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) {
> + ret = -EINVAL;
> + folio_put(folio);
I think shmem_sg_free_table() drops the folio reference already,
right?
> + goto err_sg;
> + }
Is this WARN_ON_ONCE() check really necessary?
>
> nr_pages = min_array(((unsigned long[]) {
> - folio_nr_pages(folio),
> + folio_nr_pages(folio) -
> folio_page_index,
> page_count - i,
> max_segment / PAGE_SIZE,
> }), 3);
...
> @@ -186,6 +216,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> i915_sg_trim(st);
>
> return 0;
> +
Drop this change
What I want from this patch is a minimal change, without any
churn not belonging to the sole scope of the patch.
Thanks, Krzysztof,
Andi
> err_sg:
> sg_mark_end(sg);
> if (sg != st->sgl) {
> --
> 2.34.1
>
