Skip to contentSkip to conversation

: DEBUG for PCI IO & MEM allocation

RFC PATCH In review Prarit Bhargava 2 participants 4 messages Mar 14, 2005 – Mar 22, 2005

lore
Share & tools
lore

Prarit Bhargava

author
#0 lore raw

RFC PATCH 1/1 : DEBUG for PCI IO & MEM allocation

Colleagues,

Over the past few years I've been heavily involved in two projects that deal with PCI HotPlug. While doing this work, one area of code always seems to require printk's to debug through -- the allocation & freeing of IO & MEM resources.

I've discovered many bugs surrounding Hotplug PCI IO & MEM allocations that would have been much easier to debug had there been printk's existing within the code, including one recently which impacts all of PCI Hotplug and appears to have been around since pre-2.6.9 (a patch to fix this issue has already been reviewed & accepted by Greg Kroah).

As Hotplug continues to mature and more Hotplug drivers are introduced, I suspect that more and more bugs will be introduced in the resource space.

I propose the following patch to add a compile time DEBUG option to kernel/resource.c that would help in analyzing problems in this area. It's a few simple lines of output in __request_resource, __release_resource, __request_region, and __release_region .

Thanks,

P.

===== kernel/resource.c 1.26 vs edited =====

kernel/resource.c +20 -1
@@ -20,6 +20,11 @@
 #include <linux/seq_file.h>
 #include <asm/io.h>
 
+#if 0
+#define DEBUGP printk
+#else
+#define DEBUGP(fmt , a...)
+#endif
 
 struct resource ioport_resource = {
 	.name	= "PCI IO",

@@ -155,6 +160,8 @@
 	unsigned long end = new->end;
 	struct resource *tmp, **p;
 
+	DEBUGP("%s: resource request at 0x%lx-0x%lx\n", __FUNCTION__, new->start, new->end);
+
 	if (end < start)
 		return root;
 	if (start < root->start)

@@ -168,11 +175,13 @@
 			new->sibling = tmp;
 			*p = new;
 			new->parent = root;
+			DEBUGP("%s: resource allocated\n", __FUNCTION__);
 			return NULL;
 		}
 		p = &tmp->sibling;
 		if (tmp->end < start)
 			continue;
+		DEBUGP("%s: resource conflicted with 0x%lx-0x%lx\n", __FUNCTION__, tmp->start, tmp->end);
 		return tmp;
 	}
 }

@@ -181,6 +190,7 @@
 {
 	struct resource *tmp, **p;
 
+	DEBUGP("%s: resource release for 0x%lx-0x%lx\n", __FUNCTION__, old->start, old->end);
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

@@ -189,10 +199,12 @@
 		if (tmp == old) {
 			*p = tmp->sibling;
 			old->parent = NULL;
+			DEBUGP("%s: resource free'd\n", __FUNCTION__);
 			return 0;
 		}
 		p = &tmp->sibling;
 	}
+	DEBUGP("%s: resource cannot be released\n", __FUNCTION__);
 	return -EINVAL;
 }
 

@@ -432,6 +444,7 @@
 {
 	struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
 
+	DEBUGP("%s: requesting region 0x%lx - 0x%lx\n", __FUNCTION__, start, start + n - 1);
 	if (res) {
 		memset(res, 0, sizeof(*res));
 		res->name = name;

@@ -445,8 +458,10 @@
 			struct resource *conflict;
 
 			conflict = __request_resource(parent, res);
-			if (!conflict)
+			if (!conflict) {
+				DEBUGP("%s: region assigned\n", __FUNCTION__);
 				break;
+			}
 			if (conflict != parent) {
 				parent = conflict;
 				if (!(conflict->flags & IORESOURCE_BUSY))

@@ -454,6 +469,8 @@
 			}
 
 			/* Uhhuh, that didn't work out.. */
+			DEBUGP("%s: request for region 0x%lx - 0x%lx failed\n", __FUNCTION__, res->start, 
+				res->end);
 			kfree(res);
 			res = NULL;
 			break;

@@ -504,6 +521,7 @@
 				break;
 			*p = res->sibling;
 			write_unlock(&resource_lock);
+			DEBUGP("%s: releasing region 0x%lx - 0x%lx\n", __FUNCTION__, res->start, res->end);
 			kfree(res);
 			return;
 		}

@@ -512,6 +530,7 @@
 
 	write_unlock(&resource_lock);
 
+	DEBUGP("%s: release regions  0x%lx - 0x%lx failed\n", __FUNCTION__, start, end);
 	printk(KERN_WARNING "Trying to free nonexistent resource <%08lx-%08lx>\n", start, end);
 }
