[PATCH 00/15] drm: Bounds checking, error handling, etc.

[PATCH 00/15] drm: Bounds checking, error handling, etc.

[ville.syrjala]

Mostly fixes for various bits and pieces that caught my eye while
reading the mode setting code.

[PATCH 01/15] drm: Reject mode set with current fb if no current fb is bound

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When doing a mode set with the special fb id -1, reject the mode set if
no fb is currently bound to the crtc.

Also remove the pointless list traversal to find the current crtc based
on the current crtc :)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 6fdaf6f..bbcecdb 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1755,7 +1755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_mode_crtc *crtc_req = data;
 	struct drm_mode_object *obj;
-	struct drm_crtc *crtc, *crtcfb;
+	struct drm_crtc *crtc;
 	struct drm_connector **connector_set = NULL, *connector;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_display_mode *mode = NULL;
@@ -1782,14 +1782,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		/* If we have a mode we need a framebuffer. */
 		/* If we pass -1, set the mode with the currently bound fb */
 		if (crtc_req->fb_id == -1) {
-			list_for_each_entry(crtcfb,
-					    &dev->mode_config.crtc_list, head) {
-				if (crtcfb == crtc) {
-					DRM_DEBUG_KMS("Using current fb for "
-							"setmode\n");
-					fb = crtc->fb;
-				}
+			if (!crtc->fb) {
+				DRM_DEBUG_KMS("CRTC doesn't have current FB\n");
+				ret = -EINVAL;
+				goto out;
 			}
+			fb = crtc->fb;
 		} else {
 			obj = drm_mode_object_find(dev, crtc_req->fb_id,
 						   DRM_MODE_OBJECT_FB);
-- 
1.7.3.4

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

[PATCH 02/15] drm: Change drm_display_mode::type to unsigned

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The drm_display_mode type is a bitmask so it should be unsigned.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/drm/drm_crtc.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 2a0872c..31715bd 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -121,7 +121,7 @@ struct drm_display_mode {
 	char name[DRM_DISPLAY_MODE_LEN];
 
 	enum drm_mode_status status;
-	int type;
+	unsigned int type;
 
 	/* Proposed mode values */
 	int clock;		/* in kHz */
-- 
1.7.3.4

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

[PATCH 03/15] drm: Warn if mode to umode conversion overflows the destination types

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When converting from a drm_display_mode to drm_mode_modeinfo, print a
warning if the the timings values don't fit into the __u16 datatype.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index bbcecdb..d11763f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1002,6 +1002,13 @@ EXPORT_SYMBOL(drm_mode_config_cleanup);
 void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
 			       struct drm_display_mode *in)
 {
+	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
+	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
+	     in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX ||
+	     in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX ||
+	     in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX,
+	     "timing values too large for mode info\n");
+
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
 	out->hsync_start = in->hsync_start;
-- 
1.7.3.4

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

[PATCH 04/15] drm: Check crtc x and y coordinates

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The crtc x/y panning coordinates are stored as signed integers
internally. The user provides them as unsigned, so we should check
that the user provided values actually fit in the internal datatypes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d11763f..3a42c9c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1774,6 +1774,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
+	/* For some reason crtc x/y offsets are signed internally. */
+	if (crtc_req->x > INT_MAX || crtc_req->y > INT_MAX)
+		return -ERANGE;
+
 	mutex_lock(&dev->mode_config.mutex);
 	obj = drm_mode_object_find(dev, crtc_req->crtc_id,
 				   DRM_MODE_OBJECT_CRTC);
-- 
1.7.3.4

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

[PATCH 05/15] drm: Make drm_mode_attachmode() void

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_mode_attachmode() always returns 0. Change the return type to void.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3a42c9c..d2e09d9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2379,21 +2379,17 @@ void drm_fb_release(struct drm_file *priv)
  *
  * Add @mode to @connector's user mode list.
  */
-static int drm_mode_attachmode(struct drm_device *dev,
-			       struct drm_connector *connector,
-			       struct drm_display_mode *mode)
+static void drm_mode_attachmode(struct drm_device *dev,
+				struct drm_connector *connector,
+				struct drm_display_mode *mode)
 {
-	int ret = 0;
-
 	list_add_tail(&mode->head, &connector->user_modes);
-	return ret;
 }
 
 int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc,
 			     struct drm_display_mode *mode)
 {
 	struct drm_connector *connector;
-	int ret = 0;
 	struct drm_display_mode *dup_mode;
 	int need_dup = 0;
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
@@ -2404,9 +2400,7 @@ int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc,
 				dup_mode = drm_mode_duplicate(dev, mode);
 			else
 				dup_mode = mode;
-			ret = drm_mode_attachmode(dev, connector, dup_mode);
-			if (ret)
-				return ret;
+			drm_mode_attachmode(dev, connector, dup_mode);
 			need_dup = 1;
 		}
 	}
@@ -2491,7 +2485,7 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev,
 
 	drm_crtc_convert_umode(mode, umode);
 
-	ret = drm_mode_attachmode(dev, connector, mode);
+	drm_mode_attachmode(dev, connector, mode);
 out:
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
-- 
1.7.3.4

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

[PATCH 06/15] drm: Fix memory leak in drm_mode_setcrtc()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The mode passed to the .set_config() hook was never freed. The drivers
will make a copy of the mode, so simply free it when done.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2e09d9..9ccb92f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -643,6 +643,9 @@ EXPORT_SYMBOL(drm_mode_create);
  */
 void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode)
 {
+	if (!mode)
+		return;
+
 	drm_mode_object_put(dev, &mode->base);
 
 	kfree(mode);
@@ -1812,6 +1815,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		}
 
 		mode = drm_mode_create(dev);
+		if (!mode) {
+			ret = -ENOMEM;
+			goto out;
+		}
+
 		drm_crtc_convert_umode(mode, &crtc_req->mode);
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
 	}
@@ -1881,6 +1889,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 
 out:
 	kfree(connector_set);
+	drm_mode_destroy(dev, mode);
 	mutex_unlock(&dev->mode_config.mutex);
 	return ret;
 }
-- 
1.7.3.4

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

[PATCH 07/15] drm: Check user mode against overflows

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The internal mode representation drm_display_mode uses signed data
types. When converting the user mode to internal representation,
check that the unsigned values don't overflow the signed datatypes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   33 ++++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9ccb92f..4d9e69c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1040,10 +1040,16 @@ void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
  *
  * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to
  * the caller.
+ *
+ * RETURNS:
+ * Zero on success, errno on failure.
  */
-void drm_crtc_convert_umode(struct drm_display_mode *out,
-			    struct drm_mode_modeinfo *in)
+int drm_crtc_convert_umode(struct drm_display_mode *out,
+			   struct drm_mode_modeinfo *in)
 {
+	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
+		return -ERANGE;
+
 	out->clock = in->clock;
 	out->hdisplay = in->hdisplay;
 	out->hsync_start = in->hsync_start;
@@ -1060,6 +1066,8 @@ void drm_crtc_convert_umode(struct drm_display_mode *out,
 	out->type = in->type;
 	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
 	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
+
+	return 0;
 }
 
 /**
@@ -1820,7 +1828,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
-		drm_crtc_convert_umode(mode, &crtc_req->mode);
+		ret = drm_crtc_convert_umode(mode, &crtc_req->mode);
+		if (ret) {
+			DRM_DEBUG_KMS("Invalid mode\n");
+			goto out;
+		}
+
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
 	}
 
@@ -2492,7 +2505,12 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	drm_crtc_convert_umode(mode, umode);
+	ret = drm_crtc_convert_umode(mode, umode);
+	if (ret) {
+		DRM_DEBUG_KMS("Invalid mode\n");
+		drm_mode_destroy(dev, mode);
+		goto out;
+	}
 
 	drm_mode_attachmode(dev, connector, mode);
 out:
@@ -2535,7 +2553,12 @@ int drm_mode_detachmode_ioctl(struct drm_device *dev,
 	}
 	connector = obj_to_connector(obj);
 
-	drm_crtc_convert_umode(&mode, umode);
+	ret = drm_crtc_convert_umode(&mode, umode);
+	if (ret) {
+		DRM_DEBUG_KMS("Invalid mode\n");
+		goto out;
+	}
+
 	ret = drm_mode_detachmode(dev, connector, &mode);
 out:
 	mutex_unlock(&dev->mode_config.mutex);
-- 
1.7.3.4

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

[PATCH 08/15] drm: Check CRTC viewport against framebuffer size

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure the requested CRTC viewport fits inside the
framebuffer.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4d9e69c..3f5c603 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1835,6 +1835,18 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		}
 
 		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
+
+		if (mode->hdisplay > fb->width ||
+		    mode->vdisplay > fb->height ||
+		    crtc_req->x > fb->width - mode->hdisplay ||
+		    crtc_req->y > fb->height - mode->vdisplay) {
+			DRM_DEBUG_KMS("Invalid CRTC viewport %ux%u+%u+%u for fb size %ux%u.\n",
+				      mode->hdisplay, mode->vdisplay,
+				      crtc_req->x, crtc_req->y,
+				      fb->width, fb->height);
+			ret = -ENOSPC;
+			goto out;
+		}
 	}
 
 	if (crtc_req->count_connectors == 0 && mode) {
@@ -3206,6 +3218,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	fb = obj_to_fb(obj);
 
+	if (crtc->mode.hdisplay > fb->width ||
+	    crtc->mode.vdisplay > fb->height ||
+	    crtc->x > fb->width - crtc->mode.hdisplay ||
+	    crtc->y > fb->height - crtc->mode.vdisplay) {
+		DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d.\n",
+			      fb->width, fb->height,
+			      crtc->mode.hdisplay, crtc->mode.vdisplay,
+			      crtc->x, crtc->y);
+		ret = -ENOSPC;
+		goto out;
+	}
+
 	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
 		ret = -ENOMEM;
 		spin_lock_irqsave(&dev->event_lock, flags);
-- 
1.7.3.4

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

[PATCH 09/15] drm: Fix drm_mode_attachmode_crtc()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Change drm_mode_attachmode_crtc() to take an "all or nothing" approach.
If an error is returned, there are no side effects visible.

Also change the function to always duplicate the mode passed in.

Also change the function to not give up when it finds the first
connector without and encoder.

A simpler approach would be to just remove the function completely as
it's unused currently.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   38 +++++++++++++++++++++++++++-----------
 include/drm/drm_crtc.h     |    2 +-
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3f5c603..37d34ad 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2421,24 +2421,40 @@ static void drm_mode_attachmode(struct drm_device *dev,
 }
 
 int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc,
-			     struct drm_display_mode *mode)
+			     const struct drm_display_mode *mode)
 {
 	struct drm_connector *connector;
-	struct drm_display_mode *dup_mode;
-	int need_dup = 0;
+	int ret = 0;
+	struct drm_display_mode *dup_mode, *next;
+	LIST_HEAD(list);
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (!connector->encoder)
-			break;
+			continue;
 		if (connector->encoder->crtc == crtc) {
-			if (need_dup)
-				dup_mode = drm_mode_duplicate(dev, mode);
-			else
-				dup_mode = mode;
-			drm_mode_attachmode(dev, connector, dup_mode);
-			need_dup = 1;
+			dup_mode = drm_mode_duplicate(dev, mode);
+			if (!dup_mode) {
+				ret = -ENOMEM;
+				goto out;
+			}
+			list_add_tail(&dup_mode->head, &list);
 		}
 	}
-	return 0;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		if (!connector->encoder)
+			continue;
+		if (connector->encoder->crtc == crtc)
+			list_move_tail(list.next, &connector->user_modes);
+	}
+
+	WARN_ON(!list_empty(&list));
+
+ out:
+	list_for_each_entry_safe(dup_mode, next, &list, head)
+		drm_mode_destroy(dev, dup_mode);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_mode_attachmode_crtc);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 31715bd..fe7ebc6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -869,7 +869,7 @@ extern int drm_mode_height(struct drm_display_mode *mode);
 /* for us by fb module */
 extern int drm_mode_attachmode_crtc(struct drm_device *dev,
 				    struct drm_crtc *crtc,
-				    struct drm_display_mode *mode);
+				    const struct drm_display_mode *mode);
 extern int drm_mode_detachmode_crtc(struct drm_device *dev, struct drm_display_mode *mode);
 
 extern struct drm_display_mode *drm_mode_create(struct drm_device *dev);
-- 
1.7.3.4

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

[PATCH 10/15] drm: Make drm_crtc_convert_{umode, to_umode} static and constify their params

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_crtc_convert_umode() and drm_crtc_convert_to_umode() are never
used outside drm_crtc.c, so make them static. Also make the input
mode structure const for both functions.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 37d34ad..e36bd43 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1002,8 +1002,8 @@ EXPORT_SYMBOL(drm_mode_config_cleanup);
  * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to
  * the user.
  */
-void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
-			       struct drm_display_mode *in)
+static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
+				      const struct drm_display_mode *in)
 {
 	WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX ||
 	     in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX ||
@@ -1044,8 +1044,8 @@ void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out,
  * RETURNS:
  * Zero on success, errno on failure.
  */
-int drm_crtc_convert_umode(struct drm_display_mode *out,
-			   struct drm_mode_modeinfo *in)
+static int drm_crtc_convert_umode(struct drm_display_mode *out,
+				  const struct drm_mode_modeinfo *in)
 {
 	if (in->clock > INT_MAX || in->vrefresh > INT_MAX)
 		return -ERANGE;
-- 
1.7.3.4

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

[PATCH 11/15] drm: Handle drm_object_get() failures

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check drm_mode_object_get() return value everywhere.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c        |   95 ++++++++++++++++++++++++++++---------
 drivers/gpu/drm/drm_crtc_helper.c |    2 +
 include/drm/drm_crtc.h            |   22 ++++----
 3 files changed, 85 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index e36bd43..f5b098e 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -293,9 +293,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	int ret;
 
 	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
-	if (ret) {
+	if (ret)
 		return ret;
-	}
 
 	fb->dev = dev;
 	fb->funcs = funcs;
@@ -365,19 +364,31 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
  * Caller must hold mode config lock.
  *
  * Inits a new object created as base part of an driver crtc object.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure.
  */
-void drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   const struct drm_crtc_funcs *funcs)
 {
+	int ret;
+
 	crtc->dev = dev;
 	crtc->funcs = funcs;
 
 	mutex_lock(&dev->mode_config.mutex);
-	drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
+
+	ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
+	if (ret)
+		goto out;
 
 	list_add_tail(&crtc->head, &dev->mode_config.crtc_list);
 	dev->mode_config.num_crtc++;
+
+ out:
 	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_crtc_init);
 
@@ -453,17 +464,25 @@ EXPORT_SYMBOL(drm_mode_remove);
  *
  * Initialises a preallocated connector. Connectors should be
  * subclassed as part of driver connector objects.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure.
  */
-void drm_connector_init(struct drm_device *dev,
-		     struct drm_connector *connector,
-		     const struct drm_connector_funcs *funcs,
-		     int connector_type)
+int drm_connector_init(struct drm_device *dev,
+		       struct drm_connector *connector,
+		       const struct drm_connector_funcs *funcs,
+		       int connector_type)
 {
+	int ret;
+
 	mutex_lock(&dev->mode_config.mutex);
 
+	ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
+	if (ret)
+		goto out;
+
 	connector->dev = dev;
 	connector->funcs = funcs;
-	drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
 	connector->connector_type = connector_type;
 	connector->connector_type_id =
 		++drm_connector_enum_list[connector_type].count; /* TODO */
@@ -483,7 +502,10 @@ void drm_connector_init(struct drm_device *dev,
 	drm_connector_attach_property(connector,
 				      dev->mode_config.dpms_property, 0);
 
+ out:
 	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_connector_init);
 
@@ -518,23 +540,30 @@ void drm_connector_cleanup(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_cleanup);
 
-void drm_encoder_init(struct drm_device *dev,
-		      struct drm_encoder *encoder,
-		      const struct drm_encoder_funcs *funcs,
-		      int encoder_type)
+int drm_encoder_init(struct drm_device *dev,
+		     struct drm_encoder *encoder,
+		     const struct drm_encoder_funcs *funcs,
+		     int encoder_type)
 {
+	int ret;
+
 	mutex_lock(&dev->mode_config.mutex);
 
+	ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
+	if (ret)
+		goto out;
+
 	encoder->dev = dev;
-
-	drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
 	encoder->encoder_type = encoder_type;
 	encoder->funcs = funcs;
 
 	list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
 	dev->mode_config.num_encoder++;
 
+ out:
 	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_encoder_init);
 
@@ -555,18 +584,23 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   const uint32_t *formats, uint32_t format_count,
 		   bool priv)
 {
+	int ret;
+
 	mutex_lock(&dev->mode_config.mutex);
 
+	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
+	if (ret)
+		goto out;
+
 	plane->dev = dev;
-	drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
 	plane->funcs = funcs;
 	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
 				      GFP_KERNEL);
 	if (!plane->format_types) {
 		DRM_DEBUG_KMS("out of memory when allocating plane\n");
 		drm_mode_object_put(dev, &plane->base);
-		mutex_unlock(&dev->mode_config.mutex);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out;
 	}
 
 	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
@@ -584,9 +618,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		INIT_LIST_HEAD(&plane->head);
 	}
 
+ out:
 	mutex_unlock(&dev->mode_config.mutex);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(drm_plane_init);
 
@@ -626,7 +661,11 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev)
 	if (!nmode)
 		return NULL;
 
-	drm_mode_object_get(dev, &nmode->base, DRM_MODE_OBJECT_MODE);
+	if (drm_mode_object_get(dev, &nmode->base, DRM_MODE_OBJECT_MODE)) {
+		kfree(nmode);
+		return NULL;
+	}
+
 	return nmode;
 }
 EXPORT_SYMBOL(drm_mode_create);
@@ -2597,6 +2636,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 					 const char *name, int num_values)
 {
 	struct drm_property *property = NULL;
+	int ret;
 
 	property = kzalloc(sizeof(struct drm_property), GFP_KERNEL);
 	if (!property)
@@ -2608,7 +2648,10 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 			goto fail;
 	}
 
-	drm_mode_object_get(dev, &property->base, DRM_MODE_OBJECT_PROPERTY);
+	ret = drm_mode_object_get(dev, &property->base, DRM_MODE_OBJECT_PROPERTY);
+	if (ret)
+		goto fail;
+
 	property->flags = flags;
 	property->num_values = num_values;
 	INIT_LIST_HEAD(&property->enum_blob_list);
@@ -2621,6 +2664,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags,
 	list_add_tail(&property->head, &dev->mode_config.property_list);
 	return property;
 fail:
+	kfree(property->values);
 	kfree(property);
 	return NULL;
 }
@@ -2884,6 +2928,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 							  void *data)
 {
 	struct drm_property_blob *blob;
+	int ret;
 
 	if (!length || !data)
 		return NULL;
@@ -2892,13 +2937,17 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 	if (!blob)
 		return NULL;
 
+	ret = drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB);
+	if (ret) {
+		kfree(blob);
+		return NULL;
+	}
+
 	blob->data = (void *)((char *)blob + sizeof(struct drm_property_blob));
 	blob->length = length;
 
 	memcpy(blob->data, data, length);
 
-	drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB);
-
 	list_add_tail(&blob->head, &dev->mode_config.property_blob_list);
 	return blob;
 }
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index d761d12..d9d6684 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -352,6 +352,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		return true;
 
 	adjusted_mode = drm_mode_duplicate(dev, mode);
+	if (!adjusted_mode)
+		return false;
 
 	saved_hwmode = crtc->hwmode;
 	saved_mode = crtc->mode;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index fe7ebc6..00f4007 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -815,22 +815,22 @@ struct drm_prop_enum_list {
 	char *name;
 };
 
-extern void drm_crtc_init(struct drm_device *dev,
-			  struct drm_crtc *crtc,
-			  const struct drm_crtc_funcs *funcs);
+extern int drm_crtc_init(struct drm_device *dev,
+			 struct drm_crtc *crtc,
+			 const struct drm_crtc_funcs *funcs);
 extern void drm_crtc_cleanup(struct drm_crtc *crtc);
 
-extern void drm_connector_init(struct drm_device *dev,
-			    struct drm_connector *connector,
-			    const struct drm_connector_funcs *funcs,
-			    int connector_type);
+extern int drm_connector_init(struct drm_device *dev,
+			      struct drm_connector *connector,
+			      const struct drm_connector_funcs *funcs,
+			      int connector_type);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 
-extern void drm_encoder_init(struct drm_device *dev,
-			     struct drm_encoder *encoder,
-			     const struct drm_encoder_funcs *funcs,
-			     int encoder_type);
+extern int drm_encoder_init(struct drm_device *dev,
+			    struct drm_encoder *encoder,
+			    const struct drm_encoder_funcs *funcs,
+			    int encoder_type);
 
 extern int drm_plane_init(struct drm_device *dev,
 			  struct drm_plane *plane,
-- 
1.7.3.4

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

[PATCH 12/15] drm: Use a flexible array member for blob property data

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The blob property data is always allocated immediately after the object
header. No need for the extra indirection when accessing it, just use
a flexible array member.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    1 -
 include/drm/drm_crtc.h     |    2 +-
 2 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f5b098e..d2d9dc5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2943,7 +2943,6 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev
 		return NULL;
 	}
 
