[PATCH v2 2/4] drm/fb-helper: Keep references for the current set of used connectors

[Chris Wilson]

On Wed, Oct 26, 2016 at 04:31:20PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> The fbdev helper code keeps around two lists of connectors. One is the
> list of all connectors it could use, and that list already holds
> references for all the connectors. However the other list, or rather
> lists, is the one actively being used. That list is tracked per-crtc
> and currently doesn't hold any extra references. Let's grab those
> extra references to avoid oopsing when the connector vanishes. The
> list of all possible connectors should get updated when the hpd happens,
> but the list of actively used connectors would not get updated until
> the next time the fb-helper picks through the set of possible connectors.
> And so we need to hang on to the connectors until that time.
> 
> Since we need to clean up in drm_fb_helper_crtc_free() as well,
> let's pull the code to a common place. And while at it let's
> pull in up the modeset->mode cleanup in there as well. The case
> of modeset->fb is a bit less clear. I'm thinking we should probably
> hold a reference to it, but for now I just slapped on a FIXME.
> 
> v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: stable at vger.kernel.org
> Cc: Carlos Santa <carlos.santa at intel.com>
> Cc: Kirill A. Shutemov <kirill at shutemov.name>
> Tested-by: Carlos Santa <carlos.santa at intel.com> (v1)
> Tested-by: Kirill A. Shutemov <kirill at shutemov.name> (v1)
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 58 +++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index db469d12d195..83961f1a97d2 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_blank);
>  
> +static void drm_fb_helper_modeset_free(struct drm_fb_helper *helper,
> +				       struct drm_mode_set *modeset)
> +{
> +	int i;
> +
> +	for (i = 0; i < modeset->num_connectors; i++) {
> +		drm_connector_unreference(modeset->connectors[i]);
> +		modeset->connectors[i] = NULL;
> +	}
> +	modeset->num_connectors = 0;
> +
> +	drm_mode_destroy(helper->dev, modeset->mode);
> +	modeset->mode = NULL;
> +
> +	/* FIXME should hold a ref? */
> +	modeset->fb = NULL;
> +}
> +
>  static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
>  {
>  	int i;
> @@ -614,11 +632,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
>  		kfree(helper->connector_info[i]);
>  	}
>  	kfree(helper->connector_info);
> -	for (i = 0; i < helper->crtc_count; i++) {
> -		kfree(helper->crtc_info[i].mode_set.connectors);
> -		if (helper->crtc_info[i].mode_set.mode)
> -			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
> -	}
> +
> +	for (i = 0; i < helper->crtc_count; i++)
> +		drm_fb_helper_modeset_free(helper,
> +					   &helper->crtc_info[i].mode_set);

We lose the kfree(mode_set.connectors) here.

So 
	for (i = 0; i < helper->crtc_count; i++)
		struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set);

		drm_fb_helper_modeset_release(helper, modeset);
		kfree(modeset->connectors);
	}
?

Couldn't spot any other missing calls to release the new ref, so with
the tiny leak fixed,
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre