From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: udmabuf: Do not create malformed scatterlists
Date: Sat, 14 Mar 2026 06:43:42 +1000
Message-ID: <review-patch1-0-v1-42779f29381a+4b9-udmabuf_sg_jgg@nvidia.com>
In-Reply-To: <0-v1-42779f29381a+4b9-udmabuf_sg_jgg@nvidia.com>
References: <0-v1-42779f29381a+4b9-udmabuf_sg_jgg@nvidia.com> <0-v1-42779f29381a+4b9-udmabuf_sg_jgg@nvidia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Correctness of the core fix:**

The key change in `get_sg_table()` replaces:
```c
ret =3D sg_alloc_table(sg, ubuf->pagecount, GFP_KERNEL);
...
for_each_sg(sg->sgl, sgl, ubuf->pagecount, i)
    sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE, ubuf->offsets[i]);
```
with:
```c
ret =3D sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, 0,
                                ubuf->pagecount << PAGE_SHIFT, GFP_KERNEL);
```

This is correct. `sg_alloc_table_from_pages()` will merge contiguous pages =
into single scatterlist entries, which is the expected behavior. The old co=
de allocated one SG entry per 4K page and set each individually, which prod=
uced a valid-looking but semantically broken scatterlist (no coalescing, an=
d the `sg_set_folio()` offset bug on large folios).

**Data structure change:**

Replacing `struct folio **folios` + `pgoff_t *offsets` with `struct page **=
pages` is appropriate. The page pointers are obtained via `folio_page()` in=
 `udmabuf_pin_folios()`:
```c
ubuf->pages[upgcnt] =3D folio_page(folios[cur_folio],
                    subpgoff >> PAGE_SHIFT);
```

This correctly extracts the tail page at the right sub-page offset within t=
he folio. The folio pin tracking remains via the separate `pinned_folios` a=
rray, which is unchanged =E2=80=94 this is fine since `unpin_folio()` works=
 on the head folio.

**vmap_udmabuf simplification:**

The old code allocated a temporary `pages` array and populated it from `fol=
ios[]`+`offsets[]` just to call `vm_map_ram()`. Now it passes `ubuf->pages`=
 directly. This is a nice cleanup that removes an allocation and avoids a p=
otential `-ENOMEM` failure path.

**Comment nit (very minor):**

The comment update at line 31-32 says:
```c
 * Unlike pages, pinned_folios is only used for unpin.
```
The grammar "Unlike pages" is slightly awkward =E2=80=94 it could be read a=
s "pages are used for unpin too" when the intent is "pages is used for more=
 than just unpin; pinned_folios is only for unpin." This is a pre-existing =
comment pattern that was just mechanically updated from "folios" to "pages"=
, so not really an issue introduced by this patch.

**No issues found.** The patch is a clean, correct fix for a real bug with =
a good Fixes tag and proper attribution.

---
Generated by Claude Code Patch Reviewer