[PATCH 0/3] drm: drm_sysfs_connector_add() stuff

[PATCH 0/3] drm: drm_sysfs_connector_add() stuff

[ville.syrjala]

Most drivers don't appear to bother with checking drm_connector_sysfs_add()
return value. That means the recent changes to drm_connector_sysfs_add()
have left most drivers susceptible to oopsing on cleanup if
drm_connector_sysfs_add() somehow failed.

This series makes drm_connector_sysfs_remove() safe to call even if
drm_connector_sysfs_add() failed. I also went a bit further and changed
i915 to check the drm_connector_sysfs_add() return value. Not sure if we
really want that patch now that the danger of oopsing is eliminated...

Ville Syrjälä (3):
      drm/sysfs: Remove stale comments about calling drm_sysfs_connector_add() multiple times
      drm/sysfs: Don't pollute connector->kdev if drm_connector_sysfs_add() fails
      drm/i915: Clean up if drm_sysfs_connector_add() fails

 drivers/gpu/drm/drm_sysfs.c       | 49 ++++++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_crt.c  | 10 +++++++-
 drivers/gpu/drm/i915/intel_ddi.c  |  5 +++-
 drivers/gpu/drm/i915/intel_dp.c   |  6 ++++-
 drivers/gpu/drm/i915/intel_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_dsi.c  |  9 ++++++-
 drivers/gpu/drm/i915/intel_dvo.c  |  8 ++++++-
 drivers/gpu/drm/i915/intel_hdmi.c | 16 ++++++++++---
 drivers/gpu/drm/i915/intel_lvds.c |  7 +++++-
 drivers/gpu/drm/i915/intel_sdvo.c | 36 ++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_tv.c   | 10 +++++++-
 11 files changed, 116 insertions(+), 42 deletions(-)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/sysfs: Remove stale comments about calling drm_sysfs_connector_add() multiple times

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_connector_sysfs_add() explicitly checks if connector->kdev
is already populated and returns success. So it clearly now allows
being called multiple times. Remove some stale comments to the contrary.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index db1c8f9..1a35ea5 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -366,11 +366,6 @@ static struct bin_attribute edid_attr = {
  * properties (so far, connection status, dpms, mode list & edid) and
  * generate a hotplug event so userspace knows there's a new connector
  * available.
- *
- * Note:
- * This routine should only be called *once* for each registered connector.
- * A second call for an already registered connector will trigger the BUG_ON
- * below.
  */
 int drm_sysfs_connector_add(struct drm_connector *connector)
 {
@@ -383,7 +378,6 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 	if (connector->kdev)
 		return 0;
 
-	/* We shouldn't get called more than once for the same connector */
 	connector->kdev = device_create(drm_class, dev->primary->kdev,
 					0, connector, "card%d-%s",
 					dev->primary->index, drm_get_connector_name(connector));
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/sysfs: Don't pollute connector->kdev if drm_connector_sysfs_add() fails

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently if drm_connector_sysfs_add() fails, it can leave connector->kdev
populated with an ERR_PTR value, or pointing to an already freed device.
Use a temporarary kdev pointer during drm_connector_sysfs_add(), and
only set connector->kdev if the function succeeds. This avoids oopsing
if drm_connector_sysfs_remove() gets called for a connector where
drm_connector_sysfs_add() previously failed.

Give drm_sysfs_device_add() the same treatment for the sake of
consistency.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 1a35ea5..a82dc8b 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -370,6 +370,7 @@ static struct bin_attribute edid_attr = {
 int drm_sysfs_connector_add(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
+	struct device *kdev;
 	int attr_cnt = 0;
 	int opt_cnt = 0;
 	int i;
@@ -378,22 +379,22 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 	if (connector->kdev)
 		return 0;
 
-	connector->kdev = device_create(drm_class, dev->primary->kdev,
-					0, connector, "card%d-%s",
-					dev->primary->index, drm_get_connector_name(connector));
+	kdev = device_create(drm_class, dev->primary->kdev,
+			     0, connector, "card%d-%s",
+			     dev->primary->index, drm_get_connector_name(connector));
 	DRM_DEBUG("adding \"%s\" to sysfs\n",
 		  drm_get_connector_name(connector));
 
-	if (IS_ERR(connector->kdev)) {
-		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));
-		ret = PTR_ERR(connector->kdev);
+	if (IS_ERR(kdev)) {
+		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(kdev));
+		ret = PTR_ERR(kdev);
 		goto out;
 	}
 
 	/* Standard attributes */
 
 	for (attr_cnt = 0; attr_cnt < ARRAY_SIZE(connector_attrs); attr_cnt++) {
-		ret = device_create_file(connector->kdev, &connector_attrs[attr_cnt]);
+		ret = device_create_file(kdev, &connector_attrs[attr_cnt]);
 		if (ret)
 			goto err_out_files;
 	}
@@ -410,7 +411,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 		case DRM_MODE_CONNECTOR_Component:
 		case DRM_MODE_CONNECTOR_TV:
 			for (opt_cnt = 0; opt_cnt < ARRAY_SIZE(connector_attrs_opt1); opt_cnt++) {
-				ret = device_create_file(connector->kdev, &connector_attrs_opt1[opt_cnt]);
+				ret = device_create_file(kdev, &connector_attrs_opt1[opt_cnt]);
 				if (ret)
 					goto err_out_files;
 			}
@@ -419,21 +420,23 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 			break;
 	}
 
-	ret = sysfs_create_bin_file(&connector->kdev->kobj, &edid_attr);
+	ret = sysfs_create_bin_file(&kdev->kobj, &edid_attr);
 	if (ret)
 		goto err_out_files;
 
 	/* Let userspace know we have a new connector */
 	drm_sysfs_hotplug_event(dev);
 
+	connector->kdev = kdev;
+
 	return 0;
 
 err_out_files:
 	for (i = 0; i < opt_cnt; i++)
-		device_remove_file(connector->kdev, &connector_attrs_opt1[i]);
+		device_remove_file(kdev, &connector_attrs_opt1[i]);
 	for (i = 0; i < attr_cnt; i++)
-		device_remove_file(connector->kdev, &connector_attrs[i]);
-	device_unregister(connector->kdev);
+		device_remove_file(kdev, &connector_attrs[i]);
+	device_unregister(kdev);
 
 out:
 	return ret;
@@ -501,6 +504,7 @@ EXPORT_SYMBOL(drm_sysfs_hotplug_event);
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
 	char *minor_str;
+	struct device *kdev;
 
 	if (minor->type == DRM_MINOR_CONTROL)
 		minor_str = "controlD%d";
@@ -509,13 +513,16 @@ int drm_sysfs_device_add(struct drm_minor *minor)
         else
                 minor_str = "card%d";
 
-	minor->kdev = device_create(drm_class, minor->dev->dev,
-				    MKDEV(DRM_MAJOR, minor->index),
-				    minor, minor_str, minor->index);
-	if (IS_ERR(minor->kdev)) {
-		DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
-		return PTR_ERR(minor->kdev);
+	kdev = device_create(drm_class, minor->dev->dev,
+			     MKDEV(DRM_MAJOR, minor->index),
+			     minor, minor_str, minor->index);
+	if (IS_ERR(kdev)) {
+		DRM_ERROR("device create failed %ld\n", PTR_ERR(kdev));
+		return PTR_ERR(kdev);
 	}
+
+	minor->kdev = kdev;
+
 	return 0;
 }
 
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/i915: Clean up if drm_sysfs_connector_add() fails

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

While we can now call drm_sysfs_connector_remove() even if
drm_connector_sysfs_add() failed, it would seem better for
the user to know that something went wrong. So instead of
ignoring drm_sysfs_connector_add() return value, checl it
and fail the whole connector registration.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c  | 10 +++++++++-
 drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
 drivers/gpu/drm/i915/intel_dp.c   |  6 +++++-
 drivers/gpu/drm/i915/intel_drv.h  |  2 +-
 drivers/gpu/drm/i915/intel_dsi.c  |  9 ++++++++-
 drivers/gpu/drm/i915/intel_dvo.c  |  8 +++++++-
 drivers/gpu/drm/i915/intel_hdmi.c | 16 +++++++++++++---
 drivers/gpu/drm/i915/intel_lvds.c |  7 ++++++-
 drivers/gpu/drm/i915/intel_sdvo.c | 36 +++++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_tv.c   | 10 +++++++++-
 10 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 2e01bd3..be8a024 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -773,6 +773,7 @@ void intel_crt_init(struct drm_device *dev)
 	struct intel_crt *crt;
 	struct intel_connector *intel_connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int error;
 
 	/* Skip machines without VGA that falsely report hotplug events */
 	if (dmi_check_system(intel_no_crt))
@@ -836,7 +837,14 @@ void intel_crt_init(struct drm_device *dev)
 
 	drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
 
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		drm_encoder_cleanup(&crt->base.base);
+		drm_connector_cleanup(connector);
+		kfree(intel_connector);
+		kfree(crt);
+		return;
+	}
 
 	if (!I915_HAS_HOTPLUG(dev))
 		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 31f4fe2..687e333 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1375,7 +1375,10 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
 		return NULL;
 
 	intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
