respin of hso patches for linux-2.6.28-rc6 hso_mutex.patch [patch 4/6]
Subsystems (4)
A new structure hso_mutex_table had to be declared statically & used as as hso_device mutex_lock(&serial->parent->mutex) etc is freed in hso_serial_open & hso_serial_close by kref_put while the mutex is still in use.
This is a substantial change but should make the driver much stabler.
---
drivers/net/usb/hso.c +88 -24
@@ -230,6 +230,11 @@struct work_struct retry_unthrottle_workqueue; }; +struct hso_mutex_t { +struct mutex mutex; +u8 allocated; +}; + struct hso_device { union { struct hso_serial *dev_serial;
@@ -248,7 +253,7 @@struct device *dev;struct kref ref; -struct mutex mutex; +struct hso_mutex_t *mutex; };/* Type of interface */
@@ -364,6 +369,13 @@static spinlock_t serial_table_lock;static struct ktermios *hso_serial_termios[HSO_SERIAL_TTY_MINORS ];static struct ktermios *hso_serial_termios_locked[HSO_SERIAL_TTY_MINORS ]; +/* hso_mutex_table has to be declared statically as hso_device +* is freed in hso_serial_open & hso_serial_close while +* the mutex is still in use. +*/ +#define HSO_NUM_MUTEXES (HSO_SERIAL_TTY_MINORS+HSO_MAX_NET_DEVICES) +static struct hso_mutex_t hso_mutex_table[HSO_NUM_MUTEXES ]; +static spinlock_t hso_mutex_lock;static const s32 default_port_spec[] = {HSO_INTF_MUX |HSO_PORT_NETWORK ,
@@ -604,6 +616,34 @@spin_unlock_irqrestore (&serial_table_lock, flags); } + +static struct hso_mutex_t *hso_get_free_mutex (void ) +{ +int index; +struct hso_mutex_t *curr_hso_mutex; + +spin_lock (&hso_mutex_lock); +for (index =0 ; index <HSO_NUM_MUTEXES ; index++) { + curr_hso_mutex = &hso_mutex_table[index]; +if (!curr_hso_mutex->allocated) { + curr_hso_mutex->allocated =1 ; +spin_unlock (&hso_mutex_lock); +return curr_hso_mutex; + } + } +printk (KERN_ERR "BUG %s: no free hso_mutexs devices in table\n" + , __func__); +spin_unlock (&hso_mutex_lock); +return NULL ; +} + +static void hso_free_mutex (struct hso_mutex_t *mutex) +{ +spin_lock (&hso_mutex_lock); + mutex->allocated =0 ; +spin_unlock (&hso_mutex_lock); +} +/* log a meaningful explanation of an USB status */ static void log_usb_status (int status,const char *function) {
@@ -1225,7 +1265,9 @@static int hso_serial_open (struct tty_struct *tty,struct file *filp) {struct hso_serial *serial =get_serial_by_index (tty->index); -int result; +int result1 =0 , result2 =0 ; +struct mutex *hso_mutex =NULL ; +int refcnt =1 ;/* sanity check */ if (serial ==NULL || serial->magic !=HSO_SERIAL_MAGIC ) {
@@ -1234,15 +1276,16 @@return -ENODEV ; } -mutex_lock (&serial->parent->mutex); + hso_mutex = &serial->parent->mutex->mutex; +mutex_lock (hso_mutex);/* check for port already opened, if not set the termios */ /* The serial->open count needs to be here as hso_serial_close * will be called even if hso_serial_open returns -ENODEV. */ serial->open_count++; - result =usb_autopm_get_interface (serial->parent->interface); -if (result <0 ) - goto err_out; + result1 =usb_autopm_get_interface (serial->parent->interface); +if (result1 <0 ) + goto err_out;D1 ("Opening %d" , serial->minor);kref_get (&serial->parent->ref);
@@ -1261,11 +1304,10 @@ (unsigned long)serial);INIT_WORK (&serial->retry_unthrottle_workqueue, hso_unthrottle_workfunc); - result =hso_start_serial_device (serial->parent,GFP_KERNEL ); -if (result) { + result2 =hso_start_serial_device (serial->parent,GFP_KERNEL ); +if (result2) {hso_stop_serial_device (serial->parent); serial->open_count--; -kref_put (&serial->parent->ref, hso_serial_ref_free); } }else {D1 ("Port was already open" );
@@ -1274,11 +1316,16 @@usb_autopm_put_interface (serial->parent->interface);/* done */ -if (result) +if (result1)hso_serial_tiocmset (tty,NULL ,TIOCM_RTS |TIOCM_DTR ,0 ); err_out: -mutex_unlock (&serial->parent->mutex); -return result; +if (result2) + refcnt =kref_put (&serial->parent->ref, hso_serial_ref_free); +mutex_unlock (hso_mutex); +if (refcnt ==0 ) +hso_free_mutex (container_of (hso_mutex, + struct hso_mutex_t, mutex)); +return result1 ==0 ? result2 : result1; }/* close the requested serial port */
@@ -1286,6 +1333,8 @@ {struct hso_serial *serial = tty->driver_data;u8 usb_gone; +struct mutex *hso_mutex; +int refcnt;D1 ("Closing serial port" );if (serial ==NULL || serial->magic !=HSO_SERIAL_MAGIC ) {
@@ -1293,8 +1342,9 @@return ; } -mutex_lock (&serial->parent->mutex); usb_gone = serial->parent->usb_gone; + hso_mutex = &serial->parent->mutex->mutex; +mutex_lock (hso_mutex);if (!usb_gone)usb_autopm_get_interface (serial->parent->interface);
@@ -1302,7 +1352,6 @@/* reset the rts and dtr */ /* do the actual close */ serial->open_count--; -kref_put (&serial->parent->ref, hso_serial_ref_free);if (serial->open_count <=0 ) { serial->open_count =0 ;if (serial->tty) {
@@ -1317,8 +1366,11 @@if (!usb_gone)usb_autopm_put_interface (serial->parent->interface); - -mutex_unlock (&serial->parent->mutex); + refcnt =kref_put (&serial->parent->ref, hso_serial_ref_free); +mutex_unlock (hso_mutex); +if (refcnt ==0 ) +hso_free_mutex (container_of (hso_mutex, + struct hso_mutex_t, mutex)); }/* close the requested serial port */
@@ -2084,8 +2136,12 @@ hso_dev->usb =interface_to_usbdev (intf); hso_dev->interface = intf;kref_init (&hso_dev->ref); -mutex_init (&hso_dev->mutex); - + hso_dev->mutex =hso_get_free_mutex (); +if (!hso_dev->mutex) { +kfree (hso_dev); +return NULL ; + } +mutex_init (&hso_dev->mutex->mutex);INIT_WORK (&hso_dev->async_get_intf, async_get_intf);INIT_WORK (&hso_dev->async_put_intf, async_put_intf);
@@ -2131,7 +2187,7 @@unregister_netdev (hso_net->net);free_netdev (hso_net->net); } - +hso_free_mutex (hso_dev->mutex);hso_free_device (hso_dev); }
@@ -2180,14 +2236,14 @@int enabled = (state ==RFKILL_STATE_ON );int rv; -mutex_lock (&hso_dev->mutex); +mutex_lock (&hso_dev->mutex->mutex);if (hso_dev->usb_gone) rv =0 ;else rv =usb_control_msg (hso_dev->usb,usb_rcvctrlpipe (hso_dev->usb,0 ), enabled ?0x82 :0x81 ,0x40 ,0 ,0 ,NULL ,0 ,USB_CTRL_SET_TIMEOUT ); -mutex_unlock (&hso_dev->mutex); +mutex_unlock (&hso_dev->mutex->mutex);return rv; }
@@ -2795,6 +2851,8 @@ {struct hso_serial *hso_dev;int i; +struct mutex *hso_mutex =NULL ; +int refcnt =1 ;for (i =0 ; i <HSO_SERIAL_TTY_MINORS ; i++) { if (serial_table[i]
@@ -2802,10 +2860,12 @@ hso_dev =dev2ser (serial_table[i]);if (hso_dev->tty)tty_hangup (hso_dev->tty); -mutex_lock (&hso_dev->parent->mutex); + hso_mutex = &hso_dev->parent->mutex->mutex; +mutex_lock (hso_mutex); hso_dev->parent->usb_gone =1 ; -mutex_unlock (&hso_dev->parent->mutex); -kref_put (&serial_table[i]->ref, hso_serial_ref_free); + refcnt =kref_put (&serial_table[i]->ref, + hso_serial_ref_free); +mutex_unlock (hso_mutex); } }
@@ -2824,6 +2884,9 @@hso_free_net_device (network_table[i]); } } +if (refcnt ==0 ) +hso_free_mutex (container_of (hso_mutex, + struct hso_mutex_t, mutex)); }/* Helper functions */
@@ -2922,6 +2985,7 @@/* Initialise the serial table semaphore and table */ spin_lock_init (&serial_table_lock); +spin_lock_init (&hso_mutex_lock);for (i =0 ; i <HSO_SERIAL_TTY_MINORS ; i++) serial_table[i] =NULL ;
Signature · 2 lines
best regards, D.J. Barrow
A new structure hso_mutex_table had to be declared statically & used as as hso_device mutex_lock(&serial->parent->mutex) etc is freed in hso_serial_open & hso_serial_close by kref_put while the mutex is still in use.
This is a substantial change but should make the driver much stabler.
---
drivers/net/usb/hso.c +88 -24
243 lines not on this page · raw message
Hi Denis,
Quoted 26 lines
A new structure
hso_mutex_tablehad to be declared statically & used as ashso_devicemutex_lock(&serial->parent->mutex)etc is freed inhso_serial_open&hso_serial_closebykref_putwhile the mutex is still in use.
This is a substantial change but should make the driver much stabler.Signed-off-by Denis Joseph Barrow <[email protected]>---
Index:linux-2.6.28-rc6.patches/drivers/net/usb/hso.c
===================================================================
---linux-2.6.28-rc6.patches.orig/drivers/net/usb/hso.c2008-11-24
14:19:17.000000000 +0100
+++linux-2.6.28-rc6.patches/drivers/net/usb/hso.c2008-11-24
14:59:29.000000000 +0100drivers/net/usb/hso.c +5
-0@@ -230,6 +230,11 @@struct work_struct retry_unthrottle_workqueue; }; +struct hso_mutex_t { +struct mutex mutex; +u8 allocated; +}; + struct hso_device { union { struct hso_serial *dev_serial;
drivers/net/usb/hso.c line 257
Quoted 6 lines
drivers/net/usb/hso.c +1
-1@@ -248,7 +253,7 @@struct device *dev;struct kref ref; -struct mutex mutex; +struct hso_mutex_t *mutex; };
this looks pretty ugly to me. Can we not find a more elegant way to solve this problem? It might involve fixing or changing the TTY layer, but that is actually fine.
Regards
Marcel
A message that is not in the archive (sent off-list or not yet received): [email protected]
David Miller
AppliedFrom: Denis Joseph Barrow <[email protected]>
Date: Mon, 24 Nov 2008 15:48:47 +0100
Quoted 6 lines
A new structure
hso_mutex_tablehad to be declared statically & used as ashso_devicemutex_lock(&serial->parent->mutex)etc is freed inhso_serial_open&hso_serial_closebykref_putwhile the mutex is still in use.
This is a substantial change but should make the driver much stabler.Signed-off-by Denis Joseph Barrow <[email protected]>
Applied, but I had to fixup a lot of whitespace errors.
All the lines that add code had a single initial space and then tabs.
I know where this comes from. You applied the patch to your tree, there were rejects, and you copied and pasted the hunks from the foo.rej file into the source file.
In doing so you left the initial space character that sits after each "+" in the foo.rej file.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · On Tue, 25 Nov 2008 00:34:27 -0800 (PST) David Miller <[email protected]>…
From: Denis Joseph Barrow <[email protected]>
Date: Mon, 24 Nov 2008 15:48:47 +0100Quoted 6 lines
A new structure
hso_mutex_tablehad to be declared statically & used as ashso_devicemutex_lock(&serial->parent->mutex)etc is freed inhso_serial_open&hso_serial_closebykref_putwhile the mutex is still in use.
This is a substantial change but should make the driver much stabler.Signed-off-by Denis Joseph Barrow <[email protected]>Applied, but I had to fixup a lot of whitespace errors.
Dave - I don't put random network layer patches in the ttydev tree, please don't put random tty driver patches in the network tree. Those patches
a) are woefully inadequate
b) will clash with the ttydev work which is where this belongs
Alan
David Miller
maintainerFrom: Alan Cox <[email protected]>
Date: Tue, 25 Nov 2008 10:20:43 +0000
Quoted 13 lines
Quoted 10 lines · On Tue, 25 Nov 2008 00:34:27 -0800 (PST) David Miller <[email protected]>…
From: Denis Joseph Barrow <[email protected]>
Date: Mon, 24 Nov 2008 15:48:47 +0100Quoted 6 lines
A new structure
hso_mutex_tablehad to be declared statically & used as ashso_devicemutex_lock(&serial->parent->mutex)etc is freed inhso_serial_open&hso_serial_closebykref_putwhile the mutex is still in use.
This is a substantial change but should make the driver much stabler.Signed-off-by Denis Joseph Barrow <[email protected]>
Quoted 8 lines
Quoted 1 line
Applied, but I had to fixup a lot of whitespace errors.
Dave - I don't put random network layer patches in the ttydev tree, please don't put random tty driver patches in the network tree. Those patches
a) are woefully inadequate
b) will clash with the ttydev work which is where this belongs
I thought this was a network driver?
If so, this poor guy has respun these patches so many times my head is spinning, and making him wait for one more procedural thing is just a good way to deter yet another developer.
Most of these patches looked fine, is there a specific one I should leave out?
I'm not leaving all of them out, most of them looked perfectly fine and didn't do any "tty" stuff.
Quoted 1 line
I thought this was a network driver?
It's both, but the resource handling mess is on the tty side, and several of the chunks (modem control etc) directly hit the tty parts and will clash with other tty layer changes.
Quoted 4 lines
If so, this poor guy has respun these patches so many times my head is spinning, and making him wait for one more procedural thing is just a good way to deter yet another developer.
Well if you check the list I sent him a set of patches to review in response to his posting. Those patches implement proper tty side refcounting and have a chance of actually fixing the problems.
Quoted 2 lines
I'm not leaving all of them out, most of them looked perfectly fine and didn't do any "tty" stuff.
That patch series is a complete mix:
1, 3 and 6 are network
2 is a broken tty layer patch
4 is a broken tty layer patch
5 is an ok tty layer patch
5 will go into the tty tree
2 and 4 clash with the tty tree already and don't deal with the tty side locking at all sanely.
So I've no problem with the network parts (1,3,6) going into the netdev tree but the other bits will make a mess if they do so.
A message that is not in the archive (sent off-list or not yet received): 20081125104213.03b280c5-qBU/[email protected]
From: Alan Cox <alan-qBU/[email protected]>
Date: Tue, 25 Nov 2008 10:42:13 +0000
Quoted 2 lines
So I've no problem with the network parts (1,3,6) going into the netdev tree but the other bits will make a mess if they do so.
Ok, I'll revert the other ones.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Alan Cox <alan-qBU/[email protected]>
Date: Tue, 25 Nov 2008 10:42:13 +0000
Quoted 7 lines
That patch series is a complete mix:
1, 3 and 6 are network
2 is a broken tty layer patch
4 is a broken tty layer patch
5 is an ok tty layer patch
Quoted 7 lines
5 will go into the tty tree
2 and 4 clash with the tty tree already and don't deal with the tty side locking at all sanely.
So I've no problem with the network parts (1,3,6) going into the netdev tree but the other bits will make a mess if they do so.
Actually Alan, can you do me a favor?
These patches were not numbered, and when you say patch N it's ambiguous (especially since they arrived on the mailing list in a different order than they arrived in my private inbox, the latter of which is the order I applied them to my tree).
So please let me know which patches exactly are which and I'll do the reverting.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20081125.031150.170595881.davem-fT/PcQaiUtIeIZ0/[email protected]
Quoted 3 lines
Actually Alan, can you do me a favor?
These patches were not numbered, and when you say patch N it's ambiguous
See the subject line including the one you replied to .. they were numbered but not in the usual way.
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html