intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures
@ 2012-11-12 16:31 Jani Nikula
  2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jani Nikula @ 2012-11-12 16:31 UTC (permalink / raw)
  To: intel-gfx, daniel; +Cc: Jani Nikula

Any failures in intel_sdvo_init() after the intel_sdvo_setup_output() call
left behind ghost connectors, attached (with a dangling pointer) to the
sdvo that has been cleaned up and freed. Properly destroy any connectors
attached to the encoder.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=46381
CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

This is *completely* untested, and I don't really know if the connector
loop is okay or naïve in some way... Please review and test carefully. CC
Chris in case this addresses his SDVO EDID failures too.
---
 drivers/gpu/drm/i915/intel_sdvo.c |   21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 30f1752..6456427 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2361,6 +2361,18 @@ intel_sdvo_output_setup(struct intel_sdvo *intel_sdvo, uint16_t flags)
 	return true;
 }
 
+static void intel_sdvo_output_cleanup(struct intel_sdvo *intel_sdvo)
+{
+	struct drm_device *dev = intel_sdvo->base.base.dev;
+	struct drm_connector *connector, *tmp;
+
+	list_for_each_entry_safe(connector, tmp,
+				 &dev->mode_config.connector_list, head) {
+		if (intel_attached_encoder(connector) == &intel_sdvo->base)
+			intel_sdvo_destroy(connector);
+	}
+}
+
 static bool intel_sdvo_tv_create_property(struct intel_sdvo *intel_sdvo,
 					  struct intel_sdvo_connector *intel_sdvo_connector,
 					  int type)
@@ -2682,7 +2694,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 				    intel_sdvo->caps.output_flags) != true) {
 		DRM_DEBUG_KMS("SDVO output failed to setup on %s\n",
 			      SDVO_NAME(intel_sdvo));
-		goto err;
+		goto err_output;
 	}
 
 	/* Only enable the hotplug irq if we need it, to work around noisy
@@ -2695,12 +2707,12 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 
 	/* Set the input timing to the screen. Assume always input 0. */
 	if (!intel_sdvo_set_target_input(intel_sdvo))
-		goto err;
+		goto err_output;
 
 	if (!intel_sdvo_get_input_pixel_clock_range(intel_sdvo,
 						    &intel_sdvo->pixel_clock_min,
 						    &intel_sdvo->pixel_clock_max))
-		goto err;
+		goto err_output;
 
 	DRM_DEBUG_KMS("%s device VID/DID: %02X:%02X.%02X, "
 			"clock range %dMHz - %dMHz, "
@@ -2720,6 +2732,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
 			(SDVO_OUTPUT_TMDS1 | SDVO_OUTPUT_RGB1) ? 'Y' : 'N');
 	return true;
 
+err_output:
+	intel_sdvo_output_cleanup(intel_sdvo);
+
 err:
 	drm_encoder_cleanup(&intel_encoder->base);
 	i2c_del_adapter(&intel_sdvo->ddc);
-- 
1.7.9.5

_______________________________________________
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/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy
  2012-11-12 16:31 [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Jani Nikula
@ 2012-11-12 16:31 ` Jani Nikula
  2012-11-12 16:48   ` Ville Syrjälä
  2012-11-22 13:10   ` Daniel Vetter
  2012-11-12 16:52 ` [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Chris Wilson
  2012-11-12 19:09 ` Daniel Vetter
  2 siblings, 2 replies; 6+ messages in thread
From: Jani Nikula @ 2012-11-12 16:31 UTC (permalink / raw)
  To: intel-gfx, daniel; +Cc: Jani Nikula

Since the base fields in both struct intel_connector and struct
intel_sdvo_connector are at the beginning of the enclosing struct, the
pointers are essentially the same, but there is no requirement or guarantee
that this is always the case. Kfree the enclosing intel_sdvo_connector
pointer that was originally allocated, not the enclosed drm_connector, in
case someone ever rearranges the structs.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 6456427..7dacd6f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1766,7 +1766,7 @@ static void intel_sdvo_destroy(struct drm_connector *connector)
 	intel_sdvo_destroy_enhance_property(connector);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
-	kfree(connector);
+	kfree(intel_sdvo_connector);
 }
 
 static bool intel_sdvo_detect_hdmi_audio(struct drm_connector *connector)
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy
  2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
@ 2012-11-12 16:48   ` Ville Syrjälä
  2012-11-22 13:10   ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2012-11-12 16:48 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Mon, Nov 12, 2012 at 06:31:36PM +0200, Jani Nikula wrote:
> Since the base fields in both struct intel_connector and struct
> intel_sdvo_connector are at the beginning of the enclosing struct, the
> pointers are essentially the same, but there is no requirement or guarantee
> that this is always the case. Kfree the enclosing intel_sdvo_connector
> pointer that was originally allocated, not the enclosed drm_connector, in
> case someone ever rearranges the structs.

BTW in several places the reworked (maybe also the old?) modeset code
depends on the base drm struct being in the beginning of the intel
specific struct.

Here's one of the culprits from intel_modeset_commit_output_state():

 connector->base.encoder = &connector->new_encoder->base;

If connector->new_encoder is NULL this will generate a bogus address
unless new_encoder.base is at offset 0.

We should probably have some compile time asserts to prevent anyone from
making a mess by accident.

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures
  2012-11-12 16:31 [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Jani Nikula
  2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
@ 2012-11-12 16:52 ` Chris Wilson
  2012-11-12 19:09 ` Daniel Vetter
  2 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2012-11-12 16:52 UTC (permalink / raw)
  To: intel-gfx, daniel; +Cc: Jani Nikula

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Mon, 12 Nov 2012 18:31:35 +0200, Jani Nikula <jani.nikula@intel.com> wrote:
> Any failures in intel_sdvo_init() after the intel_sdvo_setup_output() call
> left behind ghost connectors, attached (with a dangling pointer) to the
> sdvo that has been cleaned up and freed. Properly destroy any connectors
> attached to the encoder.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=46381
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> This is *completely* untested, and I don't really know if the connector
> loop is okay or naïve in some way... Please review and test carefully. CC
> Chris in case this addresses his SDVO EDID failures too.

Sadly not. The issue in question, for readers at home, is that after

commit 9cd300e038d492af4990b04e127e0bd2df64b1ca
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Fri Oct 19 14:51:52 2012 +0300

    drm/i915: Move cached EDID to intel_connector
    
    Move the cached EDID from intel_dp and intel_lvds_connector to
    intel_connector. Unify cached EDID handling for LVDS and eDP, in
    preparation for adding more generic EDID caching later.

I lose the EDID on a SDVO-LVDS device (an ilk dual LVDS and SDVO-LVDS
Libretto W105).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
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/sdvo: clean up connectors on intel_sdvo_init() failures
  2012-11-12 16:31 [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Jani Nikula
  2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
  2012-11-12 16:52 ` [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Chris Wilson
@ 2012-11-12 19:09 ` Daniel Vetter
  2 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2012-11-12 19:09 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Mon, Nov 12, 2012 at 06:31:35PM +0200, Jani Nikula wrote:
> Any failures in intel_sdvo_init() after the intel_sdvo_setup_output() call
> left behind ghost connectors, attached (with a dangling pointer) to the
> sdvo that has been cleaned up and freed. Properly destroy any connectors
> attached to the encoder.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=46381
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reporter says this works, tested-by&cc: stable added and merged.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy
  2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
  2012-11-12 16:48   ` Ville Syrjälä
@ 2012-11-22 13:10   ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2012-11-22 13:10 UTC (permalink / raw)
  To: Jani Nikula; +Cc: intel-gfx

On Mon, Nov 12, 2012 at 06:31:36PM +0200, Jani Nikula wrote:
> Since the base fields in both struct intel_connector and struct
> intel_sdvo_connector are at the beginning of the enclosing struct, the
> pointers are essentially the same, but there is no requirement or guarantee
> that this is always the case. Kfree the enclosing intel_sdvo_connector
> pointer that was originally allocated, not the enclosed drm_connector, in
> case someone ever rearranges the structs.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-11-22 13:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-12 16:31 [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Jani Nikula
2012-11-12 16:31 ` [PATCH 2/2] drm/i915/sdvo: kfree the intel_sdvo_connector, not drm_connector, on destroy Jani Nikula
2012-11-12 16:48   ` Ville Syrjälä
2012-11-22 13:10   ` Daniel Vetter
2012-11-12 16:52 ` [PATCH 1/2] drm/i915/sdvo: clean up connectors on intel_sdvo_init() failures Chris Wilson
2012-11-12 19:09 ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).