Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unconditional execution of io_mapping_init_wc() in check_io_mapping() causes severe ioremap_wc memory leaks during BAR2 bootstrapping. --
commit 17419e048ef493385192b9b14726d1e9ad616fed Author: Dave Airlie <[email protected]> nouveau/instmem: use iomapping interface for instmem handling (v3) This replaces constant ioremapping with the io_mapping interface to avoid the constant need to ioremap when instobjs move. It delays creating the mapping until first use to avoid resource mapping errors with simpledrm. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > index 6fbaa1e5876dc..f4489efc94a79 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c [ ... ] > @@ -188,6 +190,16 @@ nv50_instobj_map(struct nvkm_memory *memory, u64 offset, > struct nvkm_vmm *vmm, > return nvkm_memory_map(memory, offset, vmm, vma, argv, argc); > } > > +static bool > +check_io_mapping(struct nv50_instmem *imem) > +{ > + struct nvkm_device *device = imem->base.subdev.device; > + > + return io_mapping_init_wc(&imem->iomap, > + device->func->resource_addr(device, > NVKM_BAR2_INST), > + device->func->resource_size(device, > NVKM_BAR2_INST)) != NULL; > +} > + [Severity: High] Does this unconditionally initialize the mapping and leak the previous ioremap_wc allocation if called more than once? Since nv50_instobj_boot() acts as the .boot callback for BAR2 page tables, it is invoked for every leaf page table during VMM bootstrap. Because BAR2 typically spans multiple page tables, check_io_mapping() gets executed multiple times in a loop. Should this check if the mapping is already initialized (e.g., checking if imem->iomap.size is non-zero) before calling io_mapping_init_wc() to avoid leaking the vmalloc space? > static void > nv50_instobj_release(struct nvkm_memory *memory) > { [ ... ] > @@ -277,7 +289,12 @@ nv50_instobj_boot(struct nvkm_memory *memory, struct > nvkm_vmm *vmm) > iobj->lru.next = NULL; > } > > - nv50_instobj_kmap(iobj, vmm); > + /* > + * boot is only called on BAR2, if we can't remap the complete > + * BAR it's unlikely things are functioning well. > + */ > + if (check_io_mapping(iobj->imem)) > + nv50_instobj_kmap(iobj, vmm); > nvkm_instmem_boot(imem); > mutex_unlock(&imem->mutex); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
