BUG: drm_mm head_node gets broken?

Commit Message

Russell King - ARM Linux May 19, 2013, 5:22 p.m. UTC

So, while tinkering with my Dove DRM driver, I tripped over a BUG_ON()
in drm_mm_hole_node_start(), which was called from drm_mm_dump_table():

int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
{
        struct drm_mm_node *entry;
        unsigned long total_used = 0, total_free = 0, total = 0;
        unsigned long hole_start, hole_end, hole_size;

        hole_start = drm_mm_hole_node_start(&mm->head_node);
        hole_end = drm_mm_hole_node_end(&mm->head_node);

drm_mm_hole_node_start() does this:

static inline unsigned long drm_mm_hole_node_start(struct drm_mm_node *hole_node)
{
        BUG_ON(!hole_node->hole_follows);
        return __drm_mm_hole_node_start(hole_node);
}

This appears to indicate that it is required that the head node should
always have a "hole" following it.

As this used to work, I dug in the git history, and found commit 6b9d89b4
(drm: Add colouring to the range allocator), in particular this change:

 static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
                                 struct drm_mm_node *node,
-                                unsigned long size, unsigned alignment)
+                                unsigned long size, unsigned alignment,
+                                unsigned long color)
 {
...
-       if (!tmp) {
+       if (alignment) {
+               unsigned tmp = adj_start % alignment;
+               if (tmp)
+                       adj_start += alignment - tmp;
+       }
+
+       if (adj_start == hole_start) {
                hole_node->hole_follows = 0;
-               list_del_init(&hole_node->hole_stack);
-       } else
-               wasted = alignment - tmp;
+               list_del(&hole_node->hole_stack);
+       }


Now, when we do an allocation, it is typically done as follows:

        free = drm_mm_search_free(mm, size, align, 0);
        if (free)
                linear = drm_mm_get_block(free, size, align);

If nothing has been allocated in the mm, 'free' will be &mm->head_node.
The result is that drm_mm_get_block calls down into drm_mm_insert_helper
with hole_node == &mm->head_node.  This I confirmed via:


which triggers during booting.  Hence, we end up setting the head_node
to indicate no hole following - which violates the conditions of other
parts of the code in drm_mm.c.

It looks like this change was made quite a while ago - back in July
2012, so I assume that while the rest of the allocator is fine with this,
it's just the debugging code which is now broken.  However, I suspect
that DRM people may wish to either back out the coloring change or
carefully review the change for correctness throughout the drm_mm
code.

Patch

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index db1e2d6..7241be8 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -125,6 +125,7 @@  static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
 	}
 
 	if (adj_start == hole_start) {
+		BUG_ON(hole_node == &mm->head_node);
 		hole_node->hole_follows = 0;
 		list_del(&hole_node->hole_stack);
 	}