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

[Luben Tuikov <luben.tuikov@amd.com>]

On 2020-09-08 16:09, Luben Tuikov wrote:
> 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@amd.com> wrote:
>>>>>
>>>>> Convert to using devm_drm_dev_alloc(),
>>>>> as drm_dev_init() is going away.
>>>>>
>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@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?

Hi Daniel,

Have you had a chance to run this unload KASAN run with
your suggestion of removing drm_dev_put() from
the PCI release hook?

If it "should have caught this", but it didn't,
perhaps it did catch it when you removed the drm_dev_put()
hook from the PCI release hook, when you did a KASAN unload run?
Showing that drm_dev_put() is still necessary, since,
1) we're still using kref,
2) kref is kref-init-ed under devm_drm_dev_alloc() as I pointed
   out in my reply to Alex in this thread.

I believe KASAN (and logic) show this patch to be solid.

> 
>> 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@amd.com>,
> "Re: [PATCH] drm/managed: Cleanup of unused functions and polishing docs".
> 
> Then we can follow that and submit patches to conform.

Eagerly awaiting your response on this so that we can conform
to the direction you're setting forth.

Are you removing kref (release() cb) from DRM and if so,
what function should we call in order to do the "final"
(although without kref, the notion of "final" is obviated)
free, OR kref stays in and this patch, which conforms
to using devm_drm_dev_alloc(), as postulated by you,
can go in.

Regards,
Luben

> 
> 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@lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7C0c811cf4c16d4f79bc0d08d853051125%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350628521258815&amp;sdata=k9GiFNi%2Fu6Y1AlW7ea1cQINYigfYbrvPk2RkmUJkY8U%3D&amp;reserved=0
>>>
>>> -- 
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7C0c811cf4c16d4f79bc0d08d853051125%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350628521258815&amp;sdata=aIT9t6q0qCTy%2BZhHPH0XIJgZ%2FYNF8xwzAQ2HlbxxMDk%3D&amp;reserved=0
>>
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx