USB Elan FTDI: check for driver registration status

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

---

Pete, please review the patch and Ack it then.

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

index bc3327e..3cd9ae3 100644
@@ -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)

Hi Cyrill,

Em Sun, 25 Mar 2007 11:27:33 +0400
Cyrill Gorcunov <[email protected]> escreveu:

| This patch adds checking of driver registration status
| and if it fails release allocated resources.
| 
| Signed-off-by: Cyrill Gorcunov <[email protected]>
| 
| ---
| 
| 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

[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 <[email protected]> escreveu:
| 
| | This patch adds checking of driver registration status
| | and if it fails release allocated resources.
| | 
| | Signed-off-by: Cyrill Gorcunov <[email protected]>
| | 
| | ---
| | 
| | 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

Em Mon, 26 Mar 2007 23:33:12 +0400
Cyrill Gorcunov <[email protected]> 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):

index bc3327e..cf95ae8 100644
@@ -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

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(-)

index bc3327e..d9cbdb8 100644
@@ -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;
 }

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(-)

index bc3327e..3cd54af 100644
@@ -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

Em Tue, 27 Mar 2007 19:14:05 +0400
Cyrill Gorcunov <[email protected]> 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

[Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 01:28:39PM -0300]

| Em Tue, 27 Mar 2007 19:14:05 +0400
| Cyrill Gorcunov <[email protected]> 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

Em Tue, 27 Mar 2007 21:01:25 +0400
Cyrill Gorcunov <[email protected]> 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 <[email protected]> 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.

But Greg is the right person to submit this.

Luiz Fernando N. Capitulino

[Luiz Fernando N. Capitulino - Tue, Mar 27, 2007 at 02:29:04PM -0300]

| Em Tue, 27 Mar 2007 21:01:25 +0400
| Cyrill Gorcunov <[email protected]> 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 <[email protected]> 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 <[email protected]>
| 
|  But Greg is the right person to submit this.
| 
| -- 
| Luiz Fernando N. Capitulino
| 

You mean Greg KH?

Cyrill

[Pete Zaitcev - Tue, Mar 27, 2007 at 10:51:16AM -0700]

| On Tue, 27 Mar 2007 19:14:05 +0400, Cyrill Gorcunov <[email protected]> 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