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

[Jani Nikula <jani.nikula@linux.intel.com>]

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