[PATCH 00/11 v2] Miscellaneous stability patches

[PATCH 00/11 v2] Miscellaneous stability patches

[Frediano Ziglio]

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

There are no code change from v1, changes:
- fix path prefix
- rebased on drm-next branch;
- add "drm/qxl" prefix on subject;
- add reviewed by.

About which patches should be applied surely (attempting to put a priority)
- "Move main reference counter to GEM object instead of TTM ones" this can
  causes memory corruption even not wanting to;
- "Avoid double free on error" this can be cause leaks in kernel if user space
  wants, mitigated by the fact that usually DRM inodes are owned by root;
- "Handle all errors in qxl_surface_evict" could cause corruption too, not
  really probable but taking into account that Xorg implementation use a lot
  signals is not so impossible;
- "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory
  if qxl_release_list_add fails" just cause leaks on situation where memory is
  already REALLY low, can be omitted;
- "Fix print statement not using uninitialized variable", "Remove format string
  errors" should just print garbage and debugging is disabled by default, not
  necessary.

Frediano Ziglio (11):
  drm/qxl: Do not cause spice-server to clean our objects
  drm/qxl: Do not leak memory if qxl_release_list_add fails
  drm/qxl: Fix print statement not using uninitialized variable
  drm/qxl: Avoid double free on error
  drm/qxl: Handle all errors in qxl_surface_evict
  drm/qxl: Fix return for qxl_release_alloc
  drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved
  drm/qxl: Remove format string errors
  drm/qxl: Move main reference counter to GEM object instead of TTM ones
  drm/qxl: Simplify cleaning qxl processing command
  drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

 drivers/gpu/drm/qxl/qxl_cmd.c     | 11 +++++-----
 drivers/gpu/drm/qxl/qxl_display.c |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.h     |  2 +-
 drivers/gpu/drm/qxl/qxl_gem.c     | 10 +++++++--
 drivers/gpu/drm/qxl/qxl_ioctl.c   | 46 +++++++++++++++------------------------
 drivers/gpu/drm/qxl/qxl_object.c  | 11 ++++------
 drivers/gpu/drm/qxl/qxl_release.c | 13 +++++++----
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH 00/11 v2] Miscellaneous stability patches

[Frediano Ziglio]

This set of patches mainly contains fix for some memory issues
using quite aggressively surfaces and other minor problems like
images going black after a while.

There are no code change from v1, changes:
- fix path prefix
- rebased on drm-next branch;
- add "drm/qxl" prefix on subject;
- add reviewed by.

About which patches should be applied surely (attempting to put a priority)
- "Move main reference counter to GEM object instead of TTM ones" this can
  causes memory corruption even not wanting to;
- "Avoid double free on error" this can be cause leaks in kernel if user space
  wants, mitigated by the fact that usually DRM inodes are owned by root;
- "Handle all errors in qxl_surface_evict" could cause corruption too, not
  really probable but taking into account that Xorg implementation use a lot
  signals is not so impossible;
- "Handle correctly failures in qxl_alloc_relase_reserved", "Do not leak memory
  if qxl_release_list_add fails" just cause leaks on situation where memory is
  already REALLY low, can be omitted;
- "Fix print statement not using uninitialized variable", "Remove format string
  errors" should just print garbage and debugging is disabled by default, not
  necessary.

Frediano Ziglio (11):
  drm/qxl: Do not cause spice-server to clean our objects
  drm/qxl: Do not leak memory if qxl_release_list_add fails
  drm/qxl: Fix print statement not using uninitialized variable
  drm/qxl: Avoid double free on error
  drm/qxl: Handle all errors in qxl_surface_evict
  drm/qxl: Fix return for qxl_release_alloc
  drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved
  drm/qxl: Remove format string errors
  drm/qxl: Move main reference counter to GEM object instead of TTM ones
  drm/qxl: Simplify cleaning qxl processing command
  drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

 drivers/gpu/drm/qxl/qxl_cmd.c     | 11 +++++-----
 drivers/gpu/drm/qxl/qxl_display.c |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.h     |  2 +-
 drivers/gpu/drm/qxl/qxl_gem.c     | 10 +++++++--
 drivers/gpu/drm/qxl/qxl_ioctl.c   | 46 +++++++++++++++------------------------
 drivers/gpu/drm/qxl/qxl_object.c  | 11 ++++------
 drivers/gpu/drm/qxl/qxl_release.c | 13 +++++++----
 7 files changed, 46 insertions(+), 49 deletions(-)

