diff mbox series

[57/59] drm/ast: Use managed pci functions

Message ID 20200415074034.175360-58-daniel.vetter@ffwll.ch
State New, archived
Headers show
Series devm_drm_dev_alloc, v2 | expand

Commit Message

Daniel Vetter April 15, 2020, 7:40 a.m. UTC
Allows us to remove a bit of cleanup code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
---
 drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
 drivers/gpu/drm/ast/ast_main.c |  3 ---
 2 files changed, 3 insertions(+), 10 deletions(-)

Comments

Thomas Zimmermann April 15, 2020, 7:52 a.m. UTC | #1
Hi Daniel

Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> Allows us to remove a bit of cleanup code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> ---
>  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
>  drivers/gpu/drm/ast/ast_main.c |  3 ---
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index b7ba22dddcad..48a9cc4e080a 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	ast_kick_out_firmware_fb(pdev);
>  
> -	ret = pci_enable_device(pdev);
> +	ret = pcim_enable_device(pdev);
>  	if (ret)
>  		return ret;
>  
>  	dev = drm_dev_alloc(&driver, &pdev->dev);
> -	if (IS_ERR(dev)) {
> -		ret = PTR_ERR(dev);
> -		goto err_pci_disable_device;
> -	}
> +	if (IS_ERR(dev))
> +		return  PTR_ERR(dev);
>  
>  	dev->pdev = pdev;
>  	pci_set_drvdata(pdev, dev);
> @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	ast_driver_unload(dev);
>  err_drm_dev_put:
>  	drm_dev_put(dev);
> -err_pci_disable_device:
> -	pci_disable_device(pdev);
>  	return ret;
>  
>  }
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index e5398e3dabe7..1b35728ad871 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
>  	drm_mode_config_cleanup(dev);
>  
>  	ast_mm_fini(ast);
> -	if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> -		pci_iounmap(dev->pdev, ast->ioregs);
> -	pci_iounmap(dev->pdev, ast->regs);

This gets unmapped as part of the automatic pci_disable_device(), I guess?

Do we need drm_dev_enter()/_exit() to make I/O work reliably?

Best regards
Thomas

>  	kfree(ast);
>  }
>
Daniel Vetter April 15, 2020, 8:09 a.m. UTC | #2
On Wed, Apr 15, 2020 at 9:52 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi Daniel
>
> Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> > Allows us to remove a bit of cleanup code.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Emil Velikov <emil.velikov@collabora.com>
> > Cc: "Noralf Trønnes" <noralf@tronnes.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> > ---
> >  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
> >  drivers/gpu/drm/ast/ast_main.c |  3 ---
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index b7ba22dddcad..48a9cc4e080a 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       ast_kick_out_firmware_fb(pdev);
> >
> > -     ret = pci_enable_device(pdev);
> > +     ret = pcim_enable_device(pdev);
> >       if (ret)
> >               return ret;
> >
> >       dev = drm_dev_alloc(&driver, &pdev->dev);
> > -     if (IS_ERR(dev)) {
> > -             ret = PTR_ERR(dev);
> > -             goto err_pci_disable_device;
> > -     }
> > +     if (IS_ERR(dev))
> > +             return  PTR_ERR(dev);
> >
> >       dev->pdev = pdev;
> >       pci_set_drvdata(pdev, dev);
> > @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       ast_driver_unload(dev);
> >  err_drm_dev_put:
> >       drm_dev_put(dev);
> > -err_pci_disable_device:
> > -     pci_disable_device(pdev);
> >       return ret;
> >
> >  }
> > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > index e5398e3dabe7..1b35728ad871 100644
> > --- a/drivers/gpu/drm/ast/ast_main.c
> > +++ b/drivers/gpu/drm/ast/ast_main.c
> > @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
> >       drm_mode_config_cleanup(dev);
> >
> >       ast_mm_fini(ast);
> > -     if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> > -             pci_iounmap(dev->pdev, ast->ioregs);
> > -     pci_iounmap(dev->pdev, ast->regs);
>
> This gets unmapped as part of the automatic pci_disable_device(), I guess?

Yup, once you go with pcim_enable_device all pci_ functions on that
device become manged and auto-cleanup.

> Do we need drm_dev_enter()/_exit() to make I/O work reliably?

That does nothing without drm_dev_unplug(), which has the annoying
side effect that it also shuts up stuff like
drm_atomic_helper_shutdown for module unload. And developers really
want their devices to be shut off on driver unload. So yeah
unfortunately we currently can decide between "correct for hotunplug"
and "convenient for driver unload for driver authors". I'm not sure
what to best do here, since all options are kinda not great for one
use-case or the other.
-Daniel