#1 lore raw
Quoted 6 lines · Prarit Bhargava

I propose the following patch to add a compile time DEBUG option to kernel/resource.c that would help in analyzing problems in this area. It's a few simple lines of output in __request_resource, __release_resource, __request_region, and __release_region .

A sane enough requirement.

Quoted 2 lines

+ DEBUGP("%s: resource request at 0x%lx-0x%lx\n", __FUNCTION__, new->start, new->end);

Shouldn't this also be printing the ->name of the new resource?

A lot of the statements which you're adding will look screwy in an 80-col xterm. Please wrap 'em.

author
#2 lore raw

Thanks Andrew.

Quoted 5 lines

Shouldn't this also be printing the ->name of the new resource?

A lot of the statements which you're adding will look screwy in an 80-col xterm. Please wrap 'em.

-- new patch with Andrew's comments fixed.

P.

Index: io_debug/kernel/resource.c
===================================================================
RCS file: /usr/local/src/cvsroot/bk/linux-2.5/kernel/resource.c,v retrieving revision 1.1.1.1

kernel/resource.c +30 -1
diff -u -1 -0 -p -r1.1.1.1 resource.c
@@ -13,20 +13,25 @@
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/fs.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <asm/io.h>
 
+#if 0
+#define DEBUGP printk
+#else
+#define DEBUGP(fmt , a...)
+#endif
 
 struct resource ioport_resource = {
 	.name	= "PCI IO",
 	.start	= 0x0000,
 	.end	= IO_SPACE_LIMIT,
 	.flags	= IORESOURCE_IO,
 };
 
 EXPORT_SYMBOL(ioport_resource);
 

@@ -148,58 +153,70 @@ __initcall(ioresources_init);
 
 #endif /* CONFIG_PROC_FS */
 
 /* Return the conflict entry if you can't request it */
 static struct resource * __request_resource(struct resource *root, struct resource *new)
 {
 	unsigned long start = new->start;
 	unsigned long end = new->end;
 	struct resource *tmp, **p;
 
+	DEBUGP("%s: %s resource request at 0x%lx-0x%lx\n", __FUNCTION__, 
+	       new->name, new->start, new->end);
+
 	if (end < start)
 		return root;
 	if (start < root->start)
 		return root;
 	if (end > root->end)
 		return root;
 	p = &root->child;
 	for (;;) {
 		tmp = *p;
 		if (!tmp || tmp->start > end) {
 			new->sibling = tmp;
 			*p = new;
 			new->parent = root;
+			DEBUGP("%s: %s resource allocated\n", __FUNCTION__, 
+			       new->name);
 			return NULL;
 		}
 		p = &tmp->sibling;
 		if (tmp->end < start)
 			continue;
+		DEBUGP("%s: %s resource conflicted with 0x%lx-0x%lx\n", 
+		       __FUNCTION__, new->name, tmp->start, tmp->end);
 		return tmp;
 	}
 }
 
 static int __release_resource(struct resource *old)
 {
 	struct resource *tmp, **p;
 
+	DEBUGP("%s: %s resource release for 0x%lx-0x%lx\n", __FUNCTION__, 
+	       old->name, old->start, old->end);
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;
 		if (!tmp)
 			break;
 		if (tmp == old) {
 			*p = tmp->sibling;
 			old->parent = NULL;
+			DEBUGP("%s: %s resource released\n", __FUNCTION__, 
+			       old->name);
 			return 0;
 		}
 		p = &tmp->sibling;
 	}
