[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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
