[PATCH] drm: Fix locking for sysfs dpms file

[PATCH] drm: Fix locking for sysfs dpms file

[Daniel Vetter]

With atomic drivers we need to make sure that (at least in general)
property reads hold the right locks. But the legacy dpms property is
special and can be read locklessly. Since userspace loves to just
randomly look at that all the time (like with "status") do that.

To make it clear that we play tricks use the READ_ONCE compiler
barrier (and also for paranoia).

Note that there's not really anything bad going on since even with the
new atomic paths we eventually end up not chasing any pointers (and
hence possibly freed memory and other fun stuff). The locking WARNING
has been added in

commit 88a48e297b3a3bac6022c03babfb038f1a886cea
Author: Rob Clark <robdclark@gmail.com>
Date:   Thu Dec 18 16:01:50 2014 -0500

    drm: add atomic properties

but since drivers are converting not everyone will have seen this from
the start.

Jens reported this and submitted a patch to just grab the
mode_config.connection_mutex, but we can do a bit better.

Reported-by: Jens Axboe <axboe@fb.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f08873f6489c..63adcd79226f 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -231,17 +231,13 @@ static ssize_t dpms_show(struct device *device,
 {
 	struct drm_connector *connector = to_drm_connector(device);
 	struct drm_device *dev = connector->dev;
-	uint64_t dpms_status;
+	int dpms;
 	int ret;
 
-	ret = drm_object_property_get_value(&connector->base,
-					    dev->mode_config.dpms_property,
-					    &dpms_status);
-	if (ret)
-		return 0;
+	dpms = READ_ONCE(connector->dpms);
 
 	return snprintf(buf, PAGE_SIZE, "%s\n",
-			drm_get_dpms_name((int)dpms_status));
+			drm_get_dpms_name(dpms));
 }
 
 static ssize_t enabled_show(struct device *device,
-- 
2.5.1

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

[PATCH] drm: Fix locking for sysfs dpms file

[Daniel Vetter]

With atomic drivers we need to make sure that (at least in general)
property reads hold the right locks. But the legacy dpms property is
special and can be read locklessly. Since userspace loves to just
randomly look at that all the time (like with "status") do that.

To make it clear that we play tricks use the READ_ONCE compiler
barrier (and also for paranoia).

Note that there's not really anything bad going on since even with the
new atomic paths we eventually end up not chasing any pointers (and
hence possibly freed memory and other fun stuff). The locking WARNING
has been added in

commit 88a48e297b3a3bac6022c03babfb038f1a886cea
Author: Rob Clark <robdclark@gmail.com>
Date:   Thu Dec 18 16:01:50 2014 -0500

    drm: add atomic properties

but since drivers are converting not everyone will have seen this from
the start.

Jens reported this and submitted a patch to just grab the
mode_config.connection_mutex, but we can do a bit better.

v2: Remove unused variables I failed to git add for real.

Reported-by: Jens Axboe <axboe@fb.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f08873f6489c..615b7e667320 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -230,18 +230,12 @@ static ssize_t dpms_show(struct device *device,
 			   char *buf)
 {
 	struct drm_connector *connector = to_drm_connector(device);
-	struct drm_device *dev = connector->dev;
-	uint64_t dpms_status;
-	int ret;
+	int dpms;
 
-	ret = drm_object_property_get_value(&connector->base,
-					    dev->mode_config.dpms_property,
-					    &dpms_status);
-	if (ret)
-		return 0;
+	dpms = READ_ONCE(connector->dpms);
 
 	return snprintf(buf, PAGE_SIZE, "%s\n",
-			drm_get_dpms_name((int)dpms_status));
+			drm_get_dpms_name(dpms));
 }
 
 static ssize_t enabled_show(struct device *device,
-- 
2.5.1

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

[PATCH 1/3] drm/sysfs: Annote lockless show functions with READ_ONCE

[Daniel Vetter]

For documentation and paranoia.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_sysfs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 615b7e667320..7506de0a75b4 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -220,9 +220,12 @@ static ssize_t status_show(struct device *device,
 			   char *buf)
 {
 	struct drm_connector *connector = to_drm_connector(device);
+	enum drm_connector_status status;
+
+	status = READ_ONCE(connector->status);
 
 	return snprintf(buf, PAGE_SIZE, "%s\n",
-			drm_get_connector_status_name(connector->status));
+			drm_get_connector_status_name(status));
 }
 
 static ssize_t dpms_show(struct device *device,
@@ -243,9 +246,11 @@ static ssize_t enabled_show(struct device *device,
 			   char *buf)
 {
 	struct drm_connector *connector = to_drm_connector(device);
+	bool enabled;
+
+	enabled = READ_ONCE(connector->encoder);
 
-	return snprintf(buf, PAGE_SIZE, "%s\n", connector->encoder ? "enabled" :
-			"disabled");
+	return snprintf(buf, PAGE_SIZE, enabled ? "enabled\n" : "disabled\n");
 }
 
 static ssize_t edid_show(struct file *filp, struct kobject *kobj,
-- 
2.5.1

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

[PATCH 2/3] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

We chase pointers/lists without taking the locks protecting them,
which isn't that good.

Fix it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_sysfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 7506de0a75b4..d55e0d7a610d 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct kobject *kobj,
 	struct drm_connector *connector = to_drm_connector(connector_dev);
 	unsigned char *edid;
 	size_t size;
+	ssize_t ret = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	if (!connector->edid_blob_ptr)
-		return 0;
+		goto unlock;
 
 	edid = connector->edid_blob_ptr->data;
 	size = connector->edid_blob_ptr->length;
 	if (!edid)
-		return 0;
+		goto unlock;
 
 	if (off >= size)
-		return 0;
+		goto unlock;
 
 	if (off + count > size)
 		count = size - off;
 	memcpy(buf, edid + off, count);
 
-	return count;
+	ret = count;
+	mutex_lock(&connector->dev->mode_config.mutex);
+unlock:
+
+	return ret;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
 	struct drm_display_mode *mode;
 	int written = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	list_for_each_entry(mode, &connector->modes, head) {
 		written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
 				    mode->name);
 	}
+	mutex_unlock(&connector->dev->mode_config.mutex);
 
 	return written;
 }
