Re: [PATCH] nouveau/instmem: use iomapping interface for instmem handling (v3)

[sashiko-bot@kernel.org]

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 <airlied@redhat.com>

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/20260706030520.857104-1-airlied@gmail.com?part=1