-- 
2.1.0

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

[PATCH 01/11] drm/qxl: Do not cause spice-server to clean our objects

[Frediano Ziglio]

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 
 	cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_SURFACE_CMD_CREATE;
+	cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
 	cmd->u.surface_create.format = surf->surf.format;
 	cmd->u.surface_create.width = surf->surf.width;
 	cmd->u.surface_create.height = surf->surf.height;
-- 
2.1.0

[PATCH 01/11] drm/qxl: Do not cause spice-server to clean our objects

[Frediano Ziglio]

If objects are moved back from system memory to VRAM (and spice id
created again) memory is already initialized so we need to set flag
to not clear memory.
If you don't do it after a while using desktop many images turns to
black or transparents.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index bd5404e..85ed5dc 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -502,6 +502,7 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 
 	cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_SURFACE_CMD_CREATE;
+	cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
 	cmd->u.surface_create.format = surf->surf.format;
 	cmd->u.surface_create.width = surf->surf.width;
 	cmd->u.surface_create.height = surf->surf.height;
-- 
2.1.0

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

[PATCH 02/11] drm/qxl: Do not leak memory if qxl_release_list_add fails

[Frediano Ziglio]

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret)
+	if (ret) {
+		drm_gem_object_unreference_unlocked(gobj);
 		return NULL;
+	}
 
 	return qobj;
 }
-- 
2.1.0

[PATCH 02/11] drm/qxl: Do not leak memory if qxl_release_list_add fails

[Frediano Ziglio]

If the function fails reference counter to the object is not decremented
causing leaks.
This is hard to spot as it happens only on very low memory situations.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index afd7297..e8b5207 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,8 +122,10 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret)
+	if (ret) {
+		drm_gem_object_unreference_unlocked(gobj);
 		return NULL;
+	}
 
 	return qobj;
 }
-- 
2.1.0

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

[PATCH 03/11] drm/qxl: Fix print statement not using uninitialized variable

[Frediano Ziglio]

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		/* add the bos to the list of bos to validate -
 		   need to validate first then process relocs? */
 		if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type != QXL_RELOC_TYPE_SURF) {
-			DRM_DEBUG("unknown reloc type %d\n", reloc_info[i].type);
+			DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);
 
 			ret = -EINVAL;
 			goto out_free_bos;
-- 
2.1.0

[PATCH 03/11] drm/qxl: Fix print statement not using uninitialized variable

[Frediano Ziglio]

reloc_info[i] is not still initialized in the print statement.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index e8b5207..230ab84 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -212,7 +212,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		/* add the bos to the list of bos to validate -
 		   need to validate first then process relocs? */
 		if (reloc.reloc_type != QXL_RELOC_TYPE_BO && reloc.reloc_type != QXL_RELOC_TYPE_SURF) {
-			DRM_DEBUG("unknown reloc type %d\n", reloc_info[i].type);
+			DRM_DEBUG("unknown reloc type %d\n", reloc.reloc_type);
 
 			ret = -EINVAL;
 			goto out_free_bos;
-- 
2.1.0

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

[PATCH 04/11] drm/qxl: Avoid double free on error

[Frediano Ziglio]

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 				qxlhw_handle_to_bo(qdev, file_priv,
 						   reloc.src_handle, release);
 			if (!reloc_info[i].src_bo) {
-				if (reloc_info[i].dst_bo != cmd_bo)
-					drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base);
 				ret = -EINVAL;
 				goto out_free_bos;
 			}
-- 
2.1.0

[PATCH 04/11] drm/qxl: Avoid double free on error

[Frediano Ziglio]

Is we are not able to get source bo object from handle we free
destination bo object and call cleanup code however destination
object was already inserted in reloc_info array (num_relocs was
already incremented) so on cleanup we free destination again.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 230ab84..85b3808 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -240,8 +240,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 				qxlhw_handle_to_bo(qdev, file_priv,
 						   reloc.src_handle, release);
 			if (!reloc_info[i].src_bo) {
-				if (reloc_info[i].dst_bo != cmd_bo)
-					drm_gem_object_unreference_unlocked(&reloc_info[i].dst_bo->gem_base);
 				ret = -EINVAL;
 				goto out_free_bos;
 			}
-- 
2.1.0

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

[PATCH 05/11] drm/qxl: Handle all errors in qxl_surface_evict

[Frediano Ziglio]

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 	int ret;
 
 	ret = qxl_bo_reserve(surf, false);
