* [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure @ 2015-12-08 22:48 Matt Roper 2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper 2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Matt Roper @ 2015-12-08 22:48 UTC (permalink / raw) To: intel-gfx 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> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: Drop unnecessary NULL test 2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper @ 2015-12-08 22:48 ` Matt Roper 2015-12-10 14:15 ` Daniel Vetter 2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter 1 sibling, 1 reply; 6+ messages in thread From: Matt Roper @ 2015-12-08 22:48 UTC (permalink / raw) To: intel-gfx We can never be passed a NULL crtc_state (and if we could there are plenty of other places later in this function where we access it without testing for NULL). Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2e8d1a8..51980a9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11808,7 +11808,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, bool turn_off, turn_on, visible, was_visible; struct drm_framebuffer *fb = plane_state->fb; - if (crtc_state && INTEL_INFO(dev)->gen >= 9 && + if (INTEL_INFO(dev)->gen >= 9 && plane->type != DRM_PLANE_TYPE_CURSOR) { ret = skl_update_scaler_plane( to_intel_crtc_state(crtc_state), -- 2.1.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915: Drop unnecessary NULL test 2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper @ 2015-12-10 14:15 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2015-12-10 14:15 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Tue, Dec 08, 2015 at 02:48:52PM -0800, Matt Roper wrote: > We can never be passed a NULL crtc_state (and if we could there are > plenty of other places later in this function where we access it > without testing for NULL). > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Yup, atomic state structures better exist once we can do atomic_check/commit(). Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2e8d1a8..51980a9 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11808,7 +11808,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > bool turn_off, turn_on, visible, was_visible; > struct drm_framebuffer *fb = plane_state->fb; > > - if (crtc_state && INTEL_INFO(dev)->gen >= 9 && > + if (INTEL_INFO(dev)->gen >= 9 && > plane->type != DRM_PLANE_TYPE_CURSOR) { > ret = skl_update_scaler_plane( > to_intel_crtc_state(crtc_state), > -- > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure 2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper 2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper @ 2015-12-10 14:14 ` Daniel Vetter 2015-12-11 1:57 ` Matt Roper 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2015-12-10 14:14 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx 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. None of the other connectors seem to have this issue (or at least I don't see a patch for them). -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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure 2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter @ 2015-12-11 1:57 ` Matt Roper 2015-12-11 17:28 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Matt Roper @ 2015-12-11 1:57 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure 2015-12-11 1:57 ` Matt Roper @ 2015-12-11 17:28 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2015-12-11 17:28 UTC (permalink / raw) To: Matt Roper; +Cc: intel-gfx On Thu, Dec 10, 2015 at 05:57:57PM -0800, Matt Roper wrote: > 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). Ah right, didn't find that. Started a full audit but stuck at intel_dp_init. That has a kfree(intel_connector), but seems to miss this too. On the patch itself: Can't we just call drm_connector_cleanup? And an intel_connectore_free which wraps that + the kfree would be nice. Well, that would mean we'd need to move drm_connector_init into intel_connector_init, but I think that would make a lot of sense either way. -Daniel > > > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-11 17:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-12-08 22:48 [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Matt Roper 2015-12-08 22:48 ` [PATCH 2/2] drm/i915: Drop unnecessary NULL test Matt Roper 2015-12-10 14:15 ` Daniel Vetter 2015-12-10 14:14 ` [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure Daniel Vetter 2015-12-11 1:57 ` Matt Roper 2015-12-11 17:28 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox