Skip to contentSkip to conversation

respin of hso patches for linux-2.6.28-rc6 hso_mutex.patch [patch 4/6]

PATCH Applied Denis Joseph Barrow 6 participants 9 messages Nov 24, 2008 – Nov 25, 2008 3 messages not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

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
Index: linux-2.6.28-rc6.patches/drivers/net/usb/hso.c
===================================================================
@@ -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

#1 lore raw

Hi Denis,

Quoted 26 lines

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.

---
Index: linux-2.6.28-rc6.patches/drivers/net/usb/hso.c
===================================================================
--- linux-2.6.28-rc6.patches.orig/drivers/net/usb/hso.c 2008-11-24
14:19:17.000000000 +0100
+++ linux-2.6.28-rc6.patches/drivers/net/usb/hso.c 2008-11-24
14:59:29.000000000 +0100

drivers/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]

Applied
#3 lore raw

From: Denis Joseph Barrow <[email protected]>
Date: Mon, 24 Nov 2008 15:48:47 +0100

Quoted 6 lines

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.

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

#4 lore raw
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 +0100

Quoted 6 lines

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.

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

maintainer
#5 lore raw

From: 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 +0100

Quoted 6 lines

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.

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.

#6 lore raw
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]

#9 lore raw
In reply to a message not in the archive

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]

#11 lore raw
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