> Best regards
> Thomas
>
> >       kfree(ast);
> >  }
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
Daniel Vetter April 15, 2020, 8:17 a.m. UTC | #3
On Wed, Apr 15, 2020 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Wed, Apr 15, 2020 at 9:52 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Hi Daniel
> >
> > Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> > > Allows us to remove a bit of cleanup code.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Emil Velikov <emil.velikov@collabora.com>
> > > Cc: "Noralf Trønnes" <noralf@tronnes.org>
> > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> > > ---
> > >  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
> > >  drivers/gpu/drm/ast/ast_main.c |  3 ---
> > >  2 files changed, 3 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > > index b7ba22dddcad..48a9cc4e080a 100644
> > > --- a/drivers/gpu/drm/ast/ast_drv.c
> > > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > > @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >
> > >       ast_kick_out_firmware_fb(pdev);
> > >
> > > -     ret = pci_enable_device(pdev);
> > > +     ret = pcim_enable_device(pdev);
> > >       if (ret)
> > >               return ret;
> > >
> > >       dev = drm_dev_alloc(&driver, &pdev->dev);
> > > -     if (IS_ERR(dev)) {
> > > -             ret = PTR_ERR(dev);
> > > -             goto err_pci_disable_device;
> > > -     }
> > > +     if (IS_ERR(dev))
> > > +             return  PTR_ERR(dev);
> > >
> > >       dev->pdev = pdev;
> > >       pci_set_drvdata(pdev, dev);
> > > @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >       ast_driver_unload(dev);
> > >  err_drm_dev_put:
> > >       drm_dev_put(dev);
> > > -err_pci_disable_device:
> > > -     pci_disable_device(pdev);
> > >       return ret;
> > >
> > >  }
> > > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > > index e5398e3dabe7..1b35728ad871 100644
> > > --- a/drivers/gpu/drm/ast/ast_main.c
> > > +++ b/drivers/gpu/drm/ast/ast_main.c
> > > @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
> > >       drm_mode_config_cleanup(dev);
> > >
> > >       ast_mm_fini(ast);
> > > -     if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> > > -             pci_iounmap(dev->pdev, ast->ioregs);
> > > -     pci_iounmap(dev->pdev, ast->regs);
> >
> > This gets unmapped as part of the automatic pci_disable_device(), I guess?
>
> Yup, once you go with pcim_enable_device all pci_ functions on that
> device become manged and auto-cleanup.
>
> > Do we need drm_dev_enter()/_exit() to make I/O work reliably?
>
> That does nothing without drm_dev_unplug(), which has the annoying
> side effect that it also shuts up stuff like
> drm_atomic_helper_shutdown for module unload. And developers really
> want their devices to be shut off on driver unload. So yeah
> unfortunately we currently can decide between "correct for hotunplug"
> and "convenient for driver unload for driver authors". I'm not sure
> what to best do here, since all options are kinda not great for one
> use-case or the other.

So if we'd split up drm_dev_unplug into drm_dev_unregister +
drm_dev_mark_unplugged or whatever the options would be:

drm_atomic_helper_shutdown(); /* and other hw shut down */
drm_dev_unregister();
drm_dev_mark_unplugged();

Kinda annoying since userspace might race with us, so we might still
have an active fb (in sw tracking at least) that we need to clean up
again later on. Plus shutting down hw before we unregister is going to
make the hotunplug confusion for userspace even more a mess.

Next option:
drm_dev_unregster();
/* shut down hw */
drm_dev_mark_unplugged();

This just wastes time since if we're really unplugged we'll do lots of
io attempts that go nowhere because the device is gone already.
They'll all time out (if the bus subsystem/hw for our driver works
correctly at least). Plus userspace can still sneak in and do stupid
stuff I think while we're not looking.

Next up:
drm_dev_unregister();
drm_dev_mark_unplugged();
drm_atomic_helper_shutdown();

This is a bit silly since all the shutdown code now does is shut down
sw state, and never touches hw (if the driver is fully annotated with
drm_dev_enter/exit).

One option might be that we slap a lot more drm_dev_enter/exit around
the top-level ioctls (to at least stop the userspace races with driver
unload), and allow drivers to still shut down stuff internally.

tldr; it's all still a bit a mess.
-Daniel

> -Daniel
>
> > Best regards
> > Thomas
> >
> > >       kfree(ast);
> > >  }
> > >
> >
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > (HRB 36809, AG Nürnberg)
> > Geschäftsführer: Felix Imendörffer
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter April 15, 2020, 12:23 p.m. UTC | #4
On Wed, Apr 15, 2020 at 10:17 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Wed, Apr 15, 2020 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > On Wed, Apr 15, 2020 at 9:52 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > >
> > > Hi Daniel
> > >
> > > Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> > > > Allows us to remove a bit of cleanup code.
> > > >
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Emil Velikov <emil.velikov@collabora.com>
> > > > Cc: "Noralf Trønnes" <noralf@tronnes.org>
> > > > Cc: Sam Ravnborg <sam@ravnborg.org>
> > > > Cc: "Christian König" <christian.koenig@amd.com>
> > > > Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> > > > ---
> > > >  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
> > > >  drivers/gpu/drm/ast/ast_main.c |  3 ---
> > > >  2 files changed, 3 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > > > index b7ba22dddcad..48a9cc4e080a 100644
> > > > --- a/drivers/gpu/drm/ast/ast_drv.c
> > > > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > > > @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > >
> > > >       ast_kick_out_firmware_fb(pdev);
> > > >
> > > > -     ret = pci_enable_device(pdev);
> > > > +     ret = pcim_enable_device(pdev);
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > >       dev = drm_dev_alloc(&driver, &pdev->dev);
> > > > -     if (IS_ERR(dev)) {
> > > > -             ret = PTR_ERR(dev);
> > > > -             goto err_pci_disable_device;
> > > > -     }
> > > > +     if (IS_ERR(dev))
> > > > +             return  PTR_ERR(dev);
> > > >
> > > >       dev->pdev = pdev;
> > > >       pci_set_drvdata(pdev, dev);
> > > > @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > > >       ast_driver_unload(dev);
> > > >  err_drm_dev_put:
> > > >       drm_dev_put(dev);
> > > > -err_pci_disable_device:
> > > > -     pci_disable_device(pdev);
> > > >       return ret;
> > > >
> > > >  }
> > > > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > > > index e5398e3dabe7..1b35728ad871 100644
> > > > --- a/drivers/gpu/drm/ast/ast_main.c
> > > > +++ b/drivers/gpu/drm/ast/ast_main.c
> > > > @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
> > > >       drm_mode_config_cleanup(dev);
> > > >
> > > >       ast_mm_fini(ast);
> > > > -     if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> > > > -             pci_iounmap(dev->pdev, ast->ioregs);
> > > > -     pci_iounmap(dev->pdev, ast->regs);
> > >
> > > This gets unmapped as part of the automatic pci_disable_device(), I guess?
> >
> > Yup, once you go with pcim_enable_device all pci_ functions on that
> > device become manged and auto-cleanup.
> >
> > > Do we need drm_dev_enter()/_exit() to make I/O work reliably?
> >
> > That does nothing without drm_dev_unplug(), which has the annoying
> > side effect that it also shuts up stuff like
> > drm_atomic_helper_shutdown for module unload. And developers really
> > want their devices to be shut off on driver unload. So yeah
> > unfortunately we currently can decide between "correct for hotunplug"
> > and "convenient for driver unload for driver authors". I'm not sure
> > what to best do here, since all options are kinda not great for one
> > use-case or the other.
>
> So if we'd split up drm_dev_unplug into drm_dev_unregister +
> drm_dev_mark_unplugged or whatever the options would be:
>
> drm_atomic_helper_shutdown(); /* and other hw shut down */
> drm_dev_unregister();
> drm_dev_mark_unplugged();
>
> Kinda annoying since userspace might race with us, so we might still
> have an active fb (in sw tracking at least) that we need to clean up
> again later on. Plus shutting down hw before we unregister is going to
> make the hotunplug confusion for userspace even more a mess.
>
> Next option:
> drm_dev_unregster();
> /* shut down hw */
> drm_dev_mark_unplugged();
>
> This just wastes time since if we're really unplugged we'll do lots of
> io attempts that go nowhere because the device is gone already.
> They'll all time out (if the bus subsystem/hw for our driver works
> correctly at least). Plus userspace can still sneak in and do stupid
> stuff I think while we're not looking.
>
> Next up:
> drm_dev_unregister();
> drm_dev_mark_unplugged();
> drm_atomic_helper_shutdown();
>
> This is a bit silly since all the shutdown code now does is shut down
> sw state, and never touches hw (if the driver is fully annotated with
> drm_dev_enter/exit).
>
> One option might be that we slap a lot more drm_dev_enter/exit around
> the top-level ioctls (to at least stop the userspace races with driver
> unload), and allow drivers to still shut down stuff internally.
>
> tldr; it's all still a bit a mess.

Adding Laurent, I just discussed this a bit with him on irc.
-Daniel
Thomas Zimmermann June 11, 2020, 12:04 p.m. UTC | #5
Hi

Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> Allows us to remove a bit of cleanup code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: "Noralf Trønnes" <noralf@tronnes.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for answering my questions. Sorry for never getting back to it.

