On Sat, 17 Jun 2006 21:47:58 -0700
Pete Zaitcev <[EMAIL PROTECTED]> wrote:
| On Sat, 17 Jun 2006 22:09:49 -0300, "Luiz Fernando N. Capitulino" <[EMAIL
PROTECTED]> wrote:
|
| > | Greg, does this look sane to you?
|
| > 1. While you're are at it, please, create a new function to kill
| > 'port', because now the code in port_release() and destroy_serial()
| > does exactly the same thing (to kill 'port').
| >
| > 2. Just to be safe, do a 'port = NULL' after the call to kfree()
|
| So, I refactored it according to #1, but I'd rather have you send me
| a patch which implements #2. It just makes no sense to me, I cannot
| understand what you could possibly want here.
Consistency. Several functions wich deals with 'port' checks whether
it's NULL before dereferencing it.
The loop which calls device_unregister() in destroy_serial() does it
only for non-fake ports.
It's a minor issue anyway, as fake ports are not registered with the
tty layer.
| I allso threw in the patch for visor leaks. I sent it to Greg before,
| but typoed the list name at cc:
Nice, but the changelog would make things cleaner.
| P.S. Greg was up to his neck in FreedomHEC activities, as I gather from
| his blog, and it was unfortunate. Linus released 2.6.17 today... We better
| hurry with the agreement about this patch of we miss the merge window.
We're ok I think. This is the kind of change that's acceptable for -rc2,
or even -rc3.
| @@ -764,8 +776,14 @@ static int generic_startup(struct usb_se
|
| for (i = 0; i < serial->num_ports; ++i) {
| priv = kzalloc (sizeof(*priv), GFP_KERNEL);
| - if (!priv)
| + if (!priv) {
| + while (i-- != 0) {
| + priv =
usb_get_serial_port_data(serial->port[i]);
| + usb_set_serial_port_data(serial->port[i], NULL);
| + kfree(priv);
| + }
| return -ENOMEM;
| + }
The while() body takes more than 80 columns. Moving it to a goto label
seems better a bit, IMHO.
| spin_lock_init(&priv->lock);
| usb_set_serial_port_data(serial->port[i], priv);
| }
| @@ -877,7 +895,18 @@ static int clie_5_attach (struct usb_ser
|
| static void visor_shutdown (struct usb_serial *serial)
| {
| + struct visor_private *priv;
| + int i;
| +
| dbg("%s", __FUNCTION__);
| +
| + for (i = 0; i < serial->num_ports; i++) {
| + priv = usb_get_serial_port_data(serial->port[i]);
| + if (priv) {
| + usb_set_serial_port_data(serial->port[i], NULL);
| + kfree(priv);
| + }
| + }
| }
Hmm, can't you call visor_shutdown() in generic_startup() if
kzalloc() fails?
To be honest, I don't like that 'promiscuous' way of ordinary
functions like generic_startup() calling a callback like
visor_shutdown() to do its cleanup work. Maybe a
__visor_kill_priv() to do the real work sounds better.
--
Luiz Fernando N. Capitulino
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
