Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/imagination: memory leak of gpuvm_bo on pvr_vm_map error path --
commit 191fc7b0dba1141708c7edc0b3aeac368ef8588f Author: Gyeyoung Baek <[email protected]> drm/imagination: Switch the GPUVM to immediate mode This commit switches the GPUVM to immediate mode so that each GEM's GPUVA list is protected by its gpuva.lock. It introduces deferred operations such as drm_gpuvm_bo_put_deferred() and cleanup via drm_gpuvm_bo_deferred_cleanup(). > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c > b/drivers/gpu/drm/imagination/pvr_vm.c > --- a/drivers/gpu/drm/imagination/pvr_vm.c > +++ b/drivers/gpu/drm/imagination/pvr_vm.c [ ... ] > @@ -214,7 +214,7 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op > *bind_op) > > static void pvr_vm_bind_op_fini(struct pvr_vm_bind_op *bind_op) > { > - drm_gpuvm_bo_put(bind_op->gpuvm_bo); > + drm_gpuvm_bo_put_deferred(bind_op->gpuvm_bo); > > kfree(bind_op->new_va); > kfree(bind_op->prev_va); [ ... ] > @@ -758,7 +769,9 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct > pvr_gem_object *pvr_obj, > > err_cleanup: > mutex_unlock(&vm_ctx->lock); > + > pvr_vm_bind_op_fini(&bind_op); > + drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr); > > return err; > } [Severity: High] Does this code leak the gpuvm_bo and the referenced drm_gem_object on the early error path of pvr_vm_map()? If pvr_vm_bind_op_map_init() fails internally (for example, if memory is exhausted when calling pvr_gem_object_get_pages_sgt()), it takes its own error path: pvr_vm_bind_op_map_init() { ... err_bind_op_fini: pvr_vm_bind_op_fini(bind_op); return err; } This calls the newly modified pvr_vm_bind_op_fini(), which places the BO onto the gpuvm_mgr bo_defer list via drm_gpuvm_bo_put_deferred(). However, back in pvr_vm_map(), the early return bypasses the newly added cleanup call: pvr_vm_map() { ... int err = pvr_vm_bind_op_map_init(&bind_op, vm_ctx, pvr_obj, pvr_obj_offset, device_addr, size); if (err) return err; ... } Because drm_gpuvm_bo_deferred_cleanup() is bypassed here, would this cause an unbounded accumulation of leaked BOs on the bo_defer list? Can this be fixed by ensuring the cleanup function is also called on this early return path? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
