[PATCH RFC]: DEBUG for PCI IO & MEM allocation

[PATCH RFC]: DEBUG for PCI IO & MEM allocation

[Prarit Bhargava]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

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.



[-- Attachment #2: resource.patch --]
[-- Type: text/x-patch, Size: 2603 bytes --]

===== kernel/resource.c 1.26 vs edited =====
--- 1.26/kernel/resource.c	2005-01-08 00:44:13 -05:00
+++ edited/kernel/resource.c	2005-03-14 17:14:36 -05:00
@@ -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);
 }
 

Re: [PATCH RFC]: DEBUG for PCI IO & MEM allocation

[Andrew Morton]

Prarit Bhargava <prarit@sgi.com> wrote:
>
>  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.

>   
>  +	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.

Re: [PATCH RFC]: DEBUG for PCI IO & MEM allocation

[Prarit Bhargava]

[-- Attachment #1: Type: text/plain, Size: 233 bytes --]

Thanks Andrew.

>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.

[-- Attachment #2: resource2.patch --]
[-- Type: text/plain, Size: 4876 bytes --]

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
diff -u -1 -0 -p -r1.1.1.1 resource.c
--- io_debug/kernel/resource.c	16 Mar 2005 18:48:54 -0000	1.1.1.1
+++ io_debug/kernel/resource.c	21 Mar 2005 20:00:16 -0000
@@ -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)

Re: [PATCH RFC]: DEBUG for PCI IO & MEM allocation

[Andrew Morton]

Prarit Bhargava <prarit@sgi.com> wrote:
>
>  >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.