-	blob->data = (void *)((char *)blob + sizeof(struct drm_property_blob));
 	blob->length = length;
 
 	memcpy(blob->data, data, length);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 00f4007..53cb49a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -257,7 +257,7 @@ struct drm_property_blob {
 	struct drm_mode_object base;
 	struct list_head head;
 	unsigned int length;
-	void *data;
+	unsigned char data[];
 };
 
 struct drm_property_enum {
-- 
1.7.3.4

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

[PATCH 13/15] drm: Eliminate pointless goto

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use a do {} while() loop instead of a goto in drm_mode_object_get().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2d9dc5..12333ca 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -224,17 +224,17 @@ static int drm_mode_object_get(struct drm_device *dev,
 	int new_id = 0;
 	int ret;
 
-again:
-	if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) {
-		DRM_ERROR("Ran out memory getting a mode number\n");
-		return -EINVAL;
-	}
+	do {
+		if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) {
+			DRM_ERROR("Ran out memory getting a mode number\n");
+			return -EINVAL;
+		}
 
-	mutex_lock(&dev->mode_config.idr_mutex);
-	ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id);
-	mutex_unlock(&dev->mode_config.idr_mutex);
-	if (ret == -EAGAIN)
-		goto again;
+		mutex_lock(&dev->mode_config.idr_mutex);
+		ret = idr_get_new_above(&dev->mode_config.crtc_idr,
+					obj, 1, &new_id);
+		mutex_unlock(&dev->mode_config.idr_mutex);
+	} while (ret == -EAGAIN);
 
 	obj->id = new_id;
 	obj->type = obj_type;
-- 
1.7.3.4

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

