Xen devel - Oct 2009 - [PATCH] linux/usbback: fix usbstub_exit() placement

The function is being referenced from (non-__exit) usbback_init().

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

--- a/drivers/xen/usbback/usbstub.c
+++ b/drivers/xen/usbback/usbstub.c
@@ -317,7 +317,7 @@ out:
 	return err;
 }
 
-void __exit usbstub_exit(void)
+void usbstub_exit(void)
 {
 	driver_remove_file(&usbback_usb_driver.driver,
 				&driver_attr_port_ids);




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

Hi,

Jan Beulich wrote:
 > The function is being referenced from (non-__exit) usbback_init().

No, usbstub_exit() is only referenced from usbback_exit() in the
latest version.

Are you suggesting that usbstub_exit() should be referenced from 
usbback_init() and should do as follows?


Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>

diff -r ba13757d92ce drivers/xen/usbback/usbback.c
--- a/drivers/xen/usbback/usbback.c	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/usbback.c	Fri Oct 16 09:35:28 2009 +0900
@@ -1123,12 +1123,13 @@

  	return 0;

- out_of_memory:
-	 kfree(pending_reqs);
-	 kfree(pending_grant_handles);
-	 free_empty_pages_and_pagevec(pending_pages, mmap_pages);
-	 printk("%s: out of memory\n", __FUNCTION__);
-	 return -ENOMEM;
+out_of_memory:
+	usbstub_exit();
+	kfree(pending_reqs);
+	kfree(pending_grant_handles);
+	free_empty_pages_and_pagevec(pending_pages, mmap_pages);
+	printk("%s: out of memory\n", __FUNCTION__);
+	return -ENOMEM;
  }

  static void __exit usbback_exit(void)
diff -r ba13757d92ce drivers/xen/usbback/usbstub.c
--- a/drivers/xen/usbback/usbstub.c	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/usbstub.c	Fri Oct 16 09:35:28 2009 +0900
@@ -317,7 +317,7 @@
  	return err;
  }

-void __exit usbstub_exit(void)
+void usbstub_exit(void)
  {
  	driver_remove_file(&usbback_usb_driver.driver,
  				&driver_attr_port_ids);




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

>>> Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com> 16.10.09 03:00
>>>
>Jan Beulich wrote:
> > The function is being referenced from (non-__exit) usbback_init().
>
>No, usbstub_exit() is only referenced from usbback_exit() in the
>latest version.
Oops, indeed, I didn''t recall I had to change this for the 2.6.21
forward
merge. I wonder, however, why I didn''t notice the section mismatch
warning earlier...
>Are you suggesting that usbstub_exit() should be referenced from 
>usbback_init() and should do as follows?
Yes, this is what I needed to do there, since I needed to make it return
a value there, as a result of xenbus_register_backend() becoming
__must_check. But really your code, even without the __must_check,
shouldn''t ignore failure from xenbus_register_backend().

Jan


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

Hi,

Jan Beulich wrote:
> Yes, this is what I needed to do there, since I needed to make it return
> a value there, as a result of xenbus_register_backend() becoming
> __must_check. But really your code, even without the __must_check,
> shouldn''t ignore failure from xenbus_register_backend().
I added the checking return value of xenbus_register in usbback_init().
I haven''t tested, but probably ok.

Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>

diff -r ba13757d92ce -r b40bbc96f74c drivers/xen/usbback/usbback.c
--- a/drivers/xen/usbback/usbback.c	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/usbback.c	Fri Oct 16 23:17:17 2009 +0900
@@ -1093,13 +1093,11 @@
  static int __init usbback_init(void)
  {
  	int i, mmap_pages;
+	int err = 0;

  	if (!is_running_on_xen())
  		return -ENODEV;

-	if (usbstub_init())
-		return -ENODEV;
-
  	mmap_pages = usbif_reqs * USBIF_MAX_SEGMENTS_PER_REQUEST;
  	pending_reqs = kmalloc(sizeof(pending_reqs[0]) *
  			usbif_reqs, GFP_KERNEL);
@@ -1107,8 +1105,10 @@
  			mmap_pages, GFP_KERNEL);
  	pending_pages = alloc_empty_pages_and_pagevec(mmap_pages);

-	if (!pending_reqs || !pending_grant_handles || !pending_pages)
-		goto out_of_memory;
+	if (!pending_reqs || !pending_grant_handles || !pending_pages) {
+		err = -ENOMEM;
+		goto out_mem;
+	}

  	for (i = 0; i < mmap_pages; i++)
  		pending_grant_handles[i] = USBBACK_INVALID_HANDLE;
@@ -1119,16 +1119,23 @@
  	for (i = 0; i < usbif_reqs; i++)
  		list_add_tail(&pending_reqs[i].free_list, &pending_free);

-	usbback_xenbus_init();
+	err = usbstub_init();
+	if (err)
+		goto out_mem;
+
+	err = usbback_xenbus_init();
+	if (err)
+		goto out_xenbus;

  	return 0;

- out_of_memory:
-	 kfree(pending_reqs);
-	 kfree(pending_grant_handles);
-	 free_empty_pages_and_pagevec(pending_pages, mmap_pages);
-	 printk("%s: out of memory\n", __FUNCTION__);
-	 return -ENOMEM;
+out_xenbus:
+	usbstub_exit();
+out_mem:
+	kfree(pending_reqs);
+	kfree(pending_grant_handles);
+	free_empty_pages_and_pagevec(pending_pages, mmap_pages);
+	return err;
  }

  static void __exit usbback_exit(void)