-	intel_hdmi_init_connector(intel_dig_port, connector);
+	if (!intel_hdmi_init_connector(intel_dig_port, connector)) {
+		kfree(connector);
+		return NULL;
+	}
 
 	return connector;
 }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c8515bb..eb01be7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3603,7 +3603,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			  ironlake_panel_vdd_work);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		drm_connector_cleanup(connector);
+		return false;
+	}
 
 	if (HAS_DDI(dev))
 		intel_connector->get_hw_state = intel_ddi_connector_get_hw_state;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9d2624f..a944d6e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -760,7 +760,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
 
 /* intel_hdmi.c */
 void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
-void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
+bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 			       struct intel_connector *intel_connector);
 struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index d257b09..16684b5 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -535,6 +535,7 @@ bool intel_dsi_init(struct drm_device *dev)
 	struct drm_display_mode *fixed_mode = NULL;
 	const struct intel_dsi_device *dsi;
 	unsigned int i;
+	int error;
 
 	DRM_DEBUG_KMS("\n");
 
@@ -598,11 +599,17 @@ bool intel_dsi_init(struct drm_device *dev)
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		drm_connector_cleanup(connector);
+		goto err;
+	}
 
 	fixed_mode = dsi->dev_ops->get_modes(&intel_dsi->dev);
 	if (!fixed_mode) {
 		DRM_DEBUG_KMS("no fixed mode\n");
+		drm_sysfs_connector_remove(connector);
+		drm_connector_cleanup(connector);
 		goto err;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 3c77365..439e9d9 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -484,6 +484,7 @@ void intel_dvo_init(struct drm_device *dev)
 		struct i2c_adapter *i2c;
 		int gpio;
 		bool dvoinit;
+		int error;
 
 		/* Allow the I2C driver info to specify the GPIO to be used in
 		 * special cases, but otherwise default to what's defined
@@ -555,7 +556,12 @@ void intel_dvo_init(struct drm_device *dev)
 			intel_dvo->panel_wants_dither = true;
 		}
 
-		drm_sysfs_connector_add(connector);
+		error = drm_sysfs_connector_add(connector);
+		if (error) {
+			drm_connector_cleanup(connector);
+			break;
+		}
+
 		return;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 51a8336..f8ee5ca 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1211,7 +1211,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 	intel_hdmi->color_range_auto = true;
 }
 
-void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
+bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 			       struct intel_connector *intel_connector)
 {
 	struct drm_connector *connector = &intel_connector->base;
@@ -1220,6 +1220,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
+	int error;
 
 	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
 			   DRM_MODE_CONNECTOR_HDMIA);
@@ -1274,7 +1275,11 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	intel_hdmi_add_properties(intel_hdmi, connector);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		drm_connector_cleanup(connector);
+		return false;
+	}
 
 	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
 	 * 0xd.  Failure to do so will result in spurious interrupts being
@@ -1284,6 +1289,8 @@ void intel_hdmi_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 intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
@@ -1329,5 +1336,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
 	intel_dig_port->dp.output_reg = 0;
 
-	intel_hdmi_init_connector(intel_dig_port, intel_connector);
+	if (!intel_hdmi_init_connector(intel_dig_port, intel_connector)) {
+		drm_encoder_cleanup(&intel_encoder->base);
+		return;
+	}
 }
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index b0ef558..a358a5e 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -941,6 +941,7 @@ void intel_lvds_init(struct drm_device *dev)
 	u32 lvds;
 	int pipe;
 	u8 pin;
+	int error;
 
 	if (!intel_lvds_supported(dev))
 		return;
@@ -1137,7 +1138,11 @@ out:
 		DRM_DEBUG_KMS("lid notifier registration failed\n");
 		lvds_connector->lid_notifier.notifier_call = NULL;
 	}
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
+		goto failed;
+	}
 
 	intel_panel_init(&intel_connector->panel, fixed_mode);
 	intel_panel_setup_backlight(connector);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index a583e8f..23d758e 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2361,10 +2361,12 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
 		return 0x72;
 }
 
-static void
+static bool
 intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 			  struct intel_sdvo *encoder)
 {
+	int error;
+
 	drm_connector_init(encoder->base.base.dev,
 			   &connector->base.base,
 			   &intel_sdvo_connector_funcs,
@@ -2379,7 +2381,13 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
 	connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
 
 	intel_connector_attach_encoder(&connector->base, &encoder->base);
-	drm_sysfs_connector_add(&connector->base.base);
+	error = drm_sysfs_connector_add(&connector->base.base);
+	if (error) {
+		drm_connector_cleanup(&connector->base.base);
+		return false;
+	}
+
+	return true;
 }
 
 static void
@@ -2439,7 +2447,11 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo->is_hdmi = true;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	if (intel_sdvo->is_hdmi)
 		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
 
@@ -2470,7 +2482,10 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
 
 	intel_sdvo->is_tv = true;
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
 
 	if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
 		goto err;
@@ -2514,8 +2529,11 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector,
-				  intel_sdvo);
+	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	return true;
 }
 
@@ -2546,7 +2564,11 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
 		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
 	}
 
-	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
+	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
+		kfree(intel_sdvo_connector);
+		return false;
+	}
+
 	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
 		goto err;
 
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 18c4062..c1e66f6 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1554,6 +1554,7 @@ intel_tv_init(struct drm_device *dev)
 	u32 tv_dac_on, tv_dac_off, save_tv_dac;
 	char *tv_format_names[ARRAY_SIZE(tv_modes)];
 	int i, initial_mode = 0;
+	int error;
 
 	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
 		return;
@@ -1668,5 +1669,12 @@ intel_tv_init(struct drm_device *dev)
 	drm_object_attach_property(&connector->base,
 				   dev->mode_config.tv_bottom_margin_property,
 				   intel_tv->margin[TV_MARGIN_BOTTOM]);
-	drm_sysfs_connector_add(connector);
+	error = drm_sysfs_connector_add(connector);
+	if (error) {
+		drm_connector_cleanup(connector);
+		drm_encoder_cleanup(&intel_encoder->base);
+		kfree(intel_connector);
+		kfree(intel_tv);
+		return;
+	}
 }
-- 
1.8.1.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/sysfs: Remove stale comments about calling drm_sysfs_connector_add() multiple times

[Jani Nikula]

On Mon, 04 Nov 2013, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> drm_connector_sysfs_add() explicitly checks if connector->kdev
> is already populated and returns success. So it clearly now allows
> being called multiple times. Remove some stale comments to the contrary.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_sysfs.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index db1c8f9..1a35ea5 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -366,11 +366,6 @@ static struct bin_attribute edid_attr = {
>   * properties (so far, connection status, dpms, mode list & edid) and
>   * generate a hotplug event so userspace knows there's a new connector
>   * available.
> - *
> - * Note:
> - * This routine should only be called *once* for each registered connector.
> - * A second call for an already registered connector will trigger the BUG_ON
> - * below.
>   */
>  int drm_sysfs_connector_add(struct drm_connector *connector)
>  {
> @@ -383,7 +378,6 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  	if (connector->kdev)
>  		return 0;
>  
> -	/* We shouldn't get called more than once for the same connector */
>  	connector->kdev = device_create(drm_class, dev->primary->kdev,
>  					0, connector, "card%d-%s",
>  					dev->primary->index, drm_get_connector_name(connector));
> -- 
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/sysfs: Don't pollute connector->kdev if drm_connector_sysfs_add() fails

[Jani Nikula]

On Mon, 04 Nov 2013, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently if drm_connector_sysfs_add() fails, it can leave connector->kdev
> populated with an ERR_PTR value, or pointing to an already freed device.
> Use a temporarary kdev pointer during drm_connector_sysfs_add(), and
> only set connector->kdev if the function succeeds. This avoids oopsing
> if drm_connector_sysfs_remove() gets called for a connector where
> drm_connector_sysfs_add() previously failed.

s/connector_sysfs/sysfs_connector/g

> Give drm_sysfs_device_add() the same treatment for the sake of
> consistency.

Maybe that one should have the if (minor->kdev) return 0; part too if
consistency is what you're after.

The above addressed and you've got
Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 1a35ea5..a82dc8b 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -370,6 +370,7 @@ static struct bin_attribute edid_attr = {
>  int drm_sysfs_connector_add(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
> +	struct device *kdev;
>  	int attr_cnt = 0;
>  	int opt_cnt = 0;
>  	int i;
> @@ -378,22 +379,22 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  	if (connector->kdev)
>  		return 0;
>  
> -	connector->kdev = device_create(drm_class, dev->primary->kdev,
> -					0, connector, "card%d-%s",
> -					dev->primary->index, drm_get_connector_name(connector));
> +	kdev = device_create(drm_class, dev->primary->kdev,
> +			     0, connector, "card%d-%s",
> +			     dev->primary->index, drm_get_connector_name(connector));
>  	DRM_DEBUG("adding \"%s\" to sysfs\n",
>  		  drm_get_connector_name(connector));
>  
> -	if (IS_ERR(connector->kdev)) {
> -		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));
> -		ret = PTR_ERR(connector->kdev);
> +	if (IS_ERR(kdev)) {
> +		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(kdev));
> +		ret = PTR_ERR(kdev);
>  		goto out;
>  	}
>  
>  	/* Standard attributes */
>  
>  	for (attr_cnt = 0; attr_cnt < ARRAY_SIZE(connector_attrs); attr_cnt++) {
> -		ret = device_create_file(connector->kdev, &connector_attrs[attr_cnt]);
> +		ret = device_create_file(kdev, &connector_attrs[attr_cnt]);
>  		if (ret)
>  			goto err_out_files;
>  	}
> @@ -410,7 +411,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  		case DRM_MODE_CONNECTOR_Component:
>  		case DRM_MODE_CONNECTOR_TV:
>  			for (opt_cnt = 0; opt_cnt < ARRAY_SIZE(connector_attrs_opt1); opt_cnt++) {
> -				ret = device_create_file(connector->kdev, &connector_attrs_opt1[opt_cnt]);
> +				ret = device_create_file(kdev, &connector_attrs_opt1[opt_cnt]);
>  				if (ret)
>  					goto err_out_files;
>  			}
> @@ -419,21 +420,23 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
>  			break;
>  	}
>  
> -	ret = sysfs_create_bin_file(&connector->kdev->kobj, &edid_attr);
> +	ret = sysfs_create_bin_file(&kdev->kobj, &edid_attr);
>  	if (ret)
>  		goto err_out_files;
>  
>  	/* Let userspace know we have a new connector */
>  	drm_sysfs_hotplug_event(dev);
>  
> +	connector->kdev = kdev;
> +
>  	return 0;
>  
>  err_out_files:
>  	for (i = 0; i < opt_cnt; i++)
> -		device_remove_file(connector->kdev, &connector_attrs_opt1[i]);
> +		device_remove_file(kdev, &connector_attrs_opt1[i]);
>  	for (i = 0; i < attr_cnt; i++)
> -		device_remove_file(connector->kdev, &connector_attrs[i]);
> -	device_unregister(connector->kdev);
> +		device_remove_file(kdev, &connector_attrs[i]);
> +	device_unregister(kdev);
>  
>  out:
>  	return ret;
> @@ -501,6 +504,7 @@ EXPORT_SYMBOL(drm_sysfs_hotplug_event);
>  int drm_sysfs_device_add(struct drm_minor *minor)
>  {
>  	char *minor_str;
> +	struct device *kdev;
>  
>  	if (minor->type == DRM_MINOR_CONTROL)
>  		minor_str = "controlD%d";
> @@ -509,13 +513,16 @@ int drm_sysfs_device_add(struct drm_minor *minor)
>          else
>                  minor_str = "card%d";
>  
> -	minor->kdev = device_create(drm_class, minor->dev->dev,
> -				    MKDEV(DRM_MAJOR, minor->index),
> -				    minor, minor_str, minor->index);
> -	if (IS_ERR(minor->kdev)) {
> -		DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
> -		return PTR_ERR(minor->kdev);
> +	kdev = device_create(drm_class, minor->dev->dev,
> +			     MKDEV(DRM_MAJOR, minor->index),
> +			     minor, minor_str, minor->index);
> +	if (IS_ERR(kdev)) {
> +		DRM_ERROR("device create failed %ld\n", PTR_ERR(kdev));
> +		return PTR_ERR(kdev);
>  	}
> +
> +	minor->kdev = kdev;
> +
>  	return 0;
>  }
>  
> -- 
> 1.8.1.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Clean up if drm_sysfs_connector_add() fails

[Jani Nikula]

On Mon, 04 Nov 2013, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> While we can now call drm_sysfs_connector_remove() even if
> drm_connector_sysfs_add() failed, it would seem better for
> the user to know that something went wrong. So instead of
> ignoring drm_sysfs_connector_add() return value, checl it
> and fail the whole connector registration.

I think I'd rather see a cleanup of the init time fail paths first,
using the regular kernel goto style. Which I also don't think should be
a huge priority. Whaddaya think?

Cheers,
Jani.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_crt.c  | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
>  drivers/gpu/drm/i915/intel_dp.c   |  6 +++++-
>  drivers/gpu/drm/i915/intel_drv.h  |  2 +-
>  drivers/gpu/drm/i915/intel_dsi.c  |  9 ++++++++-
>  drivers/gpu/drm/i915/intel_dvo.c  |  8 +++++++-
>  drivers/gpu/drm/i915/intel_hdmi.c | 16 +++++++++++++---
>  drivers/gpu/drm/i915/intel_lvds.c |  7 ++++++-
>  drivers/gpu/drm/i915/intel_sdvo.c | 36 +++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_tv.c   | 10 +++++++++-
>  10 files changed, 91 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 2e01bd3..be8a024 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -773,6 +773,7 @@ void intel_crt_init(struct drm_device *dev)
>  	struct intel_crt *crt;
>  	struct intel_connector *intel_connector;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int error;
>  
>  	/* Skip machines without VGA that falsely report hotplug events */
>  	if (dmi_check_system(intel_no_crt))
> @@ -836,7 +837,14 @@ void intel_crt_init(struct drm_device *dev)
>  
>  	drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
>  
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		drm_encoder_cleanup(&crt->base.base);
> +		drm_connector_cleanup(connector);
> +		kfree(intel_connector);
> +		kfree(crt);
> +		return;
> +	}
>  
>  	if (!I915_HAS_HOTPLUG(dev))
>  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 31f4fe2..687e333 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1375,7 +1375,10 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
>  		return NULL;
>  
>  	intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
> -	intel_hdmi_init_connector(intel_dig_port, connector);
> +	if (!intel_hdmi_init_connector(intel_dig_port, connector)) {
> +		kfree(connector);
> +		return NULL;
> +	}
>  
>  	return connector;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c8515bb..eb01be7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3603,7 +3603,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  			  ironlake_panel_vdd_work);
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		drm_connector_cleanup(connector);
> +		return false;
> +	}
>  
>  	if (HAS_DDI(dev))
>  		intel_connector->get_hw_state = intel_ddi_connector_get_hw_state;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9d2624f..a944d6e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -760,7 +760,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
>  
>  /* intel_hdmi.c */
>  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  			       struct intel_connector *intel_connector);
>  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index d257b09..16684b5 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -535,6 +535,7 @@ bool intel_dsi_init(struct drm_device *dev)
>  	struct drm_display_mode *fixed_mode = NULL;
>  	const struct intel_dsi_device *dsi;
>  	unsigned int i;
> +	int error;
>  
>  	DRM_DEBUG_KMS("\n");
>  
> @@ -598,11 +599,17 @@ bool intel_dsi_init(struct drm_device *dev)
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
>  
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		drm_connector_cleanup(connector);
> +		goto err;
> +	}
>  
>  	fixed_mode = dsi->dev_ops->get_modes(&intel_dsi->dev);
>  	if (!fixed_mode) {
>  		DRM_DEBUG_KMS("no fixed mode\n");
> +		drm_sysfs_connector_remove(connector);
> +		drm_connector_cleanup(connector);
>  		goto err;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 3c77365..439e9d9 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -484,6 +484,7 @@ void intel_dvo_init(struct drm_device *dev)
>  		struct i2c_adapter *i2c;
>  		int gpio;
>  		bool dvoinit;
> +		int error;
>  
>  		/* Allow the I2C driver info to specify the GPIO to be used in
>  		 * special cases, but otherwise default to what's defined
> @@ -555,7 +556,12 @@ void intel_dvo_init(struct drm_device *dev)
>  			intel_dvo->panel_wants_dither = true;
>  		}
>  
> -		drm_sysfs_connector_add(connector);
> +		error = drm_sysfs_connector_add(connector);
> +		if (error) {
> +			drm_connector_cleanup(connector);
> +			break;
> +		}
> +
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 51a8336..f8ee5ca 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1211,7 +1211,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>  	intel_hdmi->color_range_auto = true;
>  }
>  
> -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  			       struct intel_connector *intel_connector)
>  {
>  	struct drm_connector *connector = &intel_connector->base;
> @@ -1220,6 +1220,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	int error;
>  
>  	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_HDMIA);
> @@ -1274,7 +1275,11 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	intel_hdmi_add_properties(intel_hdmi, connector);
>  
>  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		drm_connector_cleanup(connector);
> +		return false;
> +	}
>  
>  	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
>  	 * 0xd.  Failure to do so will result in spurious interrupts being
> @@ -1284,6 +1289,8 @@ void intel_hdmi_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 intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> @@ -1329,5 +1336,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
>  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
>  	intel_dig_port->dp.output_reg = 0;
>  
> -	intel_hdmi_init_connector(intel_dig_port, intel_connector);
> +	if (!intel_hdmi_init_connector(intel_dig_port, intel_connector)) {
> +		drm_encoder_cleanup(&intel_encoder->base);
> +		return;
> +	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index b0ef558..a358a5e 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -941,6 +941,7 @@ void intel_lvds_init(struct drm_device *dev)
>  	u32 lvds;
>  	int pipe;
>  	u8 pin;
> +	int error;
>  
>  	if (!intel_lvds_supported(dev))
>  		return;
> @@ -1137,7 +1138,11 @@ out:
>  		DRM_DEBUG_KMS("lid notifier registration failed\n");
>  		lvds_connector->lid_notifier.notifier_call = NULL;
>  	}
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
> +		goto failed;
> +	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode);
>  	intel_panel_setup_backlight(connector);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index a583e8f..23d758e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2361,10 +2361,12 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
>  		return 0x72;
>  }
>  
> -static void
> +static bool
>  intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
>  			  struct intel_sdvo *encoder)
>  {
> +	int error;
> +
>  	drm_connector_init(encoder->base.base.dev,
>  			   &connector->base.base,
>  			   &intel_sdvo_connector_funcs,
> @@ -2379,7 +2381,13 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
>  	connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
>  
>  	intel_connector_attach_encoder(&connector->base, &encoder->base);
> -	drm_sysfs_connector_add(&connector->base.base);
> +	error = drm_sysfs_connector_add(&connector->base.base);
> +	if (error) {
> +		drm_connector_cleanup(&connector->base.base);
> +		return false;
> +	}
> +
> +	return true;
>  }
>  
>  static void
> @@ -2439,7 +2447,11 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
>  		intel_sdvo->is_hdmi = true;
>  	}
>  
> -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> +		kfree(intel_sdvo_connector);
> +		return false;
> +	}
> +
>  	if (intel_sdvo->is_hdmi)
>  		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
>  
> @@ -2470,7 +2482,10 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
>  
>  	intel_sdvo->is_tv = true;
>  
> -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> +		kfree(intel_sdvo_connector);
> +		return false;
> +	}
>  
>  	if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
>  		goto err;
> @@ -2514,8 +2529,11 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
>  	}
>  
> -	intel_sdvo_connector_init(intel_sdvo_connector,
> -				  intel_sdvo);
> +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> +		kfree(intel_sdvo_connector);
> +		return false;
> +	}
> +
>  	return true;
>  }
>  
> @@ -2546,7 +2564,11 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
>  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
>  	}
>  
> -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> +		kfree(intel_sdvo_connector);
> +		return false;
> +	}
> +
>  	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
>  		goto err;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 18c4062..c1e66f6 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1554,6 +1554,7 @@ intel_tv_init(struct drm_device *dev)
>  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
>  	char *tv_format_names[ARRAY_SIZE(tv_modes)];
>  	int i, initial_mode = 0;
> +	int error;
>  
>  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
>  		return;
> @@ -1668,5 +1669,12 @@ intel_tv_init(struct drm_device *dev)
>  	drm_object_attach_property(&connector->base,
>  				   dev->mode_config.tv_bottom_margin_property,
>  				   intel_tv->margin[TV_MARGIN_BOTTOM]);
> -	drm_sysfs_connector_add(connector);
> +	error = drm_sysfs_connector_add(connector);
> +	if (error) {
> +		drm_connector_cleanup(connector);
> +		drm_encoder_cleanup(&intel_encoder->base);
> +		kfree(intel_connector);
> +		kfree(intel_tv);
> +		return;
> +	}
>  }
> -- 
> 1.8.1.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Clean up if drm_sysfs_connector_add() fails

[Daniel Vetter]

On Tue, Nov 05, 2013 at 09:23:46AM +0200, Jani Nikula wrote:
> On Mon, 04 Nov 2013, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > While we can now call drm_sysfs_connector_remove() even if
> > drm_connector_sysfs_add() failed, it would seem better for
> > the user to know that something went wrong. So instead of
> > ignoring drm_sysfs_connector_add() return value, checl it
> > and fail the whole connector registration.
> 
> I think I'd rather see a cleanup of the init time fail paths first,
> using the regular kernel goto style. Which I also don't think should be
> a huge priority. Whaddaya think?

In my dream world we could solve most of this with devres.c. Requires us
to fix the drm midlayer first though, and I'd guess that the interaction
with the shadow attach stuff could be interesting ...
-Daniel

> 
> Cheers,
> Jani.
> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c  | 10 +++++++++-
> >  drivers/gpu/drm/i915/intel_ddi.c  |  5 ++++-
> >  drivers/gpu/drm/i915/intel_dp.c   |  6 +++++-
> >  drivers/gpu/drm/i915/intel_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_dsi.c  |  9 ++++++++-
> >  drivers/gpu/drm/i915/intel_dvo.c  |  8 +++++++-
> >  drivers/gpu/drm/i915/intel_hdmi.c | 16 +++++++++++++---
> >  drivers/gpu/drm/i915/intel_lvds.c |  7 ++++++-
> >  drivers/gpu/drm/i915/intel_sdvo.c | 36 +++++++++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_tv.c   | 10 +++++++++-
> >  10 files changed, 91 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 2e01bd3..be8a024 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -773,6 +773,7 @@ void intel_crt_init(struct drm_device *dev)
> >  	struct intel_crt *crt;
> >  	struct intel_connector *intel_connector;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int error;
> >  
> >  	/* Skip machines without VGA that falsely report hotplug events */
> >  	if (dmi_check_system(intel_no_crt))
> > @@ -836,7 +837,14 @@ void intel_crt_init(struct drm_device *dev)
> >  
> >  	drm_connector_helper_add(connector, &intel_crt_connector_helper_funcs);
> >  
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		drm_encoder_cleanup(&crt->base.base);
> > +		drm_connector_cleanup(connector);
> > +		kfree(intel_connector);
> > +		kfree(crt);
> > +		return;
> > +	}
> >  
> >  	if (!I915_HAS_HOTPLUG(dev))
> >  		intel_connector->polled = DRM_CONNECTOR_POLL_CONNECT;
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 31f4fe2..687e333 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1375,7 +1375,10 @@ intel_ddi_init_hdmi_connector(struct intel_digital_port *intel_dig_port)
> >  		return NULL;
> >  
> >  	intel_dig_port->hdmi.hdmi_reg = DDI_BUF_CTL(port);
> > -	intel_hdmi_init_connector(intel_dig_port, connector);
> > +	if (!intel_hdmi_init_connector(intel_dig_port, connector)) {
> > +		kfree(connector);
> > +		return NULL;
> > +	}
> >  
> >  	return connector;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index c8515bb..eb01be7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3603,7 +3603,11 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >  			  ironlake_panel_vdd_work);
> >  
> >  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		drm_connector_cleanup(connector);
> > +		return false;
> > +	}
> >  
> >  	if (HAS_DDI(dev))
> >  		intel_connector->get_hw_state = intel_ddi_connector_get_hw_state;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 9d2624f..a944d6e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -760,7 +760,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev)
> >  
> >  /* intel_hdmi.c */
> >  void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port);
> > -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  			       struct intel_connector *intel_connector);
> >  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> >  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index d257b09..16684b5 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -535,6 +535,7 @@ bool intel_dsi_init(struct drm_device *dev)
> >  	struct drm_display_mode *fixed_mode = NULL;
> >  	const struct intel_dsi_device *dsi;
> >  	unsigned int i;
> > +	int error;
> >  
> >  	DRM_DEBUG_KMS("\n");
> >  
> > @@ -598,11 +599,17 @@ bool intel_dsi_init(struct drm_device *dev)
> >  
> >  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> >  
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		drm_connector_cleanup(connector);
> > +		goto err;
> > +	}
> >  
> >  	fixed_mode = dsi->dev_ops->get_modes(&intel_dsi->dev);
> >  	if (!fixed_mode) {
> >  		DRM_DEBUG_KMS("no fixed mode\n");
> > +		drm_sysfs_connector_remove(connector);
> > +		drm_connector_cleanup(connector);
> >  		goto err;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index 3c77365..439e9d9 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -484,6 +484,7 @@ void intel_dvo_init(struct drm_device *dev)
> >  		struct i2c_adapter *i2c;
> >  		int gpio;
> >  		bool dvoinit;
> > +		int error;
> >  
> >  		/* Allow the I2C driver info to specify the GPIO to be used in
> >  		 * special cases, but otherwise default to what's defined
> > @@ -555,7 +556,12 @@ void intel_dvo_init(struct drm_device *dev)
> >  			intel_dvo->panel_wants_dither = true;
> >  		}
> >  
> > -		drm_sysfs_connector_add(connector);
> > +		error = drm_sysfs_connector_add(connector);
> > +		if (error) {
> > +			drm_connector_cleanup(connector);
> > +			break;
> > +		}
> > +
> >  		return;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 51a8336..f8ee5ca 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1211,7 +1211,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
> >  	intel_hdmi->color_range_auto = true;
> >  }
> >  
> > -void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > +bool intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  			       struct intel_connector *intel_connector)
> >  {
> >  	struct drm_connector *connector = &intel_connector->base;
> > @@ -1220,6 +1220,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  	struct drm_device *dev = intel_encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	enum port port = intel_dig_port->port;
> > +	int error;
> >  
> >  	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
> >  			   DRM_MODE_CONNECTOR_HDMIA);
> > @@ -1274,7 +1275,11 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >  	intel_hdmi_add_properties(intel_hdmi, connector);
> >  
> >  	intel_connector_attach_encoder(intel_connector, intel_encoder);
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		drm_connector_cleanup(connector);
> > +		return false;
> > +	}
> >  
> >  	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
> >  	 * 0xd.  Failure to do so will result in spurious interrupts being
> > @@ -1284,6 +1289,8 @@ void intel_hdmi_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 intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> > @@ -1329,5 +1336,8 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
> >  	intel_dig_port->hdmi.hdmi_reg = hdmi_reg;
> >  	intel_dig_port->dp.output_reg = 0;
> >  
> > -	intel_hdmi_init_connector(intel_dig_port, intel_connector);
> > +	if (!intel_hdmi_init_connector(intel_dig_port, intel_connector)) {
> > +		drm_encoder_cleanup(&intel_encoder->base);
> > +		return;
> > +	}
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index b0ef558..a358a5e 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -941,6 +941,7 @@ void intel_lvds_init(struct drm_device *dev)
> >  	u32 lvds;
> >  	int pipe;
> >  	u8 pin;
> > +	int error;
> >  
> >  	if (!intel_lvds_supported(dev))
> >  		return;
> > @@ -1137,7 +1138,11 @@ out:
> >  		DRM_DEBUG_KMS("lid notifier registration failed\n");
> >  		lvds_connector->lid_notifier.notifier_call = NULL;
> >  	}
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
> > +		goto failed;
> > +	}
> >  
> >  	intel_panel_init(&intel_connector->panel, fixed_mode);
> >  	intel_panel_setup_backlight(connector);
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index a583e8f..23d758e 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2361,10 +2361,12 @@ intel_sdvo_get_slave_addr(struct drm_device *dev, struct intel_sdvo *sdvo)
> >  		return 0x72;
> >  }
> >  
> > -static void
> > +static bool
> >  intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
> >  			  struct intel_sdvo *encoder)
> >  {
> > +	int error;
> > +
> >  	drm_connector_init(encoder->base.base.dev,
> >  			   &connector->base.base,
> >  			   &intel_sdvo_connector_funcs,
> > @@ -2379,7 +2381,13 @@ intel_sdvo_connector_init(struct intel_sdvo_connector *connector,
> >  	connector->base.get_hw_state = intel_sdvo_connector_get_hw_state;
> >  
> >  	intel_connector_attach_encoder(&connector->base, &encoder->base);
> > -	drm_sysfs_connector_add(&connector->base.base);
> > +	error = drm_sysfs_connector_add(&connector->base.base);
> > +	if (error) {
> > +		drm_connector_cleanup(&connector->base.base);
> > +		return false;
> > +	}
> > +
> > +	return true;
> >  }
> >  
> >  static void
> > @@ -2439,7 +2447,11 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> >  		intel_sdvo->is_hdmi = true;
> >  	}
> >  
> > -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > +		kfree(intel_sdvo_connector);
> > +		return false;
> > +	}
> > +
> >  	if (intel_sdvo->is_hdmi)
> >  		intel_sdvo_add_hdmi_properties(intel_sdvo, intel_sdvo_connector);
> >  
> > @@ -2470,7 +2482,10 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> >  
> >  	intel_sdvo->is_tv = true;
> >  
> > -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > +		kfree(intel_sdvo_connector);
> > +		return false;
> > +	}
> >  
> >  	if (!intel_sdvo_tv_create_property(intel_sdvo, intel_sdvo_connector, type))
> >  		goto err;
> > @@ -2514,8 +2529,11 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> >  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
> >  	}
> >  
> > -	intel_sdvo_connector_init(intel_sdvo_connector,
> > -				  intel_sdvo);
> > +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > +		kfree(intel_sdvo_connector);
> > +		return false;
> > +	}
> > +
> >  	return true;
> >  }
> >  
> > @@ -2546,7 +2564,11 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> >  		intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
> >  	}
> >  
> > -	intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> > +	if (!intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo)) {
> > +		kfree(intel_sdvo_connector);
> > +		return false;
> > +	}
> > +
> >  	if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
> >  		goto err;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> > index 18c4062..c1e66f6 100644
> > --- a/drivers/gpu/drm/i915/intel_tv.c
> > +++ b/drivers/gpu/drm/i915/intel_tv.c
> > @@ -1554,6 +1554,7 @@ intel_tv_init(struct drm_device *dev)
> >  	u32 tv_dac_on, tv_dac_off, save_tv_dac;
> >  	char *tv_format_names[ARRAY_SIZE(tv_modes)];
> >  	int i, initial_mode = 0;
> > +	int error;
> >  
> >  	if ((I915_READ(TV_CTL) & TV_FUSE_STATE_MASK) == TV_FUSE_STATE_DISABLED)
> >  		return;
> > @@ -1668,5 +1669,12 @@ intel_tv_init(struct drm_device *dev)
> >  	drm_object_attach_property(&connector->base,
> >  				   dev->mode_config.tv_bottom_margin_property,
> >  				   intel_tv->margin[TV_MARGIN_BOTTOM]);
> > -	drm_sysfs_connector_add(connector);
> > +	error = drm_sysfs_connector_add(connector);
> > +	if (error) {
> > +		drm_connector_cleanup(connector);
> > +		drm_encoder_cleanup(&intel_encoder->base);
> > +		kfree(intel_connector);
> > +		kfree(intel_tv);
> > +		return;
> > +	}
> >  }
> > -- 
> > 1.8.1.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2 2/3] drm/sysfs: Don't pollute connector->kdev if drm_sysfs_connector_add() fails

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently if drm_sysfs_connector_add() fails, it can leave connector->kdev
populated with an ERR_PTR value, or pointing to an already freed device.
Use a temporary kdev pointer during drm_sysfs_connector_add(), and
only set connector->kdev if the function succeeds. This avoids oopsing
if drm_sysfs_connector_add() gets called for a connector where
drm_sysfs_connector_add() previously failed.

Give drm_sysfs_device_add() the same treatment for the sake of
consistency.

v2: s/drm_connector_sysfs_add/drm_sysfs_connector_add
    Make drm_sysfs_device_add() tolerate multiple calls

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 46 +++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 1a35ea5..95c701a 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -370,6 +370,7 @@ static struct bin_attribute edid_attr = {
 int drm_sysfs_connector_add(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
+	struct device *kdev;
 	int attr_cnt = 0;
 	int opt_cnt = 0;
 	int i;
@@ -378,22 +379,22 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 	if (connector->kdev)
 		return 0;
 
-	connector->kdev = device_create(drm_class, dev->primary->kdev,
-					0, connector, "card%d-%s",
-					dev->primary->index, drm_get_connector_name(connector));
+	kdev = device_create(drm_class, dev->primary->kdev,
+			     0, connector, "card%d-%s",
+			     dev->primary->index, drm_get_connector_name(connector));
 	DRM_DEBUG("adding \"%s\" to sysfs\n",
 		  drm_get_connector_name(connector));
 
-	if (IS_ERR(connector->kdev)) {
-		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));
-		ret = PTR_ERR(connector->kdev);
+	if (IS_ERR(kdev)) {
+		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(kdev));
+		ret = PTR_ERR(kdev);
 		goto out;
 	}
 
 	/* Standard attributes */
 
 	for (attr_cnt = 0; attr_cnt < ARRAY_SIZE(connector_attrs); attr_cnt++) {
-		ret = device_create_file(connector->kdev, &connector_attrs[attr_cnt]);
+		ret = device_create_file(kdev, &connector_attrs[attr_cnt]);
 		if (ret)
 			goto err_out_files;
 	}
@@ -410,7 +411,7 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 		case DRM_MODE_CONNECTOR_Component:
 		case DRM_MODE_CONNECTOR_TV:
 			for (opt_cnt = 0; opt_cnt < ARRAY_SIZE(connector_attrs_opt1); opt_cnt++) {
-				ret = device_create_file(connector->kdev, &connector_attrs_opt1[opt_cnt]);
+				ret = device_create_file(kdev, &connector_attrs_opt1[opt_cnt]);
 				if (ret)
 					goto err_out_files;
 			}
@@ -419,21 +420,23 @@ int drm_sysfs_connector_add(struct drm_connector *connector)
 			break;
 	}
 
-	ret = sysfs_create_bin_file(&connector->kdev->kobj, &edid_attr);
+	ret = sysfs_create_bin_file(&kdev->kobj, &edid_attr);
 	if (ret)
 		goto err_out_files;
 
 	/* Let userspace know we have a new connector */
 	drm_sysfs_hotplug_event(dev);
 
+	connector->kdev = kdev;
+
 	return 0;
 
 err_out_files:
 	for (i = 0; i < opt_cnt; i++)
-		device_remove_file(connector->kdev, &connector_attrs_opt1[i]);
+		device_remove_file(kdev, &connector_attrs_opt1[i]);
 	for (i = 0; i < attr_cnt; i++)
-		device_remove_file(connector->kdev, &connector_attrs[i]);
-	device_unregister(connector->kdev);
+		device_remove_file(kdev, &connector_attrs[i]);
+	device_unregister(kdev);
 
 out:
 	return ret;
@@ -501,6 +504,10 @@ EXPORT_SYMBOL(drm_sysfs_hotplug_event);
 int drm_sysfs_device_add(struct drm_minor *minor)
 {
 	char *minor_str;
+	struct device *kdev;
+
+	if (minor->kdev)
+		return 0;
 
 	if (minor->type == DRM_MINOR_CONTROL)
 		minor_str = "controlD%d";
@@ -509,13 +516,16 @@ int drm_sysfs_device_add(struct drm_minor *minor)
         else
                 minor_str = "card%d";
 
-	minor->kdev = device_create(drm_class, minor->dev->dev,
-				    MKDEV(DRM_MAJOR, minor->index),
-				    minor, minor_str, minor->index);
-	if (IS_ERR(minor->kdev)) {
-		DRM_ERROR("device create failed %ld\n", PTR_ERR(minor->kdev));
-		return PTR_ERR(minor->kdev);
+	kdev = device_create(drm_class, minor->dev->dev,
+			     MKDEV(DRM_MAJOR, minor->index),
+			     minor, minor_str, minor->index);
+	if (IS_ERR(kdev)) {
+		DRM_ERROR("device create failed %ld\n", PTR_ERR(kdev));
+		return PTR_ERR(kdev);
 	}
+
+	minor->kdev = kdev;
+
 	return 0;
 }
 
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx