From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panthor: Support sparse mappings Date: Thu, 23 Apr 2026 07:41:45 +1000 Message-ID: In-Reply-To: <20260422122538.3117380-6-adrian.larumbe@collabora.com> References: <20260422122538.3117380-1-adrian.larumbe@collabora.com> <20260422122538.3117380-6-adrian.larumbe@collabora.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 This is the main patch. Several issues: **1. Memory leak in `panthor_vm_pool_create` error path:** ```c + pfile->vms->dummy =3D panthor_dummy_bo_create(pfile->ptdev); + if (IS_ERR(pfile->vms->dummy)) + return PTR_ERR(pfile->vms->dummy); ``` If `panthor_dummy_bo_create` fails, the function returns an error without f= reeing `pfile->vms` or destroying the xarray initialized two lines above. T= he caller (`panthor_open`) goes to `err_free_file` which frees `pfile` but = not `pfile->vms`. This leaks the `panthor_vm_pool` allocation and the xarra= y. Additionally, `pfile->vms` is left pointing to memory with `dummy` set t= o an ERR_PTR, which would crash if `panthor_vm_pool_destroy` were ever call= ed on it (it does `drm_gem_object_put(&pfile->vms->dummy->base)`). The erro= r path should clean up and set `pfile->vms =3D NULL`. **2. Wrong address passed to `iova_mapped_as_huge_page` for `op->next` (reg= ression):** In `unmap_hugepage_align`, the `op->next` branch was changed from: ```c - iova_mapped_as_huge_page(op->next, unmap_end - 1)) { + (iova_mapped_as_huge_page(op->next, *unmap_start, is_sparse))) { ``` For the non-sparse case, the `addr` parameter is used to compute: ```c bo_offset =3D addr - op->va.addr + op->gem.offset; ``` The original code passed `unmap_end - 1` (last byte of the unmapped region)= , which is just before `op->next->va.addr`. The replacement `*unmap_start` = is the *beginning* of the unmap region, which is well before `op->next->va.= addr`, causing `addr - op->va.addr` to underflow (unsigned). For sparse, `b= o_offset` is forced to 0 so the bug doesn't manifest, but this is a regress= ion for non-sparse remap operations. The fix should pass `unmap_end - 1` fo= r non-sparse and `*unmap_start` (or any value) for sparse. **3. `panthor_vm_map_sparse` =E2=80=94 minor observation:** ```c + while (size > 0) { + u64 next_size =3D min(size, sg_dma_len(sgt->sgl)); ``` This always uses `sgt->sgl` (the first SG entry). If the dummy BO's pages a= re not physically contiguous, the first SG entry might be small (4KiB), mea= ning the entire sparse range maps to the same physical page repeatedly. Thi= s is functionally correct (sparse semantics say reads are undefined) but co= uld be slow for large ranges. The comment about the 2MiB dummy BO in `panth= or_dummy_bo_create` acknowledges this uncertainty =E2=80=94 worth noting bu= t not blocking. **4. Remap `size > 0` guards:** The patch adds `&& size > 0` checks in the remap prev/next branches: ```c - if (!unmap_vma->evicted) { + if (!unmap_vma->evicted && size > 0) { ``` This seems defensive =E2=80=94 it's not clear when `size` could be zero in = normal operation. If this is guarding against edge cases in the hugepage-al= igned unmap calculations for sparse mappings, a brief comment explaining wh= en this can happen would be helpful. **5. The design choice of per-file-context dummy BOs is well-motivated** = =E2=80=94 the commit message clearly explains the information-leak concern.= The VM holding its own reference to `pool->dummy` to survive past file con= text destruction is correct. --- Generated by Claude Code Patch Reviewer