drm/i915: propagate errors from intel_dp_init_connector

From: Paulo Zanoni <[email protected]>

In case we detect a "ghost eDP", intel_dp_init_connector was freeing both the connector and encoder and then returning. On Haswell, intel_ddi_init was trying to use the freed encoder on the HDMI initialization path since the following commit:

commit 21a8e6a4853b2ed39fa4c5188a710f2cf1b92026

Author: Daniel Vetter <[email protected]>
Date:   Wed Apr 10 23:28:35 2013 +0200

drm/i915: don't setup hdmi for port D edp in ddi_init

So on this patch we change intel_dp_init_connector from void to bool, check its return value on its callers and leave the resource-freeing task to its callers (since the callers are the ones who allocate the resources). With this change it will be easier to see that intel_dp_init_connector can fail, so I hope we won't have similar regressions in the future.

Also include a small change to remove extra "{" and "}" on nearby code.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c | 10 +++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 15 +++++++++------
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 3 files changed, 18 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +7 -3

index 224ce25..4e0cf37 100644
@@ -1356,14 +1356,18 @@ void intel_ddi_init(struct drm_device *dev, enum port port)
 	intel_encoder->cloneable = false;
 	intel_encoder->hot_plug = intel_ddi_hot_plug;
 
-	intel_dp_init_connector(intel_dig_port, dp_connector);
+	/* May fail if it's an eDP connector that's disconnected. */
+	if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
+		intel_ddi_destroy(encoder);
+		intel_dp_destroy(&dp_connector->base);
+		return;
+	}
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP) {
 		hdmi_connector = kzalloc(sizeof(struct intel_connector),
 					 GFP_KERNEL);
-		if (!hdmi_connector) {
+		if (!hdmi_connector)
 			return;
-		}
 
 		intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
 		intel_hdmi_init_connector(intel_dig_port, hdmi_connector);

drivers/gpu/drm/i915/intel_dp.c +9 -6

index 0de82bb..ec23b3d 100644
@@ -2687,7 +2687,7 @@ done:
 	return 0;
 }
 
-static void
+void
 intel_dp_destroy(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);

@@ -2961,7 +2961,7 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
-void
+bool
 intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector)
 {

@@ -3095,9 +3095,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		} else {
 			/* if this fails, presume the device is a ghost */
 			DRM_INFO("failed to retrieve link info, disabling eDP\n");
-			intel_dp_encoder_destroy(&intel_encoder->base);
-			intel_dp_destroy(connector);
-			return;
+			return false;
 		}
 
 		/* We now know it's not a ghost, init power sequence regs. */

@@ -3152,6 +3150,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
+
+	return true;
 }
 
 void

@@ -3197,5 +3197,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
 	intel_encoder->cloneable = false;
 	intel_encoder->hot_plug = intel_dp_hot_plug;
 
-	intel_dp_init_connector(intel_dig_port, intel_connector);
+	if (!intel_dp_init_connector(intel_dig_port, intel_connector)) {
+		intel_dp_encoder_destroy(encoder);
+		intel_dp_destroy(&intel_connector->base);
+	}
 }

drivers/gpu/drm/i915/intel_drv.h +2 -1

index b9e250c..8a7ad99 100644
@@ -569,9 +569,10 @@ extern bool intel_lvds_init(struct drm_device *dev);
 extern bool intel_is_dual_link_lvds(struct drm_device *dev);
 extern void intel_dp_init(struct drm_device *dev, int output_reg,
 			  enum port port);
-extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
+extern bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 				    struct intel_connector *intel_connector);
 extern void intel_dp_init_link_config(struct intel_dp *intel_dp);
+extern void intel_dp_destroy(struct drm_connector *connector);
 extern void intel_dp_start_link_train(struct intel_dp *intel_dp);
 extern void intel_dp_complete_link_train(struct intel_dp *intel_dp);
 extern void intel_dp_stop_link_train(struct intel_dp *intel_dp);

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

drivers/gpu/drm/i915/intel_ddi.c void intel_ddi_init(struct drm_device *dev, enum port port) · line 1364

Quoted 7 lines · Paulo Zanoni

drivers/gpu/drm/i915/intel_ddi.c +6 -1

-     intel_dp_init_connector(intel_dig_port, dp_connector);
+     /* May fail if it's an eDP connector that's disconnected. */
+     if (!intel_dp_init_connector(intel_dig_port, dp_connector)) {
+             intel_ddi_destroy(encoder);
+             intel_dp_destroy(&dp_connector->base);
+             return;
+     }

You should not need to export intel_dp_destroy() ever. Better would be for init to undo anything it allocates upon failure and here we just kfree(dp_connector). In particular, intel_dp_destroy() is wrong here as you have no idea if init returned at an appropriate juntion to make calling that function safe.