[PATCH 1/1] drm/amdgpu: Convert to using devm_drm_dev_alloc()

[Luben Tuikov]

On 2020-09-07 04:07, Daniel Vetter wrote:
> On Mon, Sep 07, 2020 at 10:06:08AM +0200, Daniel Vetter wrote:
>> On Sat, Sep 05, 2020 at 11:50:05AM -0400, Alex Deucher wrote:
>>> On Thu, Sep 3, 2020 at 9:22 PM Luben Tuikov <luben.tuikov at amd.com> wrote:
>>>>
>>>> Convert to using devm_drm_dev_alloc(),
>>>> as drm_dev_init() is going away.
>>>>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>>>
>>> I think we can drop the final drm_put in the error case?  I think the
>>> unwinding in current devm code should take care of it.
>>
>> Same applies for the pci remove hook too.
> 
> KASAN run with unload should have caught this. 

But it didn't? Why?
Could it be that drm_dev_put() actually got
the kref to 0 and then drm_dev_release()
was called which did a kfree()?

Could you try that same unload KASAN run but
with your suggestion of removing drm_dev_put() from
amdgpu_pci_remove()? What do you get then?

> I strongly recommend doing
> that for any changes to the unload code, it's way to easy to mix up
> something and release it in the wrong order or from the wrong callback or
> with the wrong managed (devm_ vs drmm_) functions.

Sorry, I don't understand what you mean by "doing that"? Do
you mean "not calling drm_dev_put()"? Sure, but what
are we supposed to call instead?

I also don't understand what you mean by "easy to mix up something
and release it in wrong order or from the wrong callback..." etc.

If you want things to happen in certain order,
you can either put the correct-order-sequence
behind the non-zero-->0 transition of kref, say in
drm_dev_release() as it is right now,

static void drm_dev_release(struct kref *ref)
{
        struct drm_device *dev = container_of(ref, struct drm_device, ref);

        if (dev->driver->release)
                dev->driver->release(dev);

        drm_managed_release(dev);

        kfree(dev->managed.final_kfree);
}

Or you can remove kref from DRM dev (which I do not
recommend), and stipulate the release sequence
as I asked in Message-ID: <165961bb-3b5b-cedc-2fc0-838b7999d2e3 at amd.com>,
"Re: [PATCH] drm/managed: Cleanup of unused functions and polishing docs".

Then we can follow that and submit patches to conform.

Regards,
Luben



> -Daniel
> 
>> -Daniel
>>>
>>> Alex
>>>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 11 +++--------
>>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 146a85c8df1c..06d994187c24 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -1142,18 +1142,13 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> -       adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>>>> -       if (!adev)
>>>> -               return -ENOMEM;
>>>> +       adev = devm_drm_dev_alloc(&pdev->dev, &kms_driver, typeof(*adev), ddev);
>>>> +       if (IS_ERR(adev))
>>>> +               return PTR_ERR(adev);
>>>>
>>>>         adev->dev  = &pdev->dev;
>>>>         adev->pdev = pdev;
>>>>         ddev = adev_to_drm(adev);
>>>> -       ret = drm_dev_init(ddev, &kms_driver, &pdev->dev);
>>>> -       if (ret)
>>>> -               goto err_free;
>>>> -
>>>> -       drmm_add_final_kfree(ddev, adev);
>>>>
>>>>         if (!supports_atomic)
>>>>                 ddev->driver_features &= ~DRIVER_ATOMIC;
>>>> --
>>>> 2.28.0.394.ge197136389
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7C0c811cf4c16d4f79bc0d08d853051125%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350628521258815&sdata=k9GiFNi%2Fu6Y1AlW7ea1cQINYigfYbrvPk2RkmUJkY8U%3D&reserved=0
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=02%7C01%7Cluben.tuikov%40amd.com%7C0c811cf4c16d4f79bc0d08d853051125%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350628521258815&sdata=aIT9t6q0qCTy%2BZhHPH0XIJgZ%2FYNF8xwzAQ2HlbxxMDk%3D&reserved=0
>