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

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