Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure

From: Matt Roper <matthew.d.roper@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
Date: Thu, 10 Dec 2015 17:57:57 -0800	[thread overview]
Message-ID: <20151211015757.GA855@intel.com> (raw)
In-Reply-To: <20151210141438.GR20822@phenom.ffwll.local>

On Thu, Dec 10, 2015 at 03:14:38PM +0100, Daniel Vetter wrote:
> On Tue, Dec 08, 2015 at 02:48:51PM -0800, Matt Roper wrote:
> > In all of our various SDVO setup functions, we allocate an SDVO
> > connector (along with an associated connector->state) object, then
> > perform initialization.  If that initialization fails, we need to make
> > sure to free the state object as well as the connector.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Where do we alloc connector->state in sdvo_connector_init()? I thought the
> flow is 1) register all the kms objects with our pile of _init() functions
> 2) do hw readout, which is the thing which creates all these states.

It's allocated by intel_sdvo_connector_alloc -> intel_connector_init
which gets run before intel_sdvo_connector_init() even gets called.
There are two separate "init" functions that get called for SDVO
connectors...the first allocates the state object, but the second can
fail too and trigger cleanup (which was failing to destroy the state).

> 
> None of the other connectors seem to have this issue (or at least I don't
> see a patch for them).

I don't think any of the other connector types have a second, special
init function that gets run after intel_connector_init() is finished
that I can see. 

Matt

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 06679f1..ff28867 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> >  	}
> >  
> >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > +		drm_atomic_helper_connector_destroy_state(connector,
> > +							  connector->state);
> >  		kfree(intel_sdvo_connector);
> >  		return false;
> >  	}
> > @@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> >  	intel_sdvo->is_tv = true;
> >  
> >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > +		drm_atomic_helper_connector_destroy_state(connector,
> > +							  connector->state);
> >  		kfree(intel_sdvo_connector);
> >  		return false;
> >  	}
> > @@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> >  	}
> >  
> >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > +		drm_atomic_helper_connector_destroy_state(connector,
> > +							  connector->state);
> >  		kfree(intel_sdvo_connector);
> >  		return false;
> >  	}
> > @@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> >  	}
> >  
> >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > +		drm_atomic_helper_connector_destroy_state(connector,
> > +							  connector->state);
> >  		kfree(intel_sdvo_connector);
> >  		return false;
> >  	}
> > -- 
> > 2.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx