Re: [PATCH 3/9] drm/i915: fix memleak in intel_set_config_save_state()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> If the save_encoder_crtcs or save_connector_encoders allocation fail
> we need to free everything we have already allocated.

There is no memleak, because the caller of intel_set_config_save_state()
checks the return value, and subsequently handles the error with a call
to intel_set_config_free(), which does the right thing.

I know one could argue this should be done within
intel_set_config_save_state() but I'm not convinced. I'd let this be as
it is.

Just in case Daniel disagrees with me here and wants to merge, the patch
does look correct. So r-b for correctness, but nak on merging from
me. ;)

BR,
Jani.


>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 05937fe..a8a8abe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11018,13 +11018,13 @@ static int intel_set_config_save_state(struct drm_device *dev,
>  		kcalloc(dev->mode_config.num_encoder,
>  			sizeof(struct drm_crtc *), GFP_KERNEL);
>  	if (!config->save_encoder_crtcs)
> -		return -ENOMEM;
> +		goto free_crtc;
>  
>  	config->save_connector_encoders =
>  		kcalloc(dev->mode_config.num_connector,
>  			sizeof(struct drm_encoder *), GFP_KERNEL);
>  	if (!config->save_connector_encoders)
> -		return -ENOMEM;
> +		goto free_encoder;
>  
>  	/* Copy data. Note that driver private data is not affected.
>  	 * Should anything bad happen only the expected state is
> @@ -11046,6 +11046,12 @@ static int intel_set_config_save_state(struct drm_device *dev,
>  	}
>  
>  	return 0;
> +
> +free_encoder:
> +	kfree(config->save_encoder_crtcs);
> +free_crtc:
> +	kfree(config->save_crtc_enabled);
> +	return -ENOMEM;
>  }
>  
>  static void intel_set_config_restore_state(struct drm_device *dev,
> -- 
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center