Re: [PATCH 1/3] drm/tegra: Fix error handling cleanup

[Sean Paul <seanpaul@chromium.org>]

On Thu, Nov 06, 2014 at 04:57:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The DRM driver's ->load() implementation didn't do a good job (no job at
> all really) cleaning up on failure. Fix that by undoing any prior setup
> when an error occurs. This requires a bit of rework to make it possible
> to clean up fbdev midway.
> 
> This was tested by injecting errors at various points during the
> initialization sequence and verifying that error cleanup didn't crash
> and no memory leaked (using kmemleak).
> 
> Reported-by: Stéphane Marchesin <marcheu@google.com>
> Reported-by: Sean Paul <seanpaul@google.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Looks much better :-)

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/tegra/drm.c | 20 ++++++++++++++++----
>  drivers/gpu/drm/tegra/drm.h |  1 +
>  drivers/gpu/drm/tegra/fb.c  | 20 +++++++++++++++++---
>  3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 59736bb810cd..e1632fb03a89 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -42,13 +42,13 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
>  
>  	err = tegra_drm_fb_prepare(drm);
>  	if (err < 0)
> -		return err;
> +		goto config;
>  
>  	drm_kms_helper_poll_init(drm);
>  
>  	err = host1x_device_init(device);
>  	if (err < 0)
> -		return err;
> +		goto fbdev;
>  
>  	/*
>  	 * We don't use the drm_irq_install() helpers provided by the DRM
> @@ -59,13 +59,25 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
>  
>  	err = drm_vblank_init(drm, drm->mode_config.num_crtc);
>  	if (err < 0)
> -		return err;
> +		goto device;
>  
>  	err = tegra_drm_fb_init(drm);
>  	if (err < 0)
> -		return err;
> +		goto vblank;
>  
>  	return 0;
> +
> +vblank:
> +	drm_vblank_cleanup(drm);
> +device:
> +	host1x_device_exit(device);
> +fbdev:
> +	drm_kms_helper_poll_fini(drm);
> +	tegra_drm_fb_free(drm);
> +config:
> +	drm_mode_config_cleanup(drm);
> +	kfree(tegra);
> +	return err;
>  }
>  
>  static int tegra_drm_unload(struct drm_device *drm)
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index b994c017971d..ef2faaef5936 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -288,6 +288,7 @@ bool tegra_fb_is_bottom_up(struct drm_framebuffer *framebuffer);
>  int tegra_fb_get_tiling(struct drm_framebuffer *framebuffer,
>  			struct tegra_bo_tiling *tiling);
>  int tegra_drm_fb_prepare(struct drm_device *drm);
> +void tegra_drm_fb_free(struct drm_device *drm);
>  int tegra_drm_fb_init(struct drm_device *drm);
>  void tegra_drm_fb_exit(struct drm_device *drm);
>  #ifdef CONFIG_DRM_TEGRA_FBDEV
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index 3513d12d5aa1..0474241ac2a4 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -289,6 +289,11 @@ static struct tegra_fbdev *tegra_fbdev_create(struct drm_device *drm)
>  	return fbdev;
>  }
>  
> +static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
> +{
> +	kfree(fbdev);
> +}
> +
>  static int tegra_fbdev_init(struct tegra_fbdev *fbdev,
>  			    unsigned int preferred_bpp,
>  			    unsigned int num_crtc,
> @@ -322,7 +327,7 @@ fini:
>  	return err;
>  }
>  
> -static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
> +static void tegra_fbdev_exit(struct tegra_fbdev *fbdev)
>  {
>  	struct fb_info *info = fbdev->base.fbdev;
>  
> @@ -345,7 +350,7 @@ static void tegra_fbdev_free(struct tegra_fbdev *fbdev)
>  	}
>  
>  	drm_fb_helper_fini(&fbdev->base);
> -	kfree(fbdev);
> +	tegra_fbdev_free(fbdev);
>  }
>  
>  void tegra_fbdev_restore_mode(struct tegra_fbdev *fbdev)
> @@ -393,6 +398,15 @@ int tegra_drm_fb_prepare(struct drm_device *drm)
>  	return 0;
>  }
>  
> +void tegra_drm_fb_free(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DRM_TEGRA_FBDEV
> +	struct tegra_drm *tegra = drm->dev_private;
> +
> +	tegra_fbdev_free(tegra->fbdev);
> +#endif
> +}
> +
>  int tegra_drm_fb_init(struct drm_device *drm)
>  {
>  #ifdef CONFIG_DRM_TEGRA_FBDEV
> @@ -413,6 +427,6 @@ void tegra_drm_fb_exit(struct drm_device *drm)
>  #ifdef CONFIG_DRM_TEGRA_FBDEV
>  	struct tegra_drm *tegra = drm->dev_private;
>  
> -	tegra_fbdev_free(tegra->fbdev);
> +	tegra_fbdev_exit(tegra->fbdev);
>  #endif
>  }
> -- 
> 2.1.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel