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

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

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