[PATCH 14/15] drm: Add drm_mode_copy()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a helper function to copy a display mode. Use it in
drm_mode_duplicate() and nouveau mode_fixup hooks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_modes.c        |   28 +++++++++++++++++++++++-----
 drivers/gpu/drm/nouveau/nv50_dac.c |    7 ++-----
 drivers/gpu/drm/nouveau/nv50_sor.c |    7 ++-----
 include/drm/drm_crtc.h             |    1 +
 4 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7ff13bc..b7adb4a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -714,6 +714,27 @@ EXPORT_SYMBOL(drm_mode_set_crtcinfo);
 
 
 /**
+ * drm_mode_copy - copy the mode
+ * @dst: mode to overwrite
+ * @src: mode to copy
+ *
+ * LOCKING:
+ * None.
+ *
+ * Copy an existing mode into another mode, preserving the object id
+ * of the destination mode.
+ */
+void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src)
+{
+	int id = dst->base.id;
+
+	*dst = *src;
+	dst->base.id = id;
+	INIT_LIST_HEAD(&dst->head);
+}
+EXPORT_SYMBOL(drm_mode_copy);
+
+/**
  * drm_mode_duplicate - allocate and duplicate an existing mode
  * @m: mode to duplicate
  *
@@ -727,16 +748,13 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
 					    const struct drm_display_mode *mode)
 {
 	struct drm_display_mode *nmode;
-	int new_id;
 
 	nmode = drm_mode_create(dev);
 	if (!nmode)
 		return NULL;
 
-	new_id = nmode->base.id;
-	*nmode = *mode;
-	nmode->base.id = new_id;
-	INIT_LIST_HEAD(&nmode->head);
+	drm_mode_copy(nmode, mode);
+
 	return nmode;
 }
 EXPORT_SYMBOL(drm_mode_duplicate);
diff --git a/drivers/gpu/drm/nouveau/nv50_dac.c b/drivers/gpu/drm/nouveau/nv50_dac.c
index a0f2beb..55c5633 100644
--- a/drivers/gpu/drm/nouveau/nv50_dac.c
+++ b/drivers/gpu/drm/nouveau/nv50_dac.c
@@ -190,11 +190,8 @@ nv50_dac_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	}
 
 	if (connector->scaling_mode != DRM_MODE_SCALE_NONE &&
-	     connector->native_mode) {
-		int id = adjusted_mode->base.id;
-		*adjusted_mode = *connector->native_mode;
-		adjusted_mode->base.id = id;
-	}
+	     connector->native_mode)
+		drm_mode_copy(adjusted_mode, connector->native_mode);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/nouveau/nv50_sor.c b/drivers/gpu/drm/nouveau/nv50_sor.c
index c4423ba..1706964 100644
--- a/drivers/gpu/drm/nouveau/nv50_sor.c
+++ b/drivers/gpu/drm/nouveau/nv50_sor.c
@@ -162,11 +162,8 @@ nv50_sor_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	}
 
 	if (connector->scaling_mode != DRM_MODE_SCALE_NONE &&
-	     connector->native_mode) {
-		int id = adjusted_mode->base.id;
-		*adjusted_mode = *connector->native_mode;
-		adjusted_mode->base.id = id;
-	}
+	     connector->native_mode)
+		drm_mode_copy(adjusted_mode, connector->native_mode);
 
 	return true;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 53cb49a..9595c2c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -855,6 +855,7 @@ extern struct edid *drm_get_edid(struct drm_connector *connector,
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
+extern void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src);
 extern struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev,
 						   const struct drm_display_mode *mode);
 extern void drm_mode_debug_printmodeline(struct drm_display_mode *mode);
-- 
1.7.3.4

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

[PATCH 15/15] drm: Fix drm_mode_objecte_get() return values

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Change drm_mode_object_get() to return -ENOMEM if idr_pre_get() fails,
and also handle -ENOSPC from idr_get_new_above().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 12333ca..8f66a15 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -227,7 +227,7 @@ static int drm_mode_object_get(struct drm_device *dev,
 	do {
 		if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) {
 			DRM_ERROR("Ran out memory getting a mode number\n");
-			return -EINVAL;
+			return -ENOMEM;
 		}
 
 		mutex_lock(&dev->mode_config.idr_mutex);
@@ -236,6 +236,9 @@ static int drm_mode_object_get(struct drm_device *dev,
 		mutex_unlock(&dev->mode_config.idr_mutex);
 	} while (ret == -EAGAIN);
 
+	if (ret)
+		return ret;
+
 	obj->id = new_id;
 	obj->type = obj_type;
 	return 0;
-- 
1.7.3.4

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

Re: [PATCH 00/15] drm: Bounds checking, error handling, etc.

[Alex Deucher]

On Tue, Mar 13, 2012 at 6:35 AM,  <ville.syrjala@linux.intel.com> wrote:
> Mostly fixes for various bits and pieces that caught my eye while
> reading the mode setting code.

For the series:

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Re: [PATCH 13/15] drm: Eliminate pointless goto

[Dave Airlie]

> Use a do {} while() loop instead of a goto in drm_mode_object_get().

I don't like this one just because it diverges our idr usage from the
canonical idr usage at

http://lwn.net/Articles/103209/

So unless there is a good reason for the change I'd rather not apply it.

I've applied the other patches except this and the last one which I
think depends on it.

Dave.

Re: [PATCH 13/15] drm: Eliminate pointless goto

[Ville Syrjälä]

On Thu, Mar 15, 2012 at 09:57:53AM +0000, Dave Airlie wrote:
> > Use a do {} while() loop instead of a goto in drm_mode_object_get().
> 
> I don't like this one just because it diverges our idr usage from the
> canonical idr usage at
> 
> http://lwn.net/Articles/103209/
> 
> So unless there is a good reason for the change I'd rather not apply it.

That's fine by me. Although "canonical" may be an overstatement. I just
quickly went through the idr_pre_get() usage in the tree, and between
the three conteders (no loop, goto loop, while loop) it was a close
race. No loop won with 29 idr_pre_get()s, goto loop came in second
with 24, and while loop came in third with 21.

The drm subsystem itself seems to have 5 goto loops. So if we were to
change them all, goto loop would lose to the while loop.

> I've applied the other patches except this and the last one which I
> think depends on it.

The error handling in the goto version is equally broken. I'll respin
the patch on top of the goto version, and I'll check the other idr
usage under drm as well...

-- 
Ville Syrjälä
Intel OTC