diff mbox

[4/5] drm: WARN when removing unallocated node

Message ID 1376111536-12461-4-git-send-email-benjamin.widawsky@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Widawsky Aug. 10, 2013, 5:12 a.m. UTC
The conditional is usually a recoverable driver bug, and so WARNing, and
preventing the drm_mm code from doing potential damage (BUG) is
desirable.

This issue was hit and fixed twice while developing the i915 multiple
address space code. The first fix is the patch just before this, and is
hit on an not frequently occuring error path. Another was fixed during
patch iteration, so it's hard to see from the patch:

commit c6cfb325677ea6305fb19acf3a4d14ea267f923e
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Fri Jul 5 14:41:06 2013 -0700

    drm/i915: Embed drm_mm_node in i915 gem obj

From the intel-gfx mailing list, we discussed this:
References: <20130705191235.GA3057@bwidawsk.net>

Cc: Dave Airlie <airlied@redhat.com>
CC: <dri-devel@lists.freedesktop.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/drm_mm.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Chris Wilson Aug. 10, 2013, 8:48 a.m. UTC | #1
On Fri, Aug 09, 2013 at 10:12:15PM -0700, Ben Widawsky wrote:
> The conditional is usually a recoverable driver bug, and so WARNing, and
> preventing the drm_mm code from doing potential damage (BUG) is
> desirable.
> 
> This issue was hit and fixed twice while developing the i915 multiple
> address space code. The first fix is the patch just before this, and is
> hit on an not frequently occuring error path. Another was fixed during
> patch iteration, so it's hard to see from the patch:
> 
> commit c6cfb325677ea6305fb19acf3a4d14ea267f923e
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Fri Jul 5 14:41:06 2013 -0700
> 
>     drm/i915: Embed drm_mm_node in i915 gem obj
> 
> From the intel-gfx mailing list, we discussed this:
> References: <20130705191235.GA3057@bwidawsk.net>
> 
> Cc: Dave Airlie <airlied@redhat.com>
> CC: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

On the other hand, if we end up with a proliferation of
  if (drm_mm_node_allocated(node))
    drm_mm_remove_node(node)
in the drivers, putting the guard into remove_node is preferrable. At
the moment we only have the one error path in i915.ko where we need to
double check so it is not much of a concern.

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index aded1e1..af93cc5 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -254,6 +254,9 @@  void drm_mm_remove_node(struct drm_mm_node *node)
 	struct drm_mm *mm = node->mm;
 	struct drm_mm_node *prev_node;
 
+	if (WARN_ON(!node->allocated))
+		return;
+
 	BUG_ON(node->scanned_block || node->scanned_prev_free
 				   || node->scanned_next_free);