On Mon, 04 Nov 2013, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
