Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Unbalanced acquire/release refcount leak when nvkm_kmap() returns NULL --
--- Patch [1]: [PATCH] drm/nouveau/gsp: Preserve BAR2 flush mapping on kmap failure --- commit 31b13bcd1196c2d2aeb9a843ab22642741567833 Author: Ruoyu Wang <[email protected]> drm/nouveau/gsp: Preserve BAR2 flush mapping on kmap failure This patch prevents a NULL pointer dereference in r535_bar_bar2_init() by checking the return value of nvkm_kmap() before assignment. It also preserves the physical-mode flush mapping unless the BAR2 flush mapping was successfully acquired. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c > index 9cd68f8622d36..b7b44baf836cb 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c [ ... ] > @@ -71,8 +71,9 @@ r535_bar_bar2_fini(struct nvkm_bar *bar) > struct nvkm_vmm *vmm = gf100_bar(bar)->bar[0].vmm; > struct nvkm_gsp *gsp = bar->subdev.device->gsp; > > + if (bar->flushFBZero && bar->flushBAR2 != bar->flushBAR2PhysMode) > + nvkm_done(bar->flushFBZero); > bar->flushBAR2 = bar->flushBAR2PhysMode; [Severity: Medium] Will this skip the necessary cleanup if nvkm_kmap() failed? Because the failure path in r535_bar_bar2_init() below doesn't call nvkm_done(), and this condition now skips nvkm_done() if the flush pointer wasn't updated, this appears to cement a reference leak of the mapping. > - nvkm_done(bar->flushFBZero); > > WARN_ON(r535_bar_bar2_update_pde(gsp, vmm->func->page[0].shift, 0)); > } [ ... ] > @@ -106,12 +108,16 @@ r535_bar_bar2_init(struct nvkm_bar *bar) > ret = nvkm_memory_kmap(fbZero, &bar->flushFBZero); > nvkm_memory_unref(&fbZero); > } > - WARN_ON(ret); > + if (WARN_ON(ret)) > + return; > } > > + flush = nvkm_kmap(bar->flushFBZero); > + if (WARN_ON(!flush)) > + return; [Severity: Medium] Does this early return cause a refcount leak? In the Nouveau driver, nvkm_kmap(o) expands to (o)->func->acquire(o). The acquire implementation always increments the mapping reference count and removes the object from the LRU list, even if it returns a NULL mapping. If nvkm_kmap() fails to map and returns NULL, shouldn't nvkm_done() still be called to decrement the refcount and unpin the object before returning? > + > + bar->flushBAR2 = flush; > bar->bar2 = true; > - bar->flushBAR2 = nvkm_kmap(bar->flushFBZero); > - WARN_ON(!bar->flushBAR2); > } > > static void -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