-	if (ret == -EBUSY)
-		return -EBUSY;
+	if (ret)
+		return ret;
 
 	if (stall)
 		mutex_unlock(&qdev->surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 
 	if (stall)
 		mutex_lock(&qdev->surf_evict_mutex);
-	if (ret == -EBUSY) {
+	if (ret) {
 		qxl_bo_unreserve(surf);
-		return -EBUSY;
+		return ret;
 	}
 
 	qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

[PATCH 05/11] drm/qxl: Handle all errors in qxl_surface_evict

[Frediano Ziglio]

Only EBUSY error was handled. This could cause code to believe
reserve was successful while it failed.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 85ed5dc..b18f84c 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -612,8 +612,8 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 	int ret;
 
 	ret = qxl_bo_reserve(surf, false);
-	if (ret == -EBUSY)
-		return -EBUSY;
+	if (ret)
+		return ret;
 
 	if (stall)
 		mutex_unlock(&qdev->surf_evict_mutex);
@@ -622,9 +622,9 @@ static int qxl_reap_surf(struct qxl_device *qdev, struct qxl_bo *surf, bool stal
 
 	if (stall)
 		mutex_lock(&qdev->surf_evict_mutex);
-	if (ret == -EBUSY) {
+	if (ret) {
 		qxl_bo_unreserve(surf);
-		return -EBUSY;
+		return ret;
 	}
 
 	qxl_surface_evict_locked(qdev, surf, true);
-- 
2.1.0

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

[PATCH 06/11] drm/qxl: Fix return for qxl_release_alloc

[Frediano Ziglio]

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
 	.wait = qxl_fence_wait,
 };
 
-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
 		  struct qxl_release **ret)
 {
-- 
2.1.0

[PATCH 06/11] drm/qxl: Fix return for qxl_release_alloc

[Frediano Ziglio]

This function return handle to allocated release object which is an int.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index d9b2568..6fd8e50 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -122,7 +122,7 @@ static const struct fence_ops qxl_fence_ops = {
 	.wait = qxl_fence_wait,
 };
 
-static uint64_t
+static int
 qxl_release_alloc(struct qxl_device *qdev, int type,
 		  struct qxl_release **ret)
 {
-- 
2.1.0

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

[PATCH 07/11] drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved

[Frediano Ziglio]

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]);
 		if (ret) {
 			mutex_unlock(&qdev->release_mutex);
+			qxl_release_free(qdev, *release);
 			return ret;
 		}
 	}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 
 	mutex_unlock(&qdev->release_mutex);
 
-	qxl_release_list_add(*release, bo);
+	ret = qxl_release_list_add(*release, bo);
+	qxl_bo_unref(&bo);
+	if (ret) {
+		qxl_release_free(qdev, *release);
+		return ret;
+	}
 
 	info = qxl_release_map(qdev, *release);
 	info->id = idr_ret;
 	qxl_release_unmap(qdev, *release, info);
 
-	qxl_bo_unref(&bo);
 	return ret;
 }
 
-- 
2.1.0

[PATCH 07/11] drm/qxl: Handle correctly failures in qxl_alloc_relase_reserved

[Frediano Ziglio]

Free resources correctly if function fails

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_release.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 6fd8e50..00604ed 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -363,6 +363,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 		ret = qxl_release_bo_alloc(qdev, &qdev->current_release_bo[cur_idx]);
 		if (ret) {
 			mutex_unlock(&qdev->release_mutex);
+			qxl_release_free(qdev, *release);
 			return ret;
 		}
 	}
@@ -377,13 +378,17 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 
 	mutex_unlock(&qdev->release_mutex);
 
-	qxl_release_list_add(*release, bo);
+	ret = qxl_release_list_add(*release, bo);
+	qxl_bo_unref(&bo);
+	if (ret) {
+		qxl_release_free(qdev, *release);
+		return ret;
+	}
 
 	info = qxl_release_map(qdev, *release);
 	info->id = idr_ret;
 	qxl_release_unmap(qdev, *release, info);
 
-	qxl_bo_unref(&bo);
 	return ret;
 }
 
-- 
2.1.0

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

[PATCH 08/11] drm/qxl: Remove format string errors

[Frediano Ziglio]

Enable format string checks for qxl_io_log and remove resulting warnings
which could lead to memory errors on different platform or just printing
wrong information.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c     | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 drivers/gpu/drm/qxl/qxl_drv.h     | 2 +-
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index b18f84c..edc1eec 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 		}
 	}
 
-	QXL_INFO(qdev, "%s: %lld\n", __func__, i);
+	QXL_INFO(qdev, "%s: %d\n", __func__, i);
 
 	return i;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 4a0a8b2..a8dbb3e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 	crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
 		  sizeof(qdev->rom->client_monitors_config));
 	if (crc != qdev->rom->client_monitors_config_crc) {
-		qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
+		qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
 			   sizeof(qdev->rom->client_monitors_config),
 			   qdev->rom->client_monitors_config_crc);
 		return 1;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 6745c44..62ef8be 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -328,7 +328,7 @@ struct qxl_device {
 };
 
 /* forward declaration for QXL_INFO_IO */
-void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
+__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
 
 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 00604ed..b66ec33 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 		return handle;
 	}
 	*ret = release;
-	QXL_INFO(qdev, "allocated release %lld\n", handle);
+	QXL_INFO(qdev, "allocated release %d\n", handle);
 	release->id = handle;
 	return handle;
 }
-- 
2.1.0

[PATCH 08/11] drm/qxl: Remove format string errors

[Frediano Ziglio]

Enable format string checks for qxl_io_log and remove resulting warnings
which could lead to memory errors on different platform or just printing
wrong information.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c     | 2 +-
 drivers/gpu/drm/qxl/qxl_display.c | 2 +-
 drivers/gpu/drm/qxl/qxl_drv.h     | 2 +-
 drivers/gpu/drm/qxl/qxl_release.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index b18f84c..edc1eec 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -248,7 +248,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 		}
 	}
 
-	QXL_INFO(qdev, "%s: %lld\n", __func__, i);
+	QXL_INFO(qdev, "%s: %d\n", __func__, i);
 
 	return i;
 }
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 4a0a8b2..a8dbb3e 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -67,7 +67,7 @@ static int qxl_display_copy_rom_client_monitors_config(struct qxl_device *qdev)
 	crc = crc32(0, (const uint8_t *)&qdev->rom->client_monitors_config,
 		  sizeof(qdev->rom->client_monitors_config));
 	if (crc != qdev->rom->client_monitors_config_crc) {
-		qxl_io_log(qdev, "crc mismatch: have %X (%d) != %X\n", crc,
+		qxl_io_log(qdev, "crc mismatch: have %X (%zd) != %X\n", crc,
 			   sizeof(qdev->rom->client_monitors_config),
 			   qdev->rom->client_monitors_config_crc);
 		return 1;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 6745c44..62ef8be 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -328,7 +328,7 @@ struct qxl_device {
 };
 
 /* forward declaration for QXL_INFO_IO */
-void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
+__printf(2,3) void qxl_io_log(struct qxl_device *qdev, const char *fmt, ...);
 
 extern const struct drm_ioctl_desc qxl_ioctls[];
 extern int qxl_max_ioctl;
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index 00604ed..b66ec33 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -153,7 +153,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 		return handle;
 	}
 	*ret = release;
-	QXL_INFO(qdev, "allocated release %lld\n", handle);
+	QXL_INFO(qdev, "allocated release %d\n", handle);
 	release->id = handle;
 	return handle;
 }
-- 
2.1.0

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

[PATCH 09/11] drm/qxl: Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_gem.c    | 10 ++++++++--
 drivers/gpu/drm/qxl/qxl_object.c | 11 ++++-------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
 	struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+	struct qxl_device *qdev;
+	struct ttm_buffer_object *tbo;
 
-	if (qobj)
-		qxl_bo_unref(&qobj);
+	qdev = (struct qxl_device *)gobj->dev->dev_private;
+
+	qxl_surface_evict(qdev, qobj, false);
+
+	tbo = &qobj->tbo;
+	ttm_bo_unref(&tbo);
 }
 
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 
 void qxl_bo_unref(struct qxl_bo **bo)
 {
-	struct ttm_buffer_object *tbo;
-
 	if ((*bo) == NULL)
 		return;
-	tbo = &((*bo)->tbo);
-	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
+
+	drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+	*bo = NULL;
 }
 
 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-	ttm_bo_reference(&bo->tbo);
+	drm_gem_object_reference(&bo->gem_base);
 	return bo;
 }
 
-- 
2.1.0

[PATCH 09/11] drm/qxl: Move main reference counter to GEM object instead of TTM ones

[Frediano Ziglio]