-- 
2.5.1

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

[PATCH 3/3] drm/sysfs: Nuke TV/DVI property files

[Daniel Vetter]

This goes all the way back to the original KMS commit aeons ago

commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Nov 7 14:05:41 2008 -0800

    DRM: add mode setting support

But it seems to be completely unused. Only i915 and nouveau even
register these properties, and the corresponding DDX don't even look
at them. Also the sysfs files are read-only, so not useful to
configure anything.

I suspect that this was added with the goal to have read-only access
to all properties in sysfs, but we never followed through on that.
Also, that should be done in a more generic fashion.

Since it would be real work to fix up the locking (with atomic we're
now chasing pointers when reading properties) and it seems unused lets
just nuke this all. It's easier. Of course we'll keep the properties
themselves, those are still exposed through the KMS ioctls.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_sysfs.c | 156 --------------------------------------------
 1 file changed, 156 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index d55e0d7a610d..5ccd0b1eea78 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -304,102 +304,6 @@ static ssize_t modes_show(struct device *device,
 	return written;
 }
 
-static ssize_t tv_subconnector_show(struct device *device,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct drm_connector *connector = to_drm_connector(device);
-	struct drm_device *dev = connector->dev;
-	struct drm_property *prop;
-	uint64_t subconnector;
-	int ret;
-
-	prop = dev->mode_config.tv_subconnector_property;
-	if (!prop) {
-		DRM_ERROR("Unable to find subconnector property\n");
-		return 0;
-	}
-
-	ret = drm_object_property_get_value(&connector->base, prop, &subconnector);
-	if (ret)
-		return 0;
-
-	return snprintf(buf, PAGE_SIZE, "%s",
-			drm_get_tv_subconnector_name((int)subconnector));
-}
-
-static ssize_t tv_select_subconnector_show(struct device *device,
-					   struct device_attribute *attr,
-					   char *buf)
-{
-	struct drm_connector *connector = to_drm_connector(device);
-	struct drm_device *dev = connector->dev;
-	struct drm_property *prop;
-	uint64_t subconnector;
-	int ret;
-
-	prop = dev->mode_config.tv_select_subconnector_property;
-	if (!prop) {
-		DRM_ERROR("Unable to find select subconnector property\n");
-		return 0;
-	}
-
-	ret = drm_object_property_get_value(&connector->base, prop, &subconnector);
-	if (ret)
-		return 0;
-
-	return snprintf(buf, PAGE_SIZE, "%s",
-			drm_get_tv_select_name((int)subconnector));
-}
-
-static ssize_t dvii_subconnector_show(struct device *device,
-				      struct device_attribute *attr,
-				      char *buf)
-{
-	struct drm_connector *connector = to_drm_connector(device);
-	struct drm_device *dev = connector->dev;
-	struct drm_property *prop;
-	uint64_t subconnector;
-	int ret;
-
-	prop = dev->mode_config.dvi_i_subconnector_property;
-	if (!prop) {
-		DRM_ERROR("Unable to find subconnector property\n");
-		return 0;
-	}
-
-	ret = drm_object_property_get_value(&connector->base, prop, &subconnector);
-	if (ret)
-		return 0;
-
-	return snprintf(buf, PAGE_SIZE, "%s",
-			drm_get_dvi_i_subconnector_name((int)subconnector));
-}
-
-static ssize_t dvii_select_subconnector_show(struct device *device,
-					     struct device_attribute *attr,
-					     char *buf)
-{
-	struct drm_connector *connector = to_drm_connector(device);
-	struct drm_device *dev = connector->dev;
-	struct drm_property *prop;
-	uint64_t subconnector;
-	int ret;
-
-	prop = dev->mode_config.dvi_i_select_subconnector_property;
-	if (!prop) {
-		DRM_ERROR("Unable to find select subconnector property\n");
-		return 0;
-	}
-
-	ret = drm_object_property_get_value(&connector->base, prop, &subconnector);
-	if (ret)
-		return 0;
-
-	return snprintf(buf, PAGE_SIZE, "%s",
-			drm_get_dvi_i_select_name((int)subconnector));
-}
-
 static DEVICE_ATTR_RW(status);
 static DEVICE_ATTR_RO(enabled);
 static DEVICE_ATTR_RO(dpms);
