[PATCH] USB Elan FTDI: check for driver registration status

[PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

This patch adds checking of driver registration status
and if it fails release allocated resources.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

---

Pete, please review the patch and Ack it then.

 drivers/usb/misc/ftdi-elan.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
index bc3327e..3cd9ae3 100644
--- a/drivers/usb/misc/ftdi-elan.c
+++ b/drivers/usb/misc/ftdi-elan.c
@@ -2909,27 +2909,36 @@ static int __init ftdi_elan_init(void)
         init_MUTEX(&ftdi_module_lock);
         INIT_LIST_HEAD(&ftdi_static_list);
         status_queue = create_singlethread_workqueue("ftdi-status-control");
-	if (!status_queue)
-		goto err1;
         command_queue = create_singlethread_workqueue("ftdi-command-engine");
-	if (!command_queue)
-		goto err2;
         respond_queue = create_singlethread_workqueue("ftdi-respond-engine");
-	if (!respond_queue)
-		goto err3;
+	if (!status_queue || !command_queue || !respond_queue) {
+		printk(KERN_ERR "%s couldn't create workqueue\n",
+		       ftdi_elan_driver.name);
+		result = -ENOMEM;
+		goto err;
+	}
         result = usb_register(&ftdi_elan_driver);
-        if (result)
+        if (result) {
                 printk(KERN_ERR "usb_register failed. Error number %d\n",
 		       result);
+		goto err;
+	}
         return result;
 
- err3:
-	destroy_workqueue(command_queue);
- err2:
-	destroy_workqueue(status_queue);
- err1:
-	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
-	return -ENOMEM;
+ err:
+	if (status_queue) {
+		destroy_workqueue(status_queue);
+		status_queue = NULL;
+	}
+	if (command_queue) {
+		destroy_workqueue(command_queue);
+		command_queue = NULL;
+	}
+	if (respond_queue) {
+		destroy_workqueue(respond_queue);
+		respond_queue = NULL;
+	}
+	return result;
 }
 
 static void __exit ftdi_elan_exit(void)

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Luiz Fernando N. Capitulino]

 Hi Cyrill,

Em Sun, 25 Mar 2007 11:27:33 +0400
Cyrill Gorcunov <gorcunov@gmail.com> escreveu:

| This patch adds checking of driver registration status
| and if it fails release allocated resources.
| 
| Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| 
| ---
| 
| Pete, please review the patch and Ack it then.
| 
|  drivers/usb/misc/ftdi-elan.c |   37 +++++++++++++++++++++++--------------
|  1 files changed, 23 insertions(+), 14 deletions(-)
| 
| diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| index bc3327e..3cd9ae3 100644
| --- a/drivers/usb/misc/ftdi-elan.c
| +++ b/drivers/usb/misc/ftdi-elan.c
| @@ -2909,27 +2909,36 @@ static int __init ftdi_elan_init(void)
|          init_MUTEX(&ftdi_module_lock);
|          INIT_LIST_HEAD(&ftdi_static_list);
|          status_queue = create_singlethread_workqueue("ftdi-status-control");
| -	if (!status_queue)
| -		goto err1;
|          command_queue = create_singlethread_workqueue("ftdi-command-engine");
| -	if (!command_queue)
| -		goto err2;
|          respond_queue = create_singlethread_workqueue("ftdi-respond-engine");
| -	if (!respond_queue)
| -		goto err3;
| +	if (!status_queue || !command_queue || !respond_queue) {
| +		printk(KERN_ERR "%s couldn't create workqueue\n",
| +		       ftdi_elan_driver.name);
| +		result = -ENOMEM;
| +		goto err;
| +	}

 The current version looks ok to me, why are you changing it?

 It's also smater, ie, if the first workqueue creation fails, it
won't try to create the others.

|          result = usb_register(&ftdi_elan_driver);
| -        if (result)
| +        if (result) {
|                  printk(KERN_ERR "usb_register failed. Error number %d\n",
|  		       result);
| +		goto err;
| +	}
|          return result;

 This could be:

"""
if (result) {
	printk(KERN_ERR "usb_register failed. Error number %d\n",
	    result);
	destroy_workqueue(respond_queue);
	goto err3;
}
"""

 And you can change 'err1' to return 'result'.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