Best regards
Thomas

> ---
>  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
>  drivers/gpu/drm/ast/ast_main.c |  3 ---
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index b7ba22dddcad..48a9cc4e080a 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	ast_kick_out_firmware_fb(pdev);
>  
> -	ret = pci_enable_device(pdev);
> +	ret = pcim_enable_device(pdev);
>  	if (ret)
>  		return ret;
>  
>  	dev = drm_dev_alloc(&driver, &pdev->dev);
> -	if (IS_ERR(dev)) {
> -		ret = PTR_ERR(dev);
> -		goto err_pci_disable_device;
> -	}
> +	if (IS_ERR(dev))
> +		return  PTR_ERR(dev);
>  
>  	dev->pdev = pdev;
>  	pci_set_drvdata(pdev, dev);
> @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	ast_driver_unload(dev);
>  err_drm_dev_put:
>  	drm_dev_put(dev);
> -err_pci_disable_device:
> -	pci_disable_device(pdev);
>  	return ret;
>  
>  }
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index e5398e3dabe7..1b35728ad871 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
>  	drm_mode_config_cleanup(dev);
>  
>  	ast_mm_fini(ast);
> -	if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> -		pci_iounmap(dev->pdev, ast->ioregs);
> -	pci_iounmap(dev->pdev, ast->regs);
>  	kfree(ast);
>  }
>
Daniel Vetter June 16, 2020, 11:55 a.m. UTC | #6
On Thu, Jun 11, 2020 at 02:04:03PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> > Allows us to remove a bit of cleanup code.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Emil Velikov <emil.velikov@collabora.com>
> > Cc: "Noralf Trønnes" <noralf@tronnes.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks for answering my questions. Sorry for never getting back to it.

Nw, patch applied now, thanks for your review.
-Daniel

> 
> Best regards
> Thomas
> 
> > ---
> >  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
> >  drivers/gpu/drm/ast/ast_main.c |  3 ---
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index b7ba22dddcad..48a9cc4e080a 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  
> >  	ast_kick_out_firmware_fb(pdev);
> >  
> > -	ret = pci_enable_device(pdev);
> > +	ret = pcim_enable_device(pdev);
> >  	if (ret)
> >  		return ret;
> >  
> >  	dev = drm_dev_alloc(&driver, &pdev->dev);
> > -	if (IS_ERR(dev)) {
> > -		ret = PTR_ERR(dev);
> > -		goto err_pci_disable_device;
> > -	}
> > +	if (IS_ERR(dev))
> > +		return  PTR_ERR(dev);
> >  
> >  	dev->pdev = pdev;
> >  	pci_set_drvdata(pdev, dev);
> > @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	ast_driver_unload(dev);
> >  err_drm_dev_put:
> >  	drm_dev_put(dev);
> > -err_pci_disable_device:
> > -	pci_disable_device(pdev);
> >  	return ret;
> >  
> >  }
> > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > index e5398e3dabe7..1b35728ad871 100644
> > --- a/drivers/gpu/drm/ast/ast_main.c
> > +++ b/drivers/gpu/drm/ast/ast_main.c
> > @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
> >  	drm_mode_config_cleanup(dev);
> >  
> >  	ast_mm_fini(ast);
> > -	if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> > -		pci_iounmap(dev->pdev, ast->ioregs);
> > -	pci_iounmap(dev->pdev, ast->regs);
> >  	kfree(ast);
> >  }
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index b7ba22dddcad..48a9cc4e080a 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -91,15 +91,13 @@  static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	ast_kick_out_firmware_fb(pdev);
 
-	ret = pci_enable_device(pdev);
+	ret = pcim_enable_device(pdev);
 	if (ret)
 		return ret;
 
 	dev = drm_dev_alloc(&driver, &pdev->dev);
-	if (IS_ERR(dev)) {
-		ret = PTR_ERR(dev);
-		goto err_pci_disable_device;
-	}
+	if (IS_ERR(dev))
+		return  PTR_ERR(dev);
 
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
@@ -120,8 +118,6 @@  static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	ast_driver_unload(dev);
 err_drm_dev_put:
 	drm_dev_put(dev);
-err_pci_disable_device:
-	pci_disable_device(pdev);
 	return ret;
 
 }
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index e5398e3dabe7..1b35728ad871 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -531,8 +531,5 @@  void ast_driver_unload(struct drm_device *dev)
 	drm_mode_config_cleanup(dev);
 
 	ast_mm_fini(ast);
-	if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
-		pci_iounmap(dev->pdev, ast->ioregs);
-	pci_iounmap(dev->pdev, ast->regs);
 	kfree(ast);
 }