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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070327181608.GC11753@cvg>
Date:	Tue, 27 Mar 2007 22:16:08 +0400
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	Pete Zaitcev <zaitcev@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel-list <linux-kernel@...r.kernel.org>,
	"Luiz Fernando N. Capitulino" <lcapitulino@...driva.com.br>
Subject: Re: [PATCH] USB Elan FTDI: check for driver registration status

[Pete Zaitcev - Tue, Mar 27, 2007 at 10:51:16AM -0700]
| On Tue, 27 Mar 2007 19:14:05 +0400, Cyrill Gorcunov <gorcunov@...il.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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/