+	DEBUGP("%s: %s resource cannot be released\n", __FUNCTION__, old->name);
 	return -EINVAL;
 }
 
 int request_resource(struct resource *root, struct resource *new)
 {
 	struct resource *conflict;
 
 	write_lock(&resource_lock);
 	conflict = __request_resource(root, new);
 	write_unlock(&resource_lock);

@@ -425,42 +442,49 @@ EXPORT_SYMBOL(adjust_resource);
  * Request-region creates a new busy region.
  *
  * Check-region returns non-zero if the area is already busy
  *
  * Release-region releases a matching busy region.
  */
 struct resource * __request_region(struct resource *parent, unsigned long start, unsigned long n, const char *name)
 {
 	struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
 
+	DEBUGP("%s: %s requesting region 0x%lx - 0x%lx\n", __FUNCTION__, 
+	       name, start, start + n - 1);
 	if (res) {
 		memset(res, 0, sizeof(*res));
 		res->name = name;
 		res->start = start;
 		res->end = start + n - 1;
 		res->flags = IORESOURCE_BUSY;
 
 		write_lock(&resource_lock);
 
 		for (;;) {
 			struct resource *conflict;
 
 			conflict = __request_resource(parent, res);
-			if (!conflict)
+			if (!conflict) {
+				DEBUGP("%s: %s region assigned\n", __FUNCTION__,
+				       name);
 				break;
+			}
 			if (conflict != parent) {
 				parent = conflict;
 				if (!(conflict->flags & IORESOURCE_BUSY))
 					continue;
 			}
 
 			/* Uhhuh, that didn't work out.. */
+			DEBUGP("%s: %s request for region 0x%lx - 0x%lx fail\n",
+			       __FUNCTION__, res->name, res->start, res->end);
 			kfree(res);
 			res = NULL;
 			break;
 		}
 		write_unlock(&resource_lock);
 	}
 	return res;
 }
 
 EXPORT_SYMBOL(__request_region);

@@ -497,28 +521,33 @@ void __release_region(struct resource *p
 			break;
 		if (res->start <= start && res->end >= end) {
 			if (!(res->flags & IORESOURCE_BUSY)) {
 				p = &res->child;
 				continue;
 			}
 			if (res->start != start || res->end != end)
 				break;
 			*p = res->sibling;
 			write_unlock(&resource_lock);
+			DEBUGP("%s: %s releasing region 0x%lx - 0x%lx\n", 
+			       __FUNCTION__, res->name, res->start, res->end);
 			kfree(res);
 			return;
 		}
 		p = &res->sibling;
 	}
 
 	write_unlock(&resource_lock);
 
+	DEBUGP("%s: release regions  0x%lx - 0x%lx failed\n", __FUNCTION__,
+	       start, end);
+
 	printk(KERN_WARNING "Trying to free nonexistent resource <%08lx-%08lx>\n", start, end);
 }
 
 EXPORT_SYMBOL(__release_region);
 
 /*
  * Called from init/main.c to reserve IO ports.
  */
 #define MAXRESERVE 4
 static int __init reserve_setup(char *str)
#3 lore raw
Quoted 7 lines · Prarit Bhargava
Quoted 6 lines

Shouldn't this also be printing the ->name of the new resource?

A lot of the statements which you're adding will look screwy in an 80-col xterm. Please wrap 'em.

-- new patch with Andrew's comments fixed.

OK, but I've forgotten the rationale for this change - please send through a little changelog entry and a Signed-off-by: line and I'll push this in Greg's direction.

Thanks.