@@ -413,54 +317,6 @@ static struct attribute *connector_dev_attrs[] = {
 	NULL
 };
 
-static DEVICE_ATTR_RO(tv_subconnector);
-static DEVICE_ATTR_RO(tv_select_subconnector);
-
-static struct attribute *connector_tv_dev_attrs[] = {
-	&dev_attr_tv_subconnector.attr,
-	&dev_attr_tv_select_subconnector.attr,
-	NULL
-};
-
-static DEVICE_ATTR_RO(dvii_subconnector);
-static DEVICE_ATTR_RO(dvii_select_subconnector);
-
-static struct attribute *connector_dvii_dev_attrs[] = {
-	&dev_attr_dvii_subconnector.attr,
-	&dev_attr_dvii_select_subconnector.attr,
-	NULL
-};
-
-/* Connector type related helpers */
-static int kobj_connector_type(struct kobject *kobj)
-{
-	struct device *dev = kobj_to_dev(kobj);
-	struct drm_connector *connector = to_drm_connector(dev);
-
-	return connector->connector_type;
-}
-
-static umode_t connector_is_dvii(struct kobject *kobj,
-				 struct attribute *attr, int idx)
-{
-	return kobj_connector_type(kobj) == DRM_MODE_CONNECTOR_DVII ?
-		attr->mode : 0;
-}
-
-static umode_t connector_is_tv(struct kobject *kobj,
-			       struct attribute *attr, int idx)
-{
-	switch (kobj_connector_type(kobj)) {
-	case DRM_MODE_CONNECTOR_Composite:
-	case DRM_MODE_CONNECTOR_SVIDEO:
-	case DRM_MODE_CONNECTOR_Component:
-	case DRM_MODE_CONNECTOR_TV:
-		return attr->mode;
-	}
-
-	return 0;
-}
-
 static struct bin_attribute edid_attr = {
 	.attr.name = "edid",
 	.attr.mode = 0444,
@@ -478,20 +334,8 @@ static const struct attribute_group connector_dev_group = {
 	.bin_attrs = connector_bin_attrs,
 };
 
-static const struct attribute_group connector_tv_dev_group = {
-	.attrs = connector_tv_dev_attrs,
-	.is_visible = connector_is_tv,
-};
-
-static const struct attribute_group connector_dvii_dev_group = {
-	.attrs = connector_dvii_dev_attrs,
-	.is_visible = connector_is_dvii,
-};
-
 static const struct attribute_group *connector_dev_groups[] = {
 	&connector_dev_group,
-	&connector_tv_dev_group,
-	&connector_dvii_dev_group,
 	NULL
 };
 
-- 
2.5.1

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

Re: [PATCH] drm: Fix locking for sysfs dpms file

[Jani Nikula]

On Tue, 29 Sep 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> With atomic drivers we need to make sure that (at least in general)
> property reads hold the right locks. But the legacy dpms property is
> special and can be read locklessly. Since userspace loves to just
> randomly look at that all the time (like with "status") do that.
>
> To make it clear that we play tricks use the READ_ONCE compiler
> barrier (and also for paranoia).
>
> Note that there's not really anything bad going on since even with the
> new atomic paths we eventually end up not chasing any pointers (and
> hence possibly freed memory and other fun stuff). The locking WARNING
> has been added in
>
> commit 88a48e297b3a3bac6022c03babfb038f1a886cea
> Author: Rob Clark <robdclark@gmail.com>
> Date:   Thu Dec 18 16:01:50 2014 -0500
>
>     drm: add atomic properties
>
> but since drivers are converting not everyone will have seen this from
> the start.
>
> Jens reported this and submitted a patch to just grab the
> mode_config.connection_mutex, but we can do a bit better.
>
> v2: Remove unused variables I failed to git add for real.
>

Reference: http://mid.gmane.org/20150928194822.GA3930@kernel.dk

> Reported-by: Jens Axboe <axboe@fb.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index f08873f6489c..615b7e667320 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -230,18 +230,12 @@ static ssize_t dpms_show(struct device *device,
>  			   char *buf)
>  {
>  	struct drm_connector *connector = to_drm_connector(device);
> -	struct drm_device *dev = connector->dev;
> -	uint64_t dpms_status;
> -	int ret;
> +	int dpms;
>  
> -	ret = drm_object_property_get_value(&connector->base,
> -					    dev->mode_config.dpms_property,
> -					    &dpms_status);
> -	if (ret)
> -		return 0;
> +	dpms = READ_ONCE(connector->dpms);
>  
>  	return snprintf(buf, PAGE_SIZE, "%s\n",
> -			drm_get_dpms_name((int)dpms_status));
> +			drm_get_dpms_name(dpms));
>  }
>  
>  static ssize_t enabled_show(struct device *device,
> -- 
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: Fix locking for sysfs dpms file

[Jens Axboe]

On 09/29/2015 01:56 AM, Daniel Vetter wrote:
> With atomic drivers we need to make sure that (at least in general)
> property reads hold the right locks. But the legacy dpms property is
> special and can be read locklessly. Since userspace loves to just
> randomly look at that all the time (like with "status") do that.
>
> To make it clear that we play tricks use the READ_ONCE compiler
> barrier (and also for paranoia).
>
> Note that there's not really anything bad going on since even with the
> new atomic paths we eventually end up not chasing any pointers (and
> hence possibly freed memory and other fun stuff). The locking WARNING
> has been added in
>
> commit 88a48e297b3a3bac6022c03babfb038f1a886cea
> Author: Rob Clark <robdclark@gmail.com>
> Date:   Thu Dec 18 16:01:50 2014 -0500
>
>      drm: add atomic properties
>
> but since drivers are converting not everyone will have seen this from
> the start.
>
> Jens reported this and submitted a patch to just grab the
> mode_config.connection_mutex, but we can do a bit better.
>
> v2: Remove unused variables I failed to git add for real.
>
> Reported-by: Jens Axboe <axboe@fb.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Works for me, thanks Daniel.

Tested-by: Jens Axboe <axboe@fb.com>

-- 
Jens Axboe

[PATCH] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

We chase pointers/lists without taking the locks protecting them,
which isn't that good.

Fix it.

v2: Actually unlock properly, spotted by Julia.

Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_sysfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 7506de0a75b4..9bffa63fe849 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct kobject *kobj,
 	struct drm_connector *connector = to_drm_connector(connector_dev);
 	unsigned char *edid;
 	size_t size;
+	ssize_t ret = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	if (!connector->edid_blob_ptr)
-		return 0;
+		goto unlock;
 
 	edid = connector->edid_blob_ptr->data;
 	size = connector->edid_blob_ptr->length;
 	if (!edid)
-		return 0;
+		goto unlock;
 
 	if (off >= size)
-		return 0;
+		goto unlock;
 
 	if (off + count > size)
 		count = size - off;
 	memcpy(buf, edid + off, count);
 
-	return count;
+	ret = count;
+	mutex_unlock(&connector->dev->mode_config.mutex);
+unlock:
+
+	return ret;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
 	struct drm_display_mode *mode;
 	int written = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	list_for_each_entry(mode, &connector->modes, head) {
 		written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
 				    mode->name);
 	}
+	mutex_unlock(&connector->dev->mode_config.mutex);
 
 	return written;
 }
-- 
2.5.1

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

[PATCH] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

We chase pointers/lists without taking the locks protecting them,
which isn't that good.

Fix it.

v2: Actually unlock properly, spotted by Julia.

v3: Put the label _before_ the mutex_unlock (Emil)

Cc: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_sysfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 7506de0a75b4..8ceb1cb6166a 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct kobject *kobj,
 	struct drm_connector *connector = to_drm_connector(connector_dev);
 	unsigned char *edid;
 	size_t size;
+	ssize_t ret = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	if (!connector->edid_blob_ptr)
-		return 0;
+		goto unlock;
 
 	edid = connector->edid_blob_ptr->data;
 	size = connector->edid_blob_ptr->length;
 	if (!edid)
-		return 0;
+		goto unlock;
 
 	if (off >= size)
-		return 0;
+		goto unlock;
 
 	if (off + count > size)
 		count = size - off;
 	memcpy(buf, edid + off, count);
 
-	return count;
+	ret = count;
+unlock:
+	mutex_unlock(&connector->dev->mode_config.mutex);
+
+	return ret;
 }
 
 static ssize_t modes_show(struct device *device,
@@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
 	struct drm_display_mode *mode;
 	int written = 0;
 
+	mutex_lock(&connector->dev->mode_config.mutex);
 	list_for_each_entry(mode, &connector->modes, head) {
 		written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
 				    mode->name);
 	}
