* [PATCH] drm/hisilicon: Use pcim_enable_device()
@ 2020-12-21 0:45 Tian Tao
2020-12-21 22:02 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Tian Tao @ 2020-12-21 0:45 UTC (permalink / raw)
To: airlied, daniel, tzimmermann, kraxel, alexander.deucher, tglx,
dri-devel, xinliang.liu
Using the managed function simplifies the error handling. After
unloading the driver, the PCI device should now get disabled as
well.
Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 02f3bd1..7159018 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
dev->pdev = pdev;
pci_set_drvdata(pdev, dev);
- ret = pci_enable_device(pdev);
+ ret = pcim_enable_device(pdev);
if (ret) {
drm_err(dev, "failed to enable pci device: %d\n", ret);
goto err_free;
@@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
ret = hibmc_load(dev);
if (ret) {
drm_err(dev, "failed to load hibmc: %d\n", ret);
- goto err_disable;
+ goto err_free;
}
ret = drm_dev_register(dev, 0);
@@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
err_unload:
hibmc_unload(dev);
-err_disable:
- pci_disable_device(pdev);
err_free:
drm_dev_put(dev);
--
2.7.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/hisilicon: Use pcim_enable_device()
2020-12-21 0:45 [PATCH] drm/hisilicon: Use pcim_enable_device() Tian Tao
@ 2020-12-21 22:02 ` Daniel Vetter
2020-12-22 0:38 ` tiantao (H)
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2020-12-21 22:02 UTC (permalink / raw)
To: Tian Tao
Cc: airlied, dri-devel, xinliang.liu, kraxel, tzimmermann,
alexander.deucher, tglx
On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote:
> Using the managed function simplifies the error handling. After
> unloading the driver, the PCI device should now get disabled as
> well.
>
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 02f3bd1..7159018 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
> dev->pdev = pdev;
> pci_set_drvdata(pdev, dev);
>
> - ret = pci_enable_device(pdev);
> + ret = pcim_enable_device(pdev);
> if (ret) {
> drm_err(dev, "failed to enable pci device: %d\n", ret);
> goto err_free;
> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
> ret = hibmc_load(dev);
> if (ret) {
> drm_err(dev, "failed to load hibmc: %d\n", ret);
> - goto err_disable;
> + goto err_free;
> }
>
> ret = drm_dev_register(dev, 0);
> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>
> err_unload:
> hibmc_unload(dev);
> -err_disable:
> - pci_disable_device(pdev);
> err_free:
> drm_dev_put(dev);
The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc
takes care of that already. I'm kinda suprised you don't have a refcount
underrun already - do you test module unload with KASAN enabled?
The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cheers, Daniel
>
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/hisilicon: Use pcim_enable_device()
2020-12-21 22:02 ` Daniel Vetter
@ 2020-12-22 0:38 ` tiantao (H)
2020-12-22 8:27 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: tiantao (H) @ 2020-12-22 0:38 UTC (permalink / raw)
To: Daniel Vetter, Tian Tao
Cc: airlied, dri-devel, xinliang.liu, kraxel, tzimmermann,
alexander.deucher, tglx
在 2020/12/22 6:02, Daniel Vetter 写道:
> On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote:
>> Using the managed function simplifies the error handling. After
>> unloading the driver, the PCI device should now get disabled as
>> well.
>>
>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>> ---
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 02f3bd1..7159018 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>> dev->pdev = pdev;
>> pci_set_drvdata(pdev, dev);
>>
>> - ret = pci_enable_device(pdev);
>> + ret = pcim_enable_device(pdev);
>> if (ret) {
>> drm_err(dev, "failed to enable pci device: %d\n", ret);
>> goto err_free;
>> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>> ret = hibmc_load(dev);
>> if (ret) {
>> drm_err(dev, "failed to load hibmc: %d\n", ret);
>> - goto err_disable;
>> + goto err_free;
>> }
>>
>> ret = drm_dev_register(dev, 0);
>> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>>
>> err_unload:
>> hibmc_unload(dev);
>> -err_disable:
>> - pci_disable_device(pdev);
>> err_free:
>> drm_dev_put(dev);
> The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc
> takes care of that already. I'm kinda suprised you don't have a refcount
> underrun already - do you test module unload with KASAN enabled?
Thanks for helping to review the code，and kindly giving me advice。
this problem have been fixed。
c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free
> The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Cheers, Daniel
>
>>
>> --
>> 2.7.4
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/hisilicon: Use pcim_enable_device()
2020-12-22 0:38 ` tiantao (H)
@ 2020-12-22 8:27 ` Daniel Vetter
2020-12-22 8:31 ` tiantao (H)
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2020-12-22 8:27 UTC (permalink / raw)
To: tiantao (H)
Cc: Dave Airlie, dri-devel, Xinliang Liu, Gerd Hoffmann,
Thomas Zimmermann, Alex Deucher, Tian Tao, Thomas Gleixner
On Tue, Dec 22, 2020 at 1:38 AM tiantao (H) <tiantao6@huawei.com> wrote:
>
>
> 在 2020/12/22 6:02, Daniel Vetter 写道:
> > On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote:
> >> Using the managed function simplifies the error handling. After
> >> unloading the driver, the PCI device should now get disabled as
> >> well.
> >>
> >> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> >> ---
> >> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++----
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> >> index 02f3bd1..7159018 100644
> >> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> >> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> >> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
> >> dev->pdev = pdev;
> >> pci_set_drvdata(pdev, dev);
> >>
> >> - ret = pci_enable_device(pdev);
> >> + ret = pcim_enable_device(pdev);
> >> if (ret) {
> >> drm_err(dev, "failed to enable pci device: %d\n", ret);
> >> goto err_free;
> >> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
> >> ret = hibmc_load(dev);
> >> if (ret) {
> >> drm_err(dev, "failed to load hibmc: %d\n", ret);
> >> - goto err_disable;
> >> + goto err_free;
> >> }
> >>
> >> ret = drm_dev_register(dev, 0);
> >> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
> >>
> >> err_unload:
> >> hibmc_unload(dev);
> >> -err_disable:
> >> - pci_disable_device(pdev);
> >> err_free:
> >> drm_dev_put(dev);
> > The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc
> > takes care of that already. I'm kinda suprised you don't have a refcount
> > underrun already - do you test module unload with KASAN enabled?
>
> Thanks for helping to review the code，and kindly giving me advice。
>
> this problem have been fixed。
>
> c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free
Sorry, I was on an older tree. Note that this fix is incomplete, the
drm_dev_put in the error path of hibmc_pci_probe still exists.
-Daniel
>
> > The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Cheers, Daniel
> >
> >>
> >> --
> >> 2.7.4
> >>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/hisilicon: Use pcim_enable_device()
2020-12-22 8:27 ` Daniel Vetter
@ 2020-12-22 8:31 ` tiantao (H)
0 siblings, 0 replies; 5+ messages in thread
From: tiantao (H) @ 2020-12-22 8:31 UTC (permalink / raw)
To: Daniel Vetter
Cc: Dave Airlie, dri-devel, Xinliang Liu, Gerd Hoffmann,
Thomas Zimmermann, Alex Deucher, Tian Tao, Thomas Gleixner
在 2020/12/22 16:27, Daniel Vetter 写道:
> On Tue, Dec 22, 2020 at 1:38 AM tiantao (H) <tiantao6@huawei.com> wrote:
>>
>> 在 2020/12/22 6:02, Daniel Vetter 写道:
>>> On Mon, Dec 21, 2020 at 08:45:22AM +0800, Tian Tao wrote:
>>>> Using the managed function simplifies the error handling. After
>>>> unloading the driver, the PCI device should now get disabled as
>>>> well.
>>>>
>>>> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
>>>> ---
>>>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++----
>>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> index 02f3bd1..7159018 100644
>>>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>>>> @@ -329,7 +329,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>>>> dev->pdev = pdev;
>>>> pci_set_drvdata(pdev, dev);
>>>>
>>>> - ret = pci_enable_device(pdev);
>>>> + ret = pcim_enable_device(pdev);
>>>> if (ret) {
>>>> drm_err(dev, "failed to enable pci device: %d\n", ret);
>>>> goto err_free;
>>>> @@ -338,7 +338,7 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>>>> ret = hibmc_load(dev);
>>>> if (ret) {
>>>> drm_err(dev, "failed to load hibmc: %d\n", ret);
>>>> - goto err_disable;
>>>> + goto err_free;
>>>> }
>>>>
>>>> ret = drm_dev_register(dev, 0);
>>>> @@ -354,8 +354,6 @@ static int hibmc_pci_probe(struct pci_dev *pdev,
>>>>
>>>> err_unload:
>>>> hibmc_unload(dev);
>>>> -err_disable:
>>>> - pci_disable_device(pdev);
>>>> err_free:
>>>> drm_dev_put(dev);
>>> The drm_dev_put here and in hibmc_pci_remove is wrong, devm_drm_dev_alloc
>>> takes care of that already. I'm kinda suprised you don't have a refcount
>>> underrun already - do you test module unload with KASAN enabled?
>> Thanks for helping to review the code，and kindly giving me advice。
>>
>> this problem have been fixed。
>>
>> c855af2f9c5c60760fd1bed7889a81bc37d2591d drm/hisilicon: Fix use-after-free
> Sorry, I was on an older tree. Note that this fix is incomplete, the
> drm_dev_put in the error path of hibmc_pci_probe still exists.
> -Daniel
I also found the problem, has raised the patch in the internal review
and testing, to confirm that there is no problem, it will be sent out.
Thanks for the reminder.
>>> The pcim patch looks ok, so Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> Cheers, Daniel
>>>
>>>> --
>>>> 2.7.4
>>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-22 9:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 0:45 [PATCH] drm/hisilicon: Use pcim_enable_device() Tian Tao
2020-12-21 22:02 ` Daniel Vetter
2020-12-22 0:38 ` tiantao (H)
2020-12-22 8:27 ` Daniel Vetter
2020-12-22 8:31 ` tiantao (H)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.