| Message ID | 20200904012218.4971-2-luben.tuikov@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert to using devm_drm_dev_alloc() | expand |
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
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
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
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&data=02%7C01%7Cluben.tuikov%40amd.com%7C17ffbdbf1cfb4aa34d1c08d851b362cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637349178195046108&sdata=tSTqRZ27GuQVAP8g7jmXAmM%2BhiGn3RUa72flIapYp9c%3D&reserved=0
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&data=02%7C01%7Cluben.tuikov%40amd.com%7Caa19e7efd2304d998c4008d85304e2c8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637350627736412131&sdata=4LzuUUOQSu5GiYSLfIymvUewNaJwwq%2BJR%2BAK60sfkpA%3D&reserved=0 >
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&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 >
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&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 >> >
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&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 > >> > > >
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;
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(-)