[PATCH] drm/amdgpu: Fix a deadlock if previous GEM object allocation fails

[Pan, Xinhui]

在 2021/8/31 13:38，“Pan, Xinhui”<Xinhui.Pan at amd.com> 写入:

    
    
    在 2021/8/31 12:03，“Grodzovsky, Andrey”<Andrey.Grodzovsky at amd.com> 写入:
    
        
        On 2021-08-30 11:24 p.m., Pan, Xinhui wrote:
        > [AMD Official Use Only]
        >
        > [AMD Official Use Only]
        >
        > Unreserve root BO before return otherwise next allocation got deadlock.
        >
        > Signed-off-by: xinhui pan <xinhui.pan at amd.com>
        > ---
        >   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 +++++------
        >   1 file changed, 5 insertions(+), 6 deletions(-)
        >
        > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
        > index 85b292ed5c43..c9db7d2c5816 100644
        > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
        > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
        > @@ -355,19 +355,18 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
        >                          DRM_DEBUG("Failed to allocate GEM object (%llu, %d, %llu, %d)\n",
        >                                    size, initial_domain, args->in.alignment, r);
        >                  }
        > +
        > +               if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
        > +                       amdgpu_bo_unreserve(vm->root.bo);
        >                  return r;
        >          }
        >
        >          if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) {
        > -               if (!r) {
        > -                       struct amdgpu_bo *abo = gem_to_amdgpu_bo(gobj);
        > +               struct amdgpu_bo *abo = gem_to_amdgpu_bo(gobj);
        >
        > -                       abo->parent = amdgpu_bo_ref(vm->root.bo);
        > -               }
        > +               abo->parent = amdgpu_bo_ref(vm->root.bo);
        >                  amdgpu_bo_unreserve(vm->root.bo);
        >          }
        > -       if (r)
        > -               return r;
        
        
        The above early return seems to be needed for -ERESTARTSYS case.
        
        Andrey
    
    There are two returns. ERESTARTSYS and other error after retry are already handled by the first return in if {}. So the second return is not needed.
    
    Thanks
    Xinhui

Also we can do something like below which is simpler.
@@ -355,7 +355,6 @@ int amdgpu_gem_create_ioctl(struct drm_device *dev, void *data,
                        DRM_DEBUG("Failed to allocate GEM object (%llu, %d, %llu, %d)\n",
                                  size, initial_domain, args->in.alignment, r);
                }
-               return r;
        }
 
        if (flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID) {
 
        
        >
        >          r = drm_gem_handle_create(filp, gobj, &handle);
        >          /* drop reference from allocate - handle holds it now */
        > --
        > 2.25.1