diff -r ba13757d92ce -r b40bbc96f74c drivers/xen/usbback/usbback.h
--- a/drivers/xen/usbback/usbback.h	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/usbback.h	Fri Oct 16 23:17:17 2009 +0900
@@ -145,7 +145,7 @@
  	} while (0)

  usbif_t *find_usbif(domid_t domid, unsigned int handle);
-void usbback_xenbus_init(void);
+int usbback_xenbus_init(void);
  void usbback_xenbus_exit(void);
  struct vusb_port_id *find_portid_by_busid(const char *busid);
  struct vusb_port_id *find_portid(const domid_t domid,
diff -r ba13757d92ce -r b40bbc96f74c drivers/xen/usbback/usbstub.c
--- a/drivers/xen/usbback/usbstub.c	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/usbstub.c	Fri Oct 16 23:17:17 2009 +0900
@@ -317,7 +317,7 @@
  	return err;
  }

-void __exit usbstub_exit(void)
+void usbstub_exit(void)
  {
  	driver_remove_file(&usbback_usb_driver.driver,
  				&driver_attr_port_ids);
diff -r ba13757d92ce -r b40bbc96f74c drivers/xen/usbback/xenbus.c
--- a/drivers/xen/usbback/xenbus.c	Thu Oct 08 15:37:22 2009 +0900
+++ b/drivers/xen/usbback/xenbus.c	Fri Oct 16 23:17:17 2009 +0900
@@ -327,12 +327,12 @@
  	.remove = usbback_remove,
  };

-void usbback_xenbus_init(void)
+int __init usbback_xenbus_init(void)
  {
-	xenbus_register_backend(&usbback_driver);
+	return xenbus_register_backend(&usbback_driver);
  }

-void usbback_xenbus_exit(void)
+void __exit usbback_xenbus_exit(void)
  {
  	xenbus_unregister_driver(&usbback_driver);
  }




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

On 16/10/2009 15:42, "Noboru Iwamatsu"
<n_iwamatsu@jp.fujitsu.com> wrote:
> I added the checking return value of xenbus_register in usbback_init().
> I haven''t tested, but probably ok.
> 
> Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
This patch didn''t apply - probably mangled. Please try sending as an
attachment.

 -- Keir



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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 19.10.09 11:52
>>>
>On 16/10/2009 15:42, "Noboru Iwamatsu"
<n_iwamatsu@jp.fujitsu.com> wrote:
>
>> I added the checking return value of xenbus_register in usbback_init().
>> I haven''t tested, but probably ok.
>> 
>> Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
>
>This patch didn''t apply - probably mangled. Please try sending as
an
>attachment.
Here it is (didn''t see Noboru ever resend it).

Jan



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

Thanks,
Noboru.

Jan Beulich wrote:
>>>> Keir Fraser <keir.fraser@eu.citrix.com> 19.10.09 11:52
>>>
>> On 16/10/2009 15:42, "Noboru Iwamatsu"
<n_iwamatsu@jp.fujitsu.com> wrote:
>>
>>> I added the checking return value of xenbus_register in
usbback_init().
>>> I haven''t tested, but probably ok.
>>>
>>> Signed-off-by: Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
>> This patch didn''t apply - probably mangled. Please try sending
as an
>> attachment.
> 
> Here it is (didn''t see Noboru ever resend it).
> 
> Jan



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