thr3ads.net - Xen devel - [Xen-devel] [PATCH] pciback: check function return values [Mar 2007]

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2007-Mar-28 12:35 UTC

[Xen-devel] [PATCH] pciback: check function return values

.. of functions declared with __must_check post-2.6.18.

(As a note - I''m getting the impression that when used as a module,
pciback
appears to have a number of memory leaks).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

Index: head-2007-03-19/drivers/xen/pciback/conf_space_header.c
==================================================================---
head-2007-03-19.orig/drivers/xen/pciback/conf_space_header.c	2007-03-21
10:18:08.000000000 +0100
+++ head-2007-03-19/drivers/xen/pciback/conf_space_header.c	2007-03-28
11:50:57.000000000 +0200
@@ -20,11 +20,15 @@ struct pci_bar_info {
 
 static int command_write(struct pci_dev *dev, int offset, u16 value, void
*data)
 {
+	int err;
+
 	if (!dev->is_enabled && is_enable_cmd(value)) {
 		if (unlikely(verbose_request))
 			printk(KERN_DEBUG "pciback: %s: enable\n",
 			       pci_name(dev));
-		pci_enable_device(dev);
+		err = pci_enable_device(dev);
+		if (err)
+			return err;
 	} else if (dev->is_enabled && !is_enable_cmd(value)) {
 		if (unlikely(verbose_request))
 			printk(KERN_DEBUG "pciback: %s: disable\n",
@@ -44,7 +48,13 @@ static int command_write(struct pci_dev 
 			printk(KERN_DEBUG
 			       "pciback: %s: enable memory-write-invalidate\n",
 			       pci_name(dev));
-		pci_set_mwi(dev);
+		err = pci_set_mwi(dev);
+		if (err) {
+			printk(KERN_WARNING
+			       "pciback: %s: cannot enable memory-write-invalidate
(%d)\n",
+			       pci_name(dev), err);
+			value &= ~PCI_COMMAND_INVALIDATE;
+		}
 	}
 
 	return pci_write_config_word(dev, offset, value);
Index: head-2007-03-19/drivers/xen/pciback/pci_stub.c
==================================================================---
head-2007-03-19.orig/drivers/xen/pciback/pci_stub.c	2007-03-19
15:26:06.000000000 +0100
+++ head-2007-03-19/drivers/xen/pciback/pci_stub.c	2007-03-28 12:50:30.000000000
+0200
@@ -805,6 +805,18 @@ static ssize_t permissive_show(struct de
 
 DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, permissive_add);
 
+static void pcistub_exit(void)
+{
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
+	driver_remove_file(&pciback_pci_driver.driver,
+			   &driver_attr_remove_slot);
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_slots);
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_quirks);
+	driver_remove_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
+
+	pci_unregister_driver(&pciback_pci_driver);
+}
+
 static int __init pcistub_init(void)
 {
 	int pos = 0;
@@ -845,12 +857,23 @@ static int __init pcistub_init(void)
 	if (err < 0)
 		goto out;
 
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-	driver_create_file(&pciback_pci_driver.driver,
-			   &driver_attr_remove_slot);
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_slots);
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_quirks);
-	driver_create_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
+	err = driver_create_file(&pciback_pci_driver.driver,
+				 &driver_attr_new_slot);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_remove_slot);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_slots);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_quirks);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_permissive);
+
+	if (err)
+		pcistub_exit();
 
       out:
 	return err;
@@ -887,23 +910,17 @@ static int __init pciback_init(void)
 #endif
 
 	pcistub_init_devices_late();
-	pciback_xenbus_register();
+	err = pciback_xenbus_register();
+	if (err)
+		pciback_exit();
 
-	return 0;
+	return err;
 }
 
 static void __exit pciback_cleanup(void)
 {
 	pciback_xenbus_unregister();
-
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-	driver_remove_file(&pciback_pci_driver.driver,
-			   &driver_attr_remove_slot);
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_slots);
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_quirks);
-	driver_remove_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
-
-	pci_unregister_driver(&pciback_pci_driver);
+	pciback_exit();
 }
 
 module_init(pciback_init);



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2007-Mar-28 13:41 UTC

head link

Re: [Xen-devel] [PATCH] pciback: check function return values

On 28/3/07 13:35, "Jan Beulich" <jbeulich@novell.com> wrote:
> .. of functions declared with __must_check post-2.6.18.
> 
> (As a note - I''m getting the impression that when used as a
module, pciback
> appears to have a number of memory leaks).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
Where is the mystery pciback_exit() function defined? You use it but
don''t
provide an implementation.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2007-Mar-28 13:53 UTC

head link

Re: [Xen-devel] [PATCH] pciback: check function return values

>>> Keir Fraser <keir@xensource.com> 28.03.07 15:41 >>>
>On 28/3/07 13:35, "Jan Beulich" <jbeulich@novell.com> wrote:
>
>> .. of functions declared with __must_check post-2.6.18.
>> 
>> (As a note - I''m getting the impression that when used as a
module, pciback
>> appears to have a number of memory leaks).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
>Where is the mystery pciback_exit() function defined? You use it but
don''t
>provide an implementation.
Huh, sorry, unrefreshed patch. Thanks for catching this. Here''s to
proper one.