[Luiz Fernando N. Capitulino - Mon, Mar 26, 2007 at 03:43:57PM -0300]
| 
|  Hi Cyrill,
| 
| Em Sun, 25 Mar 2007 11:27:33 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> escreveu:
| 
| | This patch adds checking of driver registration status
| | and if it fails release allocated resources.
| | 
| | Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| | 
| | ---
| | 
| | Pete, please review the patch and Ack it then.
| | 
| |  drivers/usb/misc/ftdi-elan.c |   37 +++++++++++++++++++++++--------------
| |  1 files changed, 23 insertions(+), 14 deletions(-)
| | 
| | diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| | index bc3327e..3cd9ae3 100644
| | --- a/drivers/usb/misc/ftdi-elan.c
| | +++ b/drivers/usb/misc/ftdi-elan.c
| | @@ -2909,27 +2909,36 @@ static int __init ftdi_elan_init(void)
| |          init_MUTEX(&ftdi_module_lock);
| |          INIT_LIST_HEAD(&ftdi_static_list);
| |          status_queue = create_singlethread_workqueue("ftdi-status-control");
| | -	if (!status_queue)
| | -		goto err1;
| |          command_queue = create_singlethread_workqueue("ftdi-command-engine");
| | -	if (!command_queue)
| | -		goto err2;
| |          respond_queue = create_singlethread_workqueue("ftdi-respond-engine");
| | -	if (!respond_queue)
| | -		goto err3;
| | +	if (!status_queue || !command_queue || !respond_queue) {
| | +		printk(KERN_ERR "%s couldn't create workqueue\n",
| | +		       ftdi_elan_driver.name);
| | +		result = -ENOMEM;
| | +		goto err;
| | +	}
| 
|  The current version looks ok to me, why are you changing it?
| 
|  It's also smater, ie, if the first workqueue creation fails, it
| won't try to create the others.
| 
| |          result = usb_register(&ftdi_elan_driver);
| | -        if (result)
| | +        if (result) {
| |                  printk(KERN_ERR "usb_register failed. Error number %d\n",
| |  		       result);
| | +		goto err;
| | +	}
| |          return result;
| 
|  This could be:
| 
| """
| if (result) {
| 	printk(KERN_ERR "usb_register failed. Error number %d\n",
| 	    result);
| 	destroy_workqueue(respond_queue);
| 	goto err3;
| }
| """
| 
|  And you can change 'err1' to return 'result'.
| 
| -- 
| Luiz Fernando N. Capitulino
| 

Hi Luiz,

if usb registration failed we should release all worqueues we've
created and that is the reason why I've changed the code...

but anyway I have to find more elegant solution I think :)

		Cyrill

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Luiz Fernando N. Capitulino]

Em Mon, 26 Mar 2007 23:33:12 +0400
Cyrill Gorcunov <gorcunov@gmail.com> escreveu:

| if usb registration failed we should release all worqueues we've
| created and that is the reason why I've changed the code...

 I see, maybe something like the following (not tested):

diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
index bc3327e..cf95ae8 100644
--- a/drivers/usb/misc/ftdi-elan.c
+++ b/drivers/usb/misc/ftdi-elan.c
@@ -2903,7 +2903,8 @@ static struct usb_driver ftdi_elan_drive
 };
 static int __init ftdi_elan_init(void)
 {
-        int result;
+        int result = -ENOMEM;
+
         printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
 	       __TIME__, __DATE__);
         init_MUTEX(&ftdi_module_lock);
@@ -2918,9 +2919,13 @@ static int __init ftdi_elan_init(void)
 	if (!respond_queue)
 		goto err3;
         result = usb_register(&ftdi_elan_driver);
-        if (result)
+        if (result) {
                 printk(KERN_ERR "usb_register failed. Error number %d\n",
 		       result);
+		destroy_workqueue(respond_queue);
+		goto err3;
+	}
+
         return result;
 
  err3:
@@ -2929,7 +2934,7 @@ static int __init ftdi_elan_init(void)
 	destroy_workqueue(status_queue);
  err1:
 	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
-	return -ENOMEM;
+	return result;
 }
 
 static void __exit ftdi_elan_exit(void)

PS: Please, don't remove people from the CC list (wait for them
to ask for that).

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Pete Zaitcev]

On Sun, 25 Mar 2007 11:27:33 +0400, Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> This patch adds checking of driver registration status
> and if it fails release allocated resources.

> +	if (status_queue) {
> +		destroy_workqueue(status_queue);
> +		status_queue = NULL;
> +	}
> +	if (command_queue) {
> +		destroy_workqueue(command_queue);
> +		command_queue = NULL;
> +	}
> +	if (respond_queue) {
> +		destroy_workqueue(respond_queue);
> +		respond_queue = NULL;
> +	}
> +	return result;