qxl_bo structure has two reference counters, one in the GEM object and
another in the TTM object. The GEM object keep a counter to the TTM object
so when GEM counter reached zero the TTM counter (using qxl_bo_unref) was
decremented. The qxl object is fully freed (both GEM and TTM part are cleaned)
when the TTM counter reach zero.
One issue was that surface idr structure has no owning on qxl_bo objects however
it contains a pointer to qxl_bo object. This caused some nasty race condition
for instance qxl_bo object was reaped even after counter was already zero.
This patch fix these races moving main counter (the one used by qxl_bo_(un)ref)
to GEM object which cleanup routine (qxl_gem_object_free) remove the idr pointer
(using qxl_surface_evict) when the counters are still valid.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_gem.c    | 10 ++++++++--
 drivers/gpu/drm/qxl/qxl_object.c | 11 ++++-------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index b96f0c9..d9746e9 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -31,9 +31,15 @@
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
 	struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
+	struct qxl_device *qdev;
+	struct ttm_buffer_object *tbo;
 
-	if (qobj)
-		qxl_bo_unref(&qobj);
+	qdev = (struct qxl_device *)gobj->dev->dev_private;
+
+	qxl_surface_evict(qdev, qobj, false);
+
+	tbo = &qobj->tbo;
+	ttm_bo_unref(&tbo);
 }
 
 int qxl_gem_object_create(struct qxl_device *qdev, int size,
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index cdeaf08..6d6f33d 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -208,19 +208,16 @@ void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
 
 void qxl_bo_unref(struct qxl_bo **bo)
 {
-	struct ttm_buffer_object *tbo;
-
 	if ((*bo) == NULL)
 		return;
-	tbo = &((*bo)->tbo);
-	ttm_bo_unref(&tbo);
-	if (tbo == NULL)
-		*bo = NULL;
+
+	drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+	*bo = NULL;
 }
 
 struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
 {
-	ttm_bo_reference(&bo->tbo);
+	drm_gem_object_reference(&bo->gem_base);
 	return bo;
 }
 
-- 
2.1.0

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

[PATCH 10/11] drm/qxl: Simplify cleaning qxl processing command

[Frediano Ziglio]

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(gobj);
+	drm_gem_object_unreference_unlocked(gobj);
+	if (ret)
 		return NULL;
-	}
 
 	return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	struct qxl_release *release;
 	struct qxl_bo *cmd_bo;
 	void *fb_cmd;
-	int i, j, ret, num_relocs;
+	int i, ret, num_relocs;
 	int unwritten;
 
 	switch (cmd->type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		qxl_release_fence_buffer_objects(release);
 
 out_free_bos:
-	for (j = 0; j < num_relocs; j++) {
-		if (reloc_info[j].dst_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
-		if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
-	}
 out_free_release:
 	if (ret)
 		qxl_release_free(qdev, release);
-- 
2.1.0

[PATCH 10/11] drm/qxl: Simplify cleaning qxl processing command

[Frediano Ziglio]

In qxlhw_handle_to_bo we incremented counters twice, one time for release object
and one for reloc_info.
In the main function however reloc_info references was drop much earlier than
release so keeping the pointer only on release is safe and make cleaning
process easier.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 85b3808..bb326ff 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -122,10 +122,9 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
-	if (ret) {
-		drm_gem_object_unreference_unlocked(gobj);
+	drm_gem_object_unreference_unlocked(gobj);
+	if (ret)
 		return NULL;
-	}
 
 	return qobj;
 }
@@ -145,7 +144,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	struct qxl_release *release;
 	struct qxl_bo *cmd_bo;
 	void *fb_cmd;
-	int i, j, ret, num_relocs;
+	int i, ret, num_relocs;
 	int unwritten;
 
 	switch (cmd->type) {
@@ -269,12 +268,6 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		qxl_release_fence_buffer_objects(release);
 
 out_free_bos:
-	for (j = 0; j < num_relocs; j++) {
-		if (reloc_info[j].dst_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].dst_bo->gem_base);
-		if (reloc_info[j].src_bo && reloc_info[j].src_bo != cmd_bo)
-			drm_gem_object_unreference_unlocked(&reloc_info[j].src_bo->gem_base);
-	}
 out_free_release:
 	if (ret)
 		qxl_release_free(qdev, release);
-- 
2.1.0

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

[PATCH 11/11] drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

[Frediano Ziglio]

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct qxl_reloc_info *info)
 }
 
 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-					 struct drm_file *file_priv, uint64_t handle,