Index: head-2007-03-19/drivers/xen/pciback/conf_space_header.c
==================================================================---
head-2007-03-19.orig/drivers/xen/pciback/conf_space_header.c	2007-03-21
10:18:08.000000000 +0100
+++ head-2007-03-19/drivers/xen/pciback/conf_space_header.c	2007-03-28
11:50:57.000000000 +0200
@@ -20,11 +20,15 @@ struct pci_bar_info {
 
 static int command_write(struct pci_dev *dev, int offset, u16 value, void
*data)
 {
+	int err;
+
 	if (!dev->is_enabled && is_enable_cmd(value)) {
 		if (unlikely(verbose_request))
 			printk(KERN_DEBUG "pciback: %s: enable\n",
 			       pci_name(dev));
-		pci_enable_device(dev);
+		err = pci_enable_device(dev);
+		if (err)
+			return err;
 	} else if (dev->is_enabled && !is_enable_cmd(value)) {
 		if (unlikely(verbose_request))
 			printk(KERN_DEBUG "pciback: %s: disable\n",
@@ -44,7 +48,13 @@ static int command_write(struct pci_dev 
 			printk(KERN_DEBUG
 			       "pciback: %s: enable memory-write-invalidate\n",
 			       pci_name(dev));
-		pci_set_mwi(dev);
+		err = pci_set_mwi(dev);
+		if (err) {
+			printk(KERN_WARNING
+			       "pciback: %s: cannot enable memory-write-invalidate
(%d)\n",
+			       pci_name(dev), err);
+			value &= ~PCI_COMMAND_INVALIDATE;
+		}
 	}
 
 	return pci_write_config_word(dev, offset, value);
Index: head-2007-03-19/drivers/xen/pciback/pci_stub.c
==================================================================---
head-2007-03-19.orig/drivers/xen/pciback/pci_stub.c	2007-03-19
15:26:06.000000000 +0100
+++ head-2007-03-19/drivers/xen/pciback/pci_stub.c	2007-03-28 13:34:33.000000000
+0200
@@ -805,6 +805,18 @@ static ssize_t permissive_show(struct de
 
 DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, permissive_add);
 
+static void pcistub_exit(void)
+{
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
+	driver_remove_file(&pciback_pci_driver.driver,
+			   &driver_attr_remove_slot);
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_slots);
+	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_quirks);
+	driver_remove_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
+
+	pci_unregister_driver(&pciback_pci_driver);
+}
+
 static int __init pcistub_init(void)
 {
 	int pos = 0;
@@ -845,12 +857,23 @@ static int __init pcistub_init(void)
 	if (err < 0)
 		goto out;
 
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-	driver_create_file(&pciback_pci_driver.driver,
-			   &driver_attr_remove_slot);
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_slots);
-	driver_create_file(&pciback_pci_driver.driver, &driver_attr_quirks);
-	driver_create_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
+	err = driver_create_file(&pciback_pci_driver.driver,
+				 &driver_attr_new_slot);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_remove_slot);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_slots);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_quirks);
+	if (!err)
+		err = driver_create_file(&pciback_pci_driver.driver,
+					 &driver_attr_permissive);
+
+	if (err)
+		pcistub_exit();
 
       out:
 	return err;
@@ -887,23 +910,17 @@ static int __init pciback_init(void)
 #endif
 
 	pcistub_init_devices_late();
-	pciback_xenbus_register();
+	err = pciback_xenbus_register();
+	if (err)
+		pcistub_exit();
 
-	return 0;
+	return err;
 }
 
 static void __exit pciback_cleanup(void)
 {
 	pciback_xenbus_unregister();
-
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-	driver_remove_file(&pciback_pci_driver.driver,
-			   &driver_attr_remove_slot);
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_slots);
-	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_quirks);
-	driver_remove_file(&pciback_pci_driver.driver,
&driver_attr_permissive);
-
-	pci_unregister_driver(&pciback_pci_driver);
+	pcistub_exit();
 }
 
 module_init(pciback_init);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Chris Bookholt

2007-Apr-03 15:56 UTC

head link

Re: [Xen-devel] [PATCH] pciback: check function return values

Jan Beulich wrote:> .. of functions declared with __must_check post-2.6.18.
> 
> (As a note - I''m getting the impression that when used as a
module, pciback
> appears to have a number of memory leaks).
Jan, thanks for the added robustness by way of return value checking.

What gives you the impression there are memory leaks when pciback is
loaded as a module?  The more specific you can be the easier problems
are to find and fix.

Cheers,
Chris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2007-Apr-04 07:53 UTC

head link

Re: [Xen-devel] [PATCH] pciback: check function return values

>>> Chris Bookholt <cgbook2@tycho.ncsc.mil> 03.04.07 17:56
>>>
>Jan Beulich wrote:
>> .. of functions declared with __must_check post-2.6.18.
>> 
>> (As a note - I''m getting the impression that when used as a
module, pciback
>> appears to have a number of memory leaks).
>
>Jan, thanks for the added robustness by way of return value checking.
>
>What gives you the impression there are memory leaks when pciback is
>loaded as a module?  The more specific you can be the easier problems
>are to find and fix.
First thing pcistub_init() does is parse the hide option, calling
pcistub_device_id_add(). If subsequent steps fail, or if the driver is unloaded,
this isn''t being cleaned up. There are others that look suspicious, but
this is
certainly easier to determine for someone more familiar with the code than
I am.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Mar 2007 - [PATCH] pciback: check function return values

[Xen-devel] [PATCH] pciback: check function return values

Re: [Xen-devel] [PATCH] pciback: check function return values

Re: [Xen-devel] [PATCH] pciback: check function return values

Re: [Xen-devel] [PATCH] pciback: check function return values

Re: [Xen-devel] [PATCH] pciback: check function return values