I hate this style with passion, but I don't have cycles to argue.
So, whatever works for you and Greg is fine with me.

-- Pete

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

Pete, Luiz

what about this one?

Actually there is just a check for where is error coming from.
Maybe that is not the best solution but it allows us to reduce
the calls to 'printk' :)

P.S. Pete your patch is good but the message about
worqueue creation fail was to print even if we've
been faltered on the usb_register procedure.

---

 drivers/usb/misc/ftdi-elan.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
index bc3327e..3cd54af 100644
--- a/drivers/usb/misc/ftdi-elan.c
+++ b/drivers/usb/misc/ftdi-elan.c
@@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
 };
 static int __init ftdi_elan_init(void)
 {
-        int result;
+        int result = 0;
         printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
 	       __TIME__, __DATE__);
         init_MUTEX(&ftdi_module_lock);
@@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
 	if (!respond_queue)
 		goto err3;
         result = usb_register(&ftdi_elan_driver);
-        if (result)
+        if (result) {
                 printk(KERN_ERR "usb_register failed. Error number %d\n",
 		       result);
+		goto err4;
+	}
         return result;
 
+ err4:
+	destroy_workqueue(respond_queue);
  err3:
 	destroy_workqueue(command_queue);
  err2:
 	destroy_workqueue(status_queue);
  err1:
-	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
-	return -ENOMEM;
+	if (result == 0) {
+		result = -ENOMEM;
+		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
+	}
+	return result;
 }
 
 static void __exit ftdi_elan_exit(void)


		Cyrill

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Luiz Fernando N. Capitulino]

Em Tue, 27 Mar 2007 19:14:05 +0400
Cyrill Gorcunov <gorcunov@gmail.com> escreveu:

| Pete, Luiz
| 
| what about this one?
| 
| Actually there is just a check for where is error coming from.
| Maybe that is not the best solution but it allows us to reduce
| the calls to 'printk' :)
| 
| P.S. Pete your patch is good but the message about
| worqueue creation fail was to print even if we've
| been faltered on the usb_register procedure.
| 
| ---
| 
|  drivers/usb/misc/ftdi-elan.c |   15 +++++++++++----
|  1 files changed, 11 insertions(+), 4 deletions(-)
| 
| diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| index bc3327e..3cd54af 100644
| --- a/drivers/usb/misc/ftdi-elan.c
| +++ b/drivers/usb/misc/ftdi-elan.c
| @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
|  };
|  static int __init ftdi_elan_init(void)
|  {
| -        int result;
| +        int result = 0;
|          printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
|  	       __TIME__, __DATE__);
|          init_MUTEX(&ftdi_module_lock);
| @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
|  	if (!respond_queue)
|  		goto err3;
|          result = usb_register(&ftdi_elan_driver);
| -        if (result)
| +        if (result) {
|                  printk(KERN_ERR "usb_register failed. Error number %d\n",
|  		       result);
| +		goto err4;
| +	}
|          return result;
|  
| + err4:
| +	destroy_workqueue(respond_queue);
|   err3:
|  	destroy_workqueue(command_queue);
|   err2:
|  	destroy_workqueue(status_queue);
|   err1:
| -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| -	return -ENOMEM;
| +	if (result == 0) {
| +		result = -ENOMEM;
| +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| +	}
| +	return result;
|  }

 I still the prefer the version I sent you yesterday. :) It
changes the minimal amount of code.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

[Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 01:28:39PM -0300]
| Em Tue, 27 Mar 2007 19:14:05 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> escreveu:
| 
| | Pete, Luiz
| | 
| | what about this one?
| | 
| | Actually there is just a check for where is error coming from.
| | Maybe that is not the best solution but it allows us to reduce
| | the calls to 'printk' :)
| | 
| | P.S. Pete your patch is good but the message about
| | worqueue creation fail was to print even if we've
| | been faltered on the usb_register procedure.
| | 
| | ---
| | 
| |  drivers/usb/misc/ftdi-elan.c |   15 +++++++++++----
| |  1 files changed, 11 insertions(+), 4 deletions(-)
| | 
| | diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| | index bc3327e..3cd54af 100644
| | --- a/drivers/usb/misc/ftdi-elan.c
| | +++ b/drivers/usb/misc/ftdi-elan.c
| | @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
| |  };
| |  static int __init ftdi_elan_init(void)
| |  {
| | -        int result;
| | +        int result = 0;
| |          printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
| |  	       __TIME__, __DATE__);
| |          init_MUTEX(&ftdi_module_lock);
| | @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
| |  	if (!respond_queue)
| |  		goto err3;
| |          result = usb_register(&ftdi_elan_driver);
| | -        if (result)
| | +        if (result) {
| |                  printk(KERN_ERR "usb_register failed. Error number %d\n",
| |  		       result);
| | +		goto err4;
| | +	}
| |          return result;
| |  
| | + err4:
| | +	destroy_workqueue(respond_queue);
| |   err3:
| |  	destroy_workqueue(command_queue);
| |   err2:
| |  	destroy_workqueue(status_queue);
| |   err1:
| | -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | -	return -ENOMEM;
| | +	if (result == 0) {
| | +		result = -ENOMEM;
| | +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | +	}
| | +	return result;
| |  }
| 
|  I still the prefer the version I sent you yesterday. :) It
| changes the minimal amount of code.
| 
| -- 
| Luiz Fernando N. Capitulino
| 

Liuz,

but the patch you sent me does call

	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);

even if error occured in usb_register and that is not good I think.
Fix me if I'm wrong.

		Cyrill

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Luiz Fernando N. Capitulino]

Em Tue, 27 Mar 2007 21:01:25 +0400
Cyrill Gorcunov <gorcunov@gmail.com> escreveu:

| [Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 01:28:39PM -0300]
| | Em Tue, 27 Mar 2007 19:14:05 +0400
| | Cyrill Gorcunov <gorcunov@gmail.com> escreveu:
| | 
| | | Pete, Luiz
| | | 
| | | what about this one?
| | | 
| | | Actually there is just a check for where is error coming from.
| | | Maybe that is not the best solution but it allows us to reduce
| | | the calls to 'printk' :)
| | | 
| | | P.S. Pete your patch is good but the message about
| | | worqueue creation fail was to print even if we've
| | | been faltered on the usb_register procedure.
| | | 
| | | ---
| | | 
| | |  drivers/usb/misc/ftdi-elan.c |   15 +++++++++++----
| | |  1 files changed, 11 insertions(+), 4 deletions(-)
| | | 
| | | diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| | | index bc3327e..3cd54af 100644
| | | --- a/drivers/usb/misc/ftdi-elan.c
| | | +++ b/drivers/usb/misc/ftdi-elan.c
| | | @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
| | |  };
| | |  static int __init ftdi_elan_init(void)
| | |  {
| | | -        int result;
| | | +        int result = 0;
| | |          printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
| | |  	       __TIME__, __DATE__);
| | |          init_MUTEX(&ftdi_module_lock);
| | | @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
| | |  	if (!respond_queue)
| | |  		goto err3;
| | |          result = usb_register(&ftdi_elan_driver);
| | | -        if (result)
| | | +        if (result) {
| | |                  printk(KERN_ERR "usb_register failed. Error number %d\n",
| | |  		       result);
| | | +		goto err4;
| | | +	}
| | |          return result;
| | |  
| | | + err4:
| | | +	destroy_workqueue(respond_queue);
| | |   err3:
| | |  	destroy_workqueue(command_queue);
| | |   err2:
| | |  	destroy_workqueue(status_queue);
| | |   err1:
| | | -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | | -	return -ENOMEM;
| | | +	if (result == 0) {
| | | +		result = -ENOMEM;
| | | +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | | +	}
| | | +	return result;
| | |  }
| | 
| |  I still the prefer the version I sent you yesterday. :) It
| | changes the minimal amount of code.
| | 
| | -- 
| | Luiz Fernando N. Capitulino
| | 
| 
| Liuz,
| 
| but the patch you sent me does call
| 
| 	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| 
| even if error occured in usb_register and that is not good I think.
| Fix me if I'm wrong.

 No, you're right. I missed it.

 Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>

 But Greg is the right person to submit this.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

[Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 02:29:04PM -0300]
| Em Tue, 27 Mar 2007 21:01:25 +0400
| Cyrill Gorcunov <gorcunov@gmail.com> escreveu:
| 
| | [Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 01:28:39PM -0300]
| | | Em Tue, 27 Mar 2007 19:14:05 +0400
| | | Cyrill Gorcunov <gorcunov@gmail.com> escreveu:
| | | 
| | | | Pete, Luiz
| | | | 
| | | | what about this one?
| | | | 
| | | | Actually there is just a check for where is error coming from.
| | | | Maybe that is not the best solution but it allows us to reduce
| | | | the calls to 'printk' :)
| | | | 
| | | | P.S. Pete your patch is good but the message about
| | | | worqueue creation fail was to print even if we've
| | | | been faltered on the usb_register procedure.
| | | | 
| | | | ---
| | | | 
| | | |  drivers/usb/misc/ftdi-elan.c |   15 +++++++++++----
| | | |  1 files changed, 11 insertions(+), 4 deletions(-)
| | | | 
| | | | diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
| | | | index bc3327e..3cd54af 100644
| | | | --- a/drivers/usb/misc/ftdi-elan.c
| | | | +++ b/drivers/usb/misc/ftdi-elan.c
| | | | @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
| | | |  };
| | | |  static int __init ftdi_elan_init(void)
| | | |  {
| | | | -        int result;
| | | | +        int result = 0;
| | | |          printk(KERN_INFO "driver %s built at %s on %s\n", ftdi_elan_driver.name,
| | | |  	       __TIME__, __DATE__);
| | | |          init_MUTEX(&ftdi_module_lock);
| | | | @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
| | | |  	if (!respond_queue)
| | | |  		goto err3;
| | | |          result = usb_register(&ftdi_elan_driver);
| | | | -        if (result)
| | | | +        if (result) {
| | | |                  printk(KERN_ERR "usb_register failed. Error number %d\n",
| | | |  		       result);
| | | | +		goto err4;
| | | | +	}
| | | |          return result;
| | | |  
| | | | + err4:
| | | | +	destroy_workqueue(respond_queue);
| | | |   err3:
| | | |  	destroy_workqueue(command_queue);
| | | |   err2:
| | | |  	destroy_workqueue(status_queue);
| | | |   err1:
| | | | -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | | | -	return -ENOMEM;
| | | | +	if (result == 0) {
| | | | +		result = -ENOMEM;
| | | | +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | | | +	}
| | | | +	return result;
| | | |  }
| | | 
| | |  I still the prefer the version I sent you yesterday. :) It
| | | changes the minimal amount of code.
| | | 
| | | -- 
| | | Luiz Fernando N. Capitulino
| | | 
| | 
| | Liuz,
| | 
| | but the patch you sent me does call
| | 
| | 	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| | 
| | even if error occured in usb_register and that is not good I think.
| | Fix me if I'm wrong.
| 
|  No, you're right. I missed it.
| 
|  Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
| 
|  But Greg is the right person to submit this.
| 
| -- 
| Luiz Fernando N. Capitulino
| 

You mean Greg KH?

		Cyrill

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Pete Zaitcev]

On Tue, 27 Mar 2007 19:14:05 +0400, Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> --- a/drivers/usb/misc/ftdi-elan.c
> @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
>  };
>  static int __init ftdi_elan_init(void)
>  {
> -        int result;
> +        int result = 0;

Why do you need this?

> @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
>  	if (!respond_queue)
>  		goto err3;
>          result = usb_register(&ftdi_elan_driver);
> -        if (result)
> +        if (result) {
>                  printk(KERN_ERR "usb_register failed. Error number %d\n",
>  		       result);
> +		goto err4;
> +	}
>          return result;
>  
> + err4:
> +	destroy_workqueue(respond_queue);
>   err3:

This is fine, although I do wish you wouldn't number the exception labels.
If anything is changed, someone might try to rearrange and renumber them
and that leads to bugs.

>   err1:
> -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
> -	return -ENOMEM;
> +	if (result == 0) {
> +		result = -ENOMEM;
> +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
> +	}
> +	return result;

What in the world is this supposed to do? Under what conditions can
result be zero here?

Personally, I would get rid of the printk. If your modprobe fails,
it's a good enough indication. Or at least, change the text to
something more neutral, like "unable to initialize (%d)" and print
the error code. It's not just about workqueues now.

-- Pete

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

[Pete Zaitcev - Tue, Mar 27, 2007 at 10:51:16AM -0700]
| On Tue, 27 Mar 2007 19:14:05 +0400, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
| 
| > --- a/drivers/usb/misc/ftdi-elan.c
| > @@ -2903,7 +2903,7 @@ static struct usb_driver ftdi_elan_driver = {
| >  };
| >  static int __init ftdi_elan_init(void)
| >  {
| > -        int result;
| > +        int result = 0;
| 
| Why do you need this?

Setting result to 0 indicate that there is no errors. Pete, look
this init code does the following - 

	1) creates 3 workqueues
	2) register usb driver

