Am 2022-01-20 um 5:17 a.m. schrieb Miaoqian Lin:

Callback function amdgpu_xgmi_hive_release() in kobject_put() calls kfree(hive), So we don't need call kfree(hive) again.

Fixes: 7b833d680481 ("drm/amd/amdgpu: fix potential memleak") Signed-off-by: Miaoqian Lin linmq006@gmail.com

The patch is

Reviewed-by: Felix Kuehling Felix.Kuehling@amd.com

This kobject_init_and_add error handling semantics is very unintuitive, and we keep stumbling over it. I wonder is there is a better way to handle this. Basically, this is what it looks like, when done correctly:

foo = kzalloc(sizeof(*foo), GFP_KERNEL); if (!foo) return -ENOMEM; r = kobject_init_and_add(&foo->kobj, &foo_type, &parent, "foo_name"); if (r) { /* OK, initialization failed, but I still need to * clean up manually as if the call had succeeded. */ kobject_put(&foo->kobj); /* Don't kfree foo, because that's already done by * a callback setup by the call that failed above. */ return r; }

Given that unintuitive behaviour, I'd argue that kobject_init_and_add fails as an abstraction. Code would be clearer, more intuitive and safer by calling kobject_init and kobject_add separately itself. kobject_init_and_add saves you typing exactly one line of code, and it's just not worth it:

foo = kzalloc(sizeof(*foo), GFP_KERNEL); if (!foo) return -ENOMEM; kobject_init(&foo->kobj, &foo_type); /* never fails */ r = kobject_add(&foo->kobj, &parent, "foo_name"); if (r) { /* since kobj_init succeeded, it's obvious that kobj_put * is the right thing to do to handle all the cleanup. */ kobject_put(&foo->kobj); return r; }

Regards,   Felix

---

drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 1 - 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c index e8b8f28c2f72..35d4b966ef2c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c @@ -393,7 +393,6 @@ struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device *adev) if (ret) { dev_err(adev->dev, "XGMI: failed initializing kobject for xgmi hive\n"); kobject_put(&hive->kobj);

• kfree(hive);
  

  hive = NULL; goto pro_end; }