-					 struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+			      struct drm_file *file_priv, uint64_t handle,
+			      struct qxl_release *release, struct qxl_bo **qbo_p)
 {
 	struct drm_gem_object *gobj;
 	struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 
 	gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
 	if (!gobj)
-		return NULL;
+		return -EINVAL;
 
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
 	drm_gem_object_unreference_unlocked(gobj);
 	if (ret)
-		return NULL;
+		return ret;
 
-	return qobj;
+	*qbo_p = qobj;
+	return 0;
 }
 
 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		reloc_info[i].type = reloc.reloc_type;
 
 		if (reloc.dst_handle) {
-			reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, file_priv,
-								  reloc.dst_handle, release);
-			if (!reloc_info[i].dst_bo) {
-				ret = -EINVAL;
-				reloc_info[i].src_bo = NULL;
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.dst_handle, release,
+						 &reloc_info[i].dst_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].dst_offset = reloc.dst_offset;
 		} else {
 			reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		num_relocs++;
 
 		/* reserve and validate the reloc dst bo */
-		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 0) {
-			reloc_info[i].src_bo =
-				qxlhw_handle_to_bo(qdev, file_priv,
-						   reloc.src_handle, release);
-			if (!reloc_info[i].src_bo) {
-				ret = -EINVAL;
+		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release,
+						 &reloc_info[i].src_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].src_offset = reloc.src_offset;
 		} else {
 			reloc_info[i].src_bo = NULL;
-- 
2.1.0

[PATCH 11/11] drm/qxl: Propagate correctly errors from qxlhw_handle_to_bo

[Frediano Ziglio]

This function could return a NULL pointer in case of handle not
present and in case of out of memory conditions however caller
function always returned EINVAL error hiding a possible ENOMEM.
This patch change the function to return the error instead to
be able to propagate the error instead of assuming EINVAL.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/qxl/qxl_ioctl.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index bb326ff..37f1faf 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -107,9 +107,9 @@ apply_surf_reloc(struct qxl_device *qdev, struct qxl_reloc_info *info)
 }
 
 /* return holding the reference to this object */
-static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
-					 struct drm_file *file_priv, uint64_t handle,
-					 struct qxl_release *release)
+static int qxlhw_handle_to_bo(struct qxl_device *qdev,
+			      struct drm_file *file_priv, uint64_t handle,
+			      struct qxl_release *release, struct qxl_bo **qbo_p)
 {
 	struct drm_gem_object *gobj;
 	struct qxl_bo *qobj;
@@ -117,16 +117,17 @@ static struct qxl_bo *qxlhw_handle_to_bo(struct qxl_device *qdev,
 
 	gobj = drm_gem_object_lookup(qdev->ddev, file_priv, handle);
 	if (!gobj)
-		return NULL;
+		return -EINVAL;
 
 	qobj = gem_to_qxl_bo(gobj);
 
 	ret = qxl_release_list_add(release, qobj);
 	drm_gem_object_unreference_unlocked(gobj);
 	if (ret)
-		return NULL;
+		return ret;
 
-	return qobj;
+	*qbo_p = qobj;
+	return 0;
 }
 
 /*
@@ -219,13 +220,10 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		reloc_info[i].type = reloc.reloc_type;
 
 		if (reloc.dst_handle) {
-			reloc_info[i].dst_bo = qxlhw_handle_to_bo(qdev, file_priv,
-								  reloc.dst_handle, release);
-			if (!reloc_info[i].dst_bo) {
-				ret = -EINVAL;
-				reloc_info[i].src_bo = NULL;
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.dst_handle, release,
+						 &reloc_info[i].dst_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].dst_offset = reloc.dst_offset;
 		} else {
 			reloc_info[i].dst_bo = cmd_bo;
@@ -234,14 +232,11 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 		num_relocs++;
 
 		/* reserve and validate the reloc dst bo */
-		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle > 0) {
-			reloc_info[i].src_bo =
-				qxlhw_handle_to_bo(qdev, file_priv,
-						   reloc.src_handle, release);
-			if (!reloc_info[i].src_bo) {
-				ret = -EINVAL;
+		if (reloc.reloc_type == QXL_RELOC_TYPE_BO || reloc.src_handle) {
+			ret = qxlhw_handle_to_bo(qdev, file_priv, reloc.src_handle, release,
+						 &reloc_info[i].src_bo);
+			if (ret)
 				goto out_free_bos;
-			}
 			reloc_info[i].src_offset = reloc.src_offset;
 		} else {
 			reloc_info[i].src_bo = NULL;
-- 
2.1.0

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