So we have:
	- at each step of creating worqueue we have to check if it was
	  succsessfull
	- if all workqeues were successfully created we need to check if
	  usb registration failed

So we can get error on any step and there is a question how to properly
and eleganly handle them... the solution I proposed maybe is not ideal
but you may help me supposing your own patch :)

And as result I set 'results=0' to prevent from printing message about
worqueue fails if the error _really_ occured in usb_register. Just
review the function at whole and you will find the reason.

| > @@ -2918,18 +2918,25 @@ static int __init ftdi_elan_init(void)
| >  	if (!respond_queue)
| >  		goto err3;
| >          result = usb_register(&ftdi_elan_driver);
| > -        if (result)
| > +        if (result) {
| >                  printk(KERN_ERR "usb_register failed. Error number %d\n",
| >  		       result);
| > +		goto err4;
| > +	}
| >          return result;
| >  
| > + err4:
| > +	destroy_workqueue(respond_queue);
| >   err3:
| 
| This is fine, although I do wish you wouldn't number the exception labels.
| If anything is changed, someone might try to rearrange and renumber them
| and that leads to bugs.

Agree, may be the labels could be like:

	err_respond_queue:

and so on?

| 
| >   err1:
| > -	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| > -	return -ENOMEM;
| > +	if (result == 0) {
| > +		result = -ENOMEM;
| > +		printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
| > +	}
| > +	return result;
| 
| What in the world is this supposed to do? Under what conditions can
| result be zero here?

If error was in worqueue creation then there will be 0.

| 
| Personally, I would get rid of the printk. If your modprobe fails,
| it's a good enough indication. Or at least, change the text to
| something more neutral, like "unable to initialize (%d)" and print
| the error code. It's not just about workqueues now.
| 
| -- Pete
| 

Can't agree... if modprobe failed I wonna know _where_ it happens and
why :) Or maybe I misunderstood you...

		Cyrill

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Cyrill Gorcunov]

Pete, Luiz

check this one please.

I've inspected ftdi-elan.c for style and as result
the solution I propose is just to add explicit
destroying of worqueues if usb_register failed.

And Pete, take a look - whoa! - I've renamed error labels :)


		Cyrill

---

 drivers/usb/misc/ftdi-elan.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/misc/ftdi-elan.c b/drivers/usb/misc/ftdi-elan.c
index bc3327e..d9cbdb8 100644
--- a/drivers/usb/misc/ftdi-elan.c
+++ b/drivers/usb/misc/ftdi-elan.c
@@ -2910,24 +2910,28 @@ static int __init ftdi_elan_init(void)
         INIT_LIST_HEAD(&ftdi_static_list);
         status_queue = create_singlethread_workqueue("ftdi-status-control");
 	if (!status_queue)
-		goto err1;
+		goto err_status_queue;
         command_queue = create_singlethread_workqueue("ftdi-command-engine");
 	if (!command_queue)
-		goto err2;
+		goto err_command_queue;
         respond_queue = create_singlethread_workqueue("ftdi-respond-engine");
 	if (!respond_queue)
-		goto err3;
+		goto err_respond_queue;
         result = usb_register(&ftdi_elan_driver);
-        if (result)
+        if (result) {
+		destroy_workqueue(status_queue);
+		destroy_workqueue(command_queue);
+		destroy_workqueue(respond_queue);
                 printk(KERN_ERR "usb_register failed. Error number %d\n",
 		       result);
+	}
         return result;
 
- err3:
+ err_respond_queue:
 	destroy_workqueue(command_queue);
- err2:
+ err_command_queue:
 	destroy_workqueue(status_queue);
- err1:
+ err_status_queue:
 	printk(KERN_ERR "%s couldn't create workqueue\n", ftdi_elan_driver.name);
 	return -ENOMEM;
 }

Re: [PATCH] USB Elan FTDI: check for driver registration status

[Pete Zaitcev]

On Wed, 28 Mar 2007 20:00:32 +0400, Cyrill Gorcunov <gorcunov@gmail.com> wrote:

>          result = usb_register(&ftdi_elan_driver);
> -        if (result)
> +        if (result) {
> +		destroy_workqueue(status_queue);
> +		destroy_workqueue(command_queue);
> +		destroy_workqueue(respond_queue);
>                  printk(KERN_ERR "usb_register failed. Error number %d\n",
>  		       result);

I would just change the printk from "couldn't create workqueue"
to something more neutral, but this is ok too. Let's just declare
it fixed and move on.

-- Pete