+	mutex_unlock(&connector->dev->mode_config.mutex);
 
 	return written;
 }
-- 
2.5.1

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

Re: [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Emil Velikov]

On 2 October 2015 at 12:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We chase pointers/lists without taking the locks protecting them,
> which isn't that good.
>
> Fix it.
>
> v2: Actually unlock properly, spotted by Julia.
>
> v3: Put the label _before_ the mutex_unlock (Emil)
>
> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Patch does exactly what it says on the tin.

Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

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

Re: [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Julia Lawall]

On Fri, 2 Oct 2015, Daniel Vetter wrote:

> We chase pointers/lists without taking the locks protecting them,
> which isn't that good.
>
> Fix it.
>
> v2: Actually unlock properly, spotted by Julia.

The unlock is still on top of the unlock label?

julia

>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_sysfs.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 7506de0a75b4..9bffa63fe849 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -261,23 +261,29 @@ static ssize_t edid_show(struct file *filp, struct kobject *kobj,
>  	struct drm_connector *connector = to_drm_connector(connector_dev);
>  	unsigned char *edid;
>  	size_t size;
> +	ssize_t ret = 0;
>
> +	mutex_lock(&connector->dev->mode_config.mutex);
>  	if (!connector->edid_blob_ptr)
> -		return 0;
> +		goto unlock;
>
>  	edid = connector->edid_blob_ptr->data;
>  	size = connector->edid_blob_ptr->length;
>  	if (!edid)
> -		return 0;
> +		goto unlock;
>
>  	if (off >= size)
> -		return 0;
> +		goto unlock;
>
>  	if (off + count > size)
>  		count = size - off;
>  	memcpy(buf, edid + off, count);
>
> -	return count;
> +	ret = count;
> +	mutex_unlock(&connector->dev->mode_config.mutex);
> +unlock:
> +
> +	return ret;
>  }
>
>  static ssize_t modes_show(struct device *device,
> @@ -288,10 +294,12 @@ static ssize_t modes_show(struct device *device,
>  	struct drm_display_mode *mode;
>  	int written = 0;
>
> +	mutex_lock(&connector->dev->mode_config.mutex);
>  	list_for_each_entry(mode, &connector->modes, head) {
>  		written += snprintf(buf + written, PAGE_SIZE - written, "%s\n",
>  				    mode->name);
>  	}
> +	mutex_unlock(&connector->dev->mode_config.mutex);
>
>  	return written;
>  }
> --
> 2.5.1
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/sysfs: Grab lock for edid/modes_show

[Daniel Vetter]

On Fri, Oct 02, 2015 at 01:08:40PM +0100, Emil Velikov wrote:
> On 2 October 2015 at 12:01, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We chase pointers/lists without taking the locks protecting them,
> > which isn't that good.
> >
> > Fix it.
> >
> > v2: Actually unlock properly, spotted by Julia.
> >
> > v3: Put the label _before_ the mutex_unlock (Emil)
> >
> > Cc: Emil Velikov <emil.l.velikov@gmail.com>
> > Cc: Julia Lawall <julia.lawall@lip6.fr>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Patch does exactly what it says on the tin.
> 
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx