On Fri, Sep 11, 2020 at 4:50 PM Luben Tuikov <luben.tuikov@xxxxxxx> wrote:
>
> 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@xxxxxxx> wrote:
> >>>>>
> >>>>> Convert to using devm_drm_dev_alloc(),
> >>>>> as drm_dev_init() is going away.
> >>>>>
> >>>>> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
> >>>>
> >>>> 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@xxxxxxx>,
> > "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.
devm_drm_dev_init() calls devm_add_action() which adds
devm_drm_dev_init_release() as the function which gets called for
resource unwinding. That calls drm_dev_put() which handles the ref
counting and clean up, so I don't think we need to call drm_dev_put()
in any of our unwinding paths anymore. All of the drm bits are
handled for us.
Alex
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> >>>>> 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
> >>
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
