[PATCH 0/1] Convert to using devm_drm_dev_alloc()

[PATCH 0/1] Convert to using devm_drm_dev_alloc()

[Luben Tuikov]

This is based on top of Daniel's documentation
patch and it applies cleanly onto amd-staging-drm-next.
I'm also running this live.

Luben Tuikov (1):
  drm/amdgpu: Convert to using devm_drm_dev_alloc()

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

-- 
2.28.0.394.ge197136389

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

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

[Luben Tuikov]

Convert to using devm_drm_dev_alloc(),
as drm_dev_init() is going away.

Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
---
 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://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

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.

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://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

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.
-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://lists.freedesktop.org/mailman/listinfo/amd-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

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. 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.
-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://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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?

> 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.

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

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

[Luben Tuikov]

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

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

[Alex Deucher]

On Fri, Sep 11, 2020 at 4:50 PM Luben Tuikov <luben.tuikov@amd.com> 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@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.

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@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

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

[Luben Tuikov]

On 2020-09-07 04:06, 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.

What same applies to the pci remove hook? Removing "drm_dev_put()"
altogether?

Well, amdgpu_pci_remove() doesn't call any devm_ or drmm_ functions--it
only calls drm_dev_put() at the very end. This in turn decrements
the kref and if the kref is 0 at this point it calls drm_dev_release()
which does this:

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);
}

Regards,
Luben

> -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%7Caa19e7efd2304d998c4008d85304e2c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350627736412131&amp;sdata=4LzuUUOQSu5GiYSLfIymvUewNaJwwq%2BJR%2BAK60sfkpA%3D&amp;reserved=0
> 

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

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

[Luben Tuikov]

On 2020-09-05 11:50, 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.

We get to the error case if devm_drm_dev_alloc() succeeds,
but we get an error thereafter, thus we do a drm_dev_put()
past the err_free label, all in amdgpu_pci_probe().

However, devm_drm_dev_alloc() calls devm_drm_dev_init(), which
calls drm_dev_init(), which does a kref_init() on &dev->ref,
which sets the kref to 1.

The drm_dev_put(), in the error case, would decrement it,
and if it is 0 after being decremented, then drm_dev_release()
is called, which does the "managed" free.

Regards,
Luben

> 
> 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%7C17ffbdbf1cfb4aa34d1c08d851b362cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637349178195046108&amp;sdata=tSTqRZ27GuQVAP8g7jmXAmM%2BhiGn3RUa72flIapYp9c%3D&amp;reserved=0

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