| Message ID | 1449614932-20258-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }
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(+)