[PATCH 1/2] hso: remove unused workqueue

Subsystems: hso 3g modem driver, networking drivers, the rest, usb networking drivers

STALE4406d

19 messages, 5 authors, 2014-07-11 · open the first message on its own page

[PATCH 1/2] hso: remove unused workqueue

From: Olivier Sobrie <hidden>
Date: 2014-07-07 09:07:33

The workqueue "retry_unthrottle_workqueue" is not scheduled anywhere
in the code. So, remove it.

Signed-off-by: Olivier Sobrie <redacted>
---
 drivers/net/usb/hso.c |   12 ------------
 1 file changed, 12 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index a3a0586..9ca2b41 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -261,7 +261,6 @@ struct hso_serial {
 	u16  curr_rx_urb_offset;
 	u8   rx_urb_filled[MAX_RX_URBS];
 	struct tasklet_struct unthrottle_tasklet;
-	struct work_struct    retry_unthrottle_workqueue;
 };
 
 struct hso_device {
@@ -1252,14 +1251,6 @@ static	void hso_unthrottle(struct tty_struct *tty)
 	tasklet_hi_schedule(&serial->unthrottle_tasklet);
 }
 
-static void hso_unthrottle_workfunc(struct work_struct *work)
-{
-	struct hso_serial *serial =
-	    container_of(work, struct hso_serial,
-			 retry_unthrottle_workqueue);
-	hso_unthrottle_tasklet(serial);
-}
-
 /* open the requested serial port */
 static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 {
@@ -1295,8 +1286,6 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		tasklet_init(&serial->unthrottle_tasklet,
 			     (void (*)(unsigned long))hso_unthrottle_tasklet,
 			     (unsigned long)serial);
-		INIT_WORK(&serial->retry_unthrottle_workqueue,
-			  hso_unthrottle_workfunc);
 		result = hso_start_serial_device(serial->parent, GFP_KERNEL);
 		if (result) {
 			hso_stop_serial_device(serial->parent);
@@ -1345,7 +1334,6 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		if (!usb_gone)
 			hso_stop_serial_device(serial->parent);
 		tasklet_kill(&serial->unthrottle_tasklet);
-		cancel_work_sync(&serial->retry_unthrottle_workqueue);
 	}
 
 	if (!usb_gone)
-- 
1.7.9.5

[PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-07 09:07:40

When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.

Signed-off-by: Olivier Sobrie <redacted>
---
 drivers/net/usb/hso.c |   51 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 9ca2b41..1dff74f 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -106,6 +106,8 @@
 
 #define MAX_RX_URBS			2
 
+#define UNTHROTTLE_TIMEOUT		100 /* msecs */
+
 /*****************************************************************************/
 /* Debugging functions                                                       */
 /*****************************************************************************/
@@ -261,6 +263,7 @@ struct hso_serial {
 	u16  curr_rx_urb_offset;
 	u8   rx_urb_filled[MAX_RX_URBS];
 	struct tasklet_struct unthrottle_tasklet;
+	struct timer_list unthrottle_timer;
 };
 
 struct hso_device {
@@ -1161,13 +1164,18 @@ static void put_rxbuf_data_and_resubmit_bulk_urb(struct hso_serial *serial)
 	while (serial->rx_urb_filled[serial->curr_rx_urb_idx]) {
 		curr_urb = serial->rx_urb[serial->curr_rx_urb_idx];
 		count = put_rxbuf_data(curr_urb, serial);
-		if (count == -1)
-			return;
 		if (count == 0) {
 			serial->curr_rx_urb_idx++;
 			if (serial->curr_rx_urb_idx >= serial->num_rx_urbs)
 				serial->curr_rx_urb_idx = 0;
 			hso_resubmit_rx_bulk_urb(serial, curr_urb);
+		} else if (count > 0) {
+			mod_timer(&serial->unthrottle_timer,
+				  jiffies
+					+ msecs_to_jiffies(UNTHROTTLE_TIMEOUT));
+			break;
+		} else {
+			break;
 		}
 	}
 }
@@ -1251,6 +1259,11 @@ static	void hso_unthrottle(struct tty_struct *tty)
 	tasklet_hi_schedule(&serial->unthrottle_tasklet);
 }
 
+static void hso_unthrottle_schedule(unsigned long data)
+{
+	tasklet_schedule((struct tasklet_struct *)data);
+}
+
 /* open the requested serial port */
 static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 {
@@ -1286,6 +1299,10 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		tasklet_init(&serial->unthrottle_tasklet,
 			     (void (*)(unsigned long))hso_unthrottle_tasklet,
 			     (unsigned long)serial);
+		serial->unthrottle_timer.function = hso_unthrottle_schedule;
+		serial->unthrottle_timer.data =
+				(unsigned long)&serial->unthrottle_tasklet;
+		init_timer(&serial->unthrottle_timer);
 		result = hso_start_serial_device(serial->parent, GFP_KERNEL);
 		if (result) {
 			hso_stop_serial_device(serial->parent);
@@ -1333,6 +1350,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		tty_port_tty_set(&serial->port, NULL);
 		if (!usb_gone)
 			hso_stop_serial_device(serial->parent);
+		del_timer_sync(&serial->unthrottle_timer);
 		tasklet_kill(&serial->unthrottle_tasklet);
 	}
 
@@ -2012,22 +2030,23 @@ static int put_rxbuf_data(struct urb *urb, struct hso_serial *serial)
 
 	tty = tty_port_tty_get(&serial->port);
 
+	if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
+		tty_kref_put(tty);
+		return -1;
+	}
+
 	/* Push data to tty */
-	write_length_remaining = urb->actual_length -
-		serial->curr_rx_urb_offset;
 	D1("data to push to tty");
-	while (write_length_remaining) {
-		if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
-			tty_kref_put(tty);
-			return -1;
-		}
-		curr_write_len = tty_insert_flip_string(&serial->port,
-			urb->transfer_buffer + serial->curr_rx_urb_offset,
-			write_length_remaining);
-		serial->curr_rx_urb_offset += curr_write_len;
-		write_length_remaining -= curr_write_len;
-		tty_flip_buffer_push(&serial->port);
-	}
+	write_length_remaining = urb->actual_length -
+					serial->curr_rx_urb_offset;
+	curr_write_len =
+		tty_insert_flip_string(&serial->port,
+				       urb->transfer_buffer
+						+ serial->curr_rx_urb_offset,
+				       write_length_remaining);
+	serial->curr_rx_urb_offset += curr_write_len;
+	write_length_remaining -= curr_write_len;
+	tty_flip_buffer_push(&serial->port);
 	tty_kref_put(tty);
 
 	if (write_length_remaining == 0) {
-- 
1.7.9.5

RE: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Laight <hidden>
Date: 2014-07-07 09:15:22

From: Olivier Sobrie
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
What is the timer for?
You need to get the sending code woken up by the urb completion.

	David

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-07 10:42:47

Hi David,

On Mon, Jul 07, 2014 at 09:13:53AM +0000, David Laight wrote:
From: Olivier Sobrie
quoted
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
What is the timer for?
You need to get the sending code woken up by the urb completion.
In put_rxbuf_data() (which can be called under irq disabled),
tty_flip_buffer_push() is called and schedules a push of the tty buffer.
When the buffer is full, I give some time to the above layer in order
to flush it.
The timer is used to recall put_rxbuf_data_and_resubmit_bulk_urb()
later in order to read the remaining data stored in
"urb->transfer_buffer" and then to resubmit the urb to receive more data
from the gsm module.
I don't understand what you mean by "getting the sending code woken up".
Calling tty_port_tty_wakeup()?? We are in the receive path...

Thanks,

-- 
Olivier

RE: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Laight <hidden>
Date: 2014-07-07 12:57:12

From: Olivier Sobrie 
Hi David,

On Mon, Jul 07, 2014 at 09:13:53AM +0000, David Laight wrote:
quoted
From: Olivier Sobrie
quoted
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
What is the timer for?
You need to get the sending code woken up by the urb completion.
In put_rxbuf_data() (which can be called under irq disabled),
tty_flip_buffer_push() is called and schedules a push of the tty buffer.
When the buffer is full, I give some time to the above layer in order
to flush it.
The timer is used to recall put_rxbuf_data_and_resubmit_bulk_urb()
later in order to read the remaining data stored in
"urb->transfer_buffer" and then to resubmit the urb to receive more data
from the gsm module.
I don't understand what you mean by "getting the sending code woken up".
Calling tty_port_tty_wakeup()?? We are in the receive path...
Sorry, it isn't at all clear from the general description whether you
are referring to the transmit or receive path.

'flush' can mean all sorts of things ...

In either case a 100ms delay to data doesn't seem right at all.

	David

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-07 13:39:19

On Mon, Jul 07, 2014 at 12:55:44PM +0000, David Laight wrote:
From: Olivier Sobrie 
quoted
Hi David,

On Mon, Jul 07, 2014 at 09:13:53AM +0000, David Laight wrote:
quoted
From: Olivier Sobrie
quoted
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
What is the timer for?
You need to get the sending code woken up by the urb completion.
In put_rxbuf_data() (which can be called under irq disabled),
tty_flip_buffer_push() is called and schedules a push of the tty buffer.
When the buffer is full, I give some time to the above layer in order
to flush it.
The timer is used to recall put_rxbuf_data_and_resubmit_bulk_urb()
later in order to read the remaining data stored in
"urb->transfer_buffer" and then to resubmit the urb to receive more data
from the gsm module.
I don't understand what you mean by "getting the sending code woken up".
Calling tty_port_tty_wakeup()?? We are in the receive path...
Sorry, it isn't at all clear from the general description whether you
are referring to the transmit or receive path.

'flush' can mean all sorts of things ...

In either case a 100ms delay to data doesn't seem right at all.
An option to avoid the delay, is to replace the timer by a workqueue
and to schedule work in put_rxbuf_data_and_resubmit_bulk_urb()
when put_rxbuf_data() returns something greater than 0.
If you have better ideas, let me know.

Thanks,

-- 
Olivier

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Dan Williams <hidden>
Date: 2014-07-07 17:27:40

On Mon, 2014-07-07 at 11:06 +0200, Olivier Sobrie wrote:
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
I assume this problem happens when using PPP for data transfer, instead
of using the network port that the modules expose?  Or maybe the NMEA
port?  I can't imagine that AT commands would make this happen...

Dan
quoted hunk
Signed-off-by: Olivier Sobrie <redacted>
---
 drivers/net/usb/hso.c |   51 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 9ca2b41..1dff74f 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -106,6 +106,8 @@
 
 #define MAX_RX_URBS			2
 
+#define UNTHROTTLE_TIMEOUT		100 /* msecs */
+
 /*****************************************************************************/
 /* Debugging functions                                                       */
 /*****************************************************************************/
@@ -261,6 +263,7 @@ struct hso_serial {
 	u16  curr_rx_urb_offset;
 	u8   rx_urb_filled[MAX_RX_URBS];
 	struct tasklet_struct unthrottle_tasklet;
+	struct timer_list unthrottle_timer;
 };
 
 struct hso_device {
@@ -1161,13 +1164,18 @@ static void put_rxbuf_data_and_resubmit_bulk_urb(struct hso_serial *serial)
 	while (serial->rx_urb_filled[serial->curr_rx_urb_idx]) {
 		curr_urb = serial->rx_urb[serial->curr_rx_urb_idx];
 		count = put_rxbuf_data(curr_urb, serial);
-		if (count == -1)
-			return;
 		if (count == 0) {
 			serial->curr_rx_urb_idx++;
 			if (serial->curr_rx_urb_idx >= serial->num_rx_urbs)
 				serial->curr_rx_urb_idx = 0;
 			hso_resubmit_rx_bulk_urb(serial, curr_urb);
+		} else if (count > 0) {
+			mod_timer(&serial->unthrottle_timer,
+				  jiffies
+					+ msecs_to_jiffies(UNTHROTTLE_TIMEOUT));
+			break;
+		} else {
+			break;
 		}
 	}
 }
@@ -1251,6 +1259,11 @@ static	void hso_unthrottle(struct tty_struct *tty)
 	tasklet_hi_schedule(&serial->unthrottle_tasklet);
 }
 
+static void hso_unthrottle_schedule(unsigned long data)
+{
+	tasklet_schedule((struct tasklet_struct *)data);
+}
+
 /* open the requested serial port */
 static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 {
@@ -1286,6 +1299,10 @@ static int hso_serial_open(struct tty_struct *tty, struct file *filp)
 		tasklet_init(&serial->unthrottle_tasklet,
 			     (void (*)(unsigned long))hso_unthrottle_tasklet,
 			     (unsigned long)serial);
+		serial->unthrottle_timer.function = hso_unthrottle_schedule;
+		serial->unthrottle_timer.data =
+				(unsigned long)&serial->unthrottle_tasklet;
+		init_timer(&serial->unthrottle_timer);
 		result = hso_start_serial_device(serial->parent, GFP_KERNEL);
 		if (result) {
 			hso_stop_serial_device(serial->parent);
@@ -1333,6 +1350,7 @@ static void hso_serial_close(struct tty_struct *tty, struct file *filp)
 		tty_port_tty_set(&serial->port, NULL);
 		if (!usb_gone)
 			hso_stop_serial_device(serial->parent);
+		del_timer_sync(&serial->unthrottle_timer);
 		tasklet_kill(&serial->unthrottle_tasklet);
 	}
 
@@ -2012,22 +2030,23 @@ static int put_rxbuf_data(struct urb *urb, struct hso_serial *serial)
 
 	tty = tty_port_tty_get(&serial->port);
 
+	if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
+		tty_kref_put(tty);
+		return -1;
+	}
+
 	/* Push data to tty */
-	write_length_remaining = urb->actual_length -
-		serial->curr_rx_urb_offset;
 	D1("data to push to tty");
-	while (write_length_remaining) {
-		if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
-			tty_kref_put(tty);
-			return -1;
-		}
-		curr_write_len = tty_insert_flip_string(&serial->port,
-			urb->transfer_buffer + serial->curr_rx_urb_offset,
-			write_length_remaining);
-		serial->curr_rx_urb_offset += curr_write_len;
-		write_length_remaining -= curr_write_len;
-		tty_flip_buffer_push(&serial->port);
-	}
+	write_length_remaining = urb->actual_length -
+					serial->curr_rx_urb_offset;
+	curr_write_len =
+		tty_insert_flip_string(&serial->port,
+				       urb->transfer_buffer
+						+ serial->curr_rx_urb_offset,
+				       write_length_remaining);
+	serial->curr_rx_urb_offset += curr_write_len;
+	write_length_remaining -= curr_write_len;
+	tty_flip_buffer_push(&serial->port);
 	tty_kref_put(tty);
 
 	if (write_length_remaining == 0) {

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-07 18:49:49

Hi Dan,

On Mon, Jul 07, 2014 at 11:41:20AM -0500, Dan Williams wrote:
On Mon, 2014-07-07 at 11:06 +0200, Olivier Sobrie wrote:
quoted
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.
I assume this problem happens when using PPP for data transfer, instead
of using the network port that the modules expose?  Or maybe the NMEA
port?  I can't imagine that AT commands would make this happen...
Yes it happens when using ppp for data transfer.
The transfer of a large file sometimes triggers the problem.

-- 
Olivier

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Miller <davem@davemloft.net>
Date: 2014-07-08 23:16:36

From: Olivier Sobrie <redacted>
Date: Mon,  7 Jul 2014 11:06:07 +0200
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.

Signed-off-by: Olivier Sobrie <redacted>
I agree with the feedback you've been given in that adding a delay
like this is really not a reasonable solution.

Why is it so difficult to make the event which places the data there
trigger to necessary calls to pull the data out of the URB transfer
buffer?

This should be totally and completely event based.

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-10 14:29:06

Hi David,

On Tue, Jul 08, 2014 at 04:16:33PM -0700, David Miller wrote:
From: Olivier Sobrie <redacted>
Date: Mon,  7 Jul 2014 11:06:07 +0200
quoted
When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

To avoid this, first, we remove the endless while loop in
put_rx_bufdata() which is the root cause of the deadlock.
Secondly, when there is no room anymore in the tty buffer, we set up a
timer of 100 msecs to give a chance to the upper layer to flush the tty
buffer and make room for new data.

Signed-off-by: Olivier Sobrie <redacted>
I agree with the feedback you've been given in that adding a delay
like this is really not a reasonable solution.

Why is it so difficult to make the event which places the data there
trigger to necessary calls to pull the data out of the URB transfer
buffer?

This should be totally and completely event based.
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
An option I see is: If tty_insert_flip_string_flags() returns 0, start a
workqueue that will insert the remaining data in the tty buffer and then
restart the urb. But I'm not convinced that it is a good solution.
I should miss something...

In put_rxbuf_data(), when tty_insert_flip_string_flags() returns 0, would
it be correct to set the TTY_THROTTLED flag? I assume not...

I'll have a look in other drivers how such cases are handled.

Thanks,

-- 
Olivier

RE: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Laight <hidden>
Date: 2014-07-10 14:39:18

From: Olivier Sobrie
...
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
Surely you can just keep the urb?
Resubmit it later when all the data has been transferred.

	David

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: One Thousand Gnomes <hidden>
Date: 2014-07-10 15:50:41

On Thu, 10 Jul 2014 14:37:37 +0000
David Laight [off-list ref] wrote:
From: Olivier Sobrie
...
quoted
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
The tty provides the input queue, if the queue is full then just chuck
the data in the bitbucket.  hso is trying to be far too clever.

If hso is fast enough that the buffering isn't sufficient on the tty side
then we need to fix the tty buffer size.

Arguably what we need are tty fastpaths for non N_TTY but thats rather
more work. There's no fundamental reason that hso can't throw the buffer
at buffer straight at the PPP ldisc and straight into the network stack -
just that 

1. The tty mid layer glue is standing in the way
2. The change of line discipline code has to lock against it

Alan

RE: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Laight <hidden>
Date: 2014-07-10 15:56:55

From: One Thousand Gnomes
On Thu, 10 Jul 2014 14:37:37 +0000
David Laight [off-list ref] wrote:
quoted
From: Olivier Sobrie
...
quoted
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
The tty provides the input queue, if the queue is full then just chuck
the data in the bitbucket.  hso is trying to be far too clever.

If hso is fast enough that the buffering isn't sufficient on the tty side
then we need to fix the tty buffer size.
You really want to apply flow control back over the 'serial' link.
That may just cause data discards earlier on the local system.
But it is possible that not resubmitting the receive urb will cause the
modem to flow control back to the sender.
In which case there is some chance that data won't be lost.

	David
Arguably what we need are tty fastpaths for non N_TTY but thats rather
more work. There's no fundamental reason that hso can't throw the buffer
at buffer straight at the PPP ldisc and straight into the network stack -
just that

1. The tty mid layer glue is standing in the way
2. The change of line discipline code has to lock against it

Alan

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: One Thousand Gnomes <hidden>
Date: 2014-07-10 21:21:03

You really want to apply flow control back over the 'serial' link.
That may just cause data discards earlier on the local system.
But it is possible that not resubmitting the receive urb will cause the
modem to flow control back to the sender.
In which case there is some chance that data won't be lost.
If you are doing PPP and you can't keep up the sooner you chuck data the
better. Flow control actually works against performance and good network
behaviour. It's counter intuitive but TCP/IP works best if any
performance bottlenecks are immediately visible and not covered over.

Alan

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-11 09:18:21

Hi Alan and Davids,

On Thu, Jul 10, 2014 at 04:50:03PM +0100, One Thousand Gnomes wrote:
On Thu, 10 Jul 2014 14:37:37 +0000
David Laight [off-list ref] wrote:
quoted
From: Olivier Sobrie
...
quoted
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
The tty provides the input queue, if the queue is full then just chuck
the data in the bitbucket.  hso is trying to be far too clever.

If hso is fast enough that the buffering isn't sufficient on the tty side
then we need to fix the tty buffer size.
Ok I'll adapt the patch to drop the data that can't be put in the tty
buffer. I test this and resend a new patch.

Thanks for your help!

-- 
Olivier

RE: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: David Laight <hidden>
Date: 2014-07-11 09:29:31

From: Olivier Sobrie Olivier Sobrie
Hi Alan and Davids,

On Thu, Jul 10, 2014 at 04:50:03PM +0100, One Thousand Gnomes wrote:
quoted
On Thu, 10 Jul 2014 14:37:37 +0000
David Laight [off-list ref] wrote:
quoted
From: Olivier Sobrie
...
quoted
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
The tty provides the input queue, if the queue is full then just chuck
the data in the bitbucket.  hso is trying to be far too clever.

If hso is fast enough that the buffering isn't sufficient on the tty side
then we need to fix the tty buffer size.
Ok I'll adapt the patch to drop the data that can't be put in the tty
buffer. I test this and resend a new patch.
If you are going to drop data, then ideally you want to discard entire ppp
packets. Depending on exactly how the interface works it might be that
urb are likely to contain complete packets.

So discarding entire urb might work better than discarding a few bytes.
(But don't even think of scanning the data stream in the usb driver - except
for experiments.)

	David



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-11 12:05:55

On Fri, Jul 11, 2014 at 09:28:47AM +0000, David Laight wrote:
From: Olivier Sobrie Olivier Sobrie
quoted
Hi Alan and Davids,

On Thu, Jul 10, 2014 at 04:50:03PM +0100, One Thousand Gnomes wrote:
quoted
On Thu, 10 Jul 2014 14:37:37 +0000
David Laight [off-list ref] wrote:
quoted
From: Olivier Sobrie
...
quoted
The function put_rxbuf_data() is called from the urb completion handler.
It puts the data of the urb transfer in the tty buffer with
tty_insert_flip_string_flags() and schedules a work queue in order to
push the data to the ldisc.
Problem is that we are in a urb completion handler so we can't wait
until there is room in the tty buffer.
The tty provides the input queue, if the queue is full then just chuck
the data in the bitbucket.  hso is trying to be far too clever.

If hso is fast enough that the buffering isn't sufficient on the tty side
then we need to fix the tty buffer size.
Ok I'll adapt the patch to drop the data that can't be put in the tty
buffer. I test this and resend a new patch.
If you are going to drop data, then ideally you want to discard entire ppp
packets. Depending on exactly how the interface works it might be that
urb are likely to contain complete packets.
Indeed, urbs contains complete ppp packets. I see that the first and last
byte are equal to 0x7e.
So discarding entire urb might work better than discarding a few bytes.
(But don't even think of scanning the data stream in the usb driver - except
for experiments.)
I will check with tty_buffer_request_room() that there is enough room to
receive the frame before inserting the data in the tty buffer with
tty_insert_flip_string().

-- 
Olivier

[PATCH v2 2/2] hso: fix deadlock when receiving bursts of data

From: Olivier Sobrie <hidden>
Date: 2014-07-11 13:06:48

When the module sends bursts of data, sometimes a deadlock happens in
the hso driver when the tty buffer doesn't get the chance to be flushed
quickly enough.

Remove the endless while loop in function put_rxbuf_data() which is
called by the urb completion handler.
If there isn't enough room in the tty buffer, discards all the data
received in the URB.

Cc: David Miller <davem@davemloft.net>
Cc: David Laight <redacted>
Cc: One Thousand Gnomes <redacted>
Cc: Dan Williams <redacted>
Cc: Jan Dumon <redacted>
Signed-off-by: Olivier Sobrie <redacted>
---
Changes in v2:
 - remove the unthrottle timer added in the previous patch version
 - drop entire rx urb data if there is not enough space in tty buffer
 - remove variable curr_rx_urb_offset from hso_serial structure

 drivers/net/usb/hso.c |   38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 9ca2b41..a4272ed 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -258,7 +258,6 @@ struct hso_serial {
 	 * so as not to drop characters on the floor.
 	 */
 	int  curr_rx_urb_idx;
-	u16  curr_rx_urb_offset;
 	u8   rx_urb_filled[MAX_RX_URBS];
 	struct tasklet_struct unthrottle_tasklet;
 };
@@ -2001,8 +2000,7 @@ static void ctrl_callback(struct urb *urb)
 static int put_rxbuf_data(struct urb *urb, struct hso_serial *serial)
 {
 	struct tty_struct *tty;
-	int write_length_remaining = 0;
-	int curr_write_len;
+	int count;
 
 	/* Sanity check */
 	if (urb == NULL || serial == NULL) {
@@ -2012,29 +2010,28 @@ static int put_rxbuf_data(struct urb *urb, struct hso_serial *serial)
 
 	tty = tty_port_tty_get(&serial->port);
 
+	if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
+		tty_kref_put(tty);
+		return -1;
+	}
+
 	/* Push data to tty */
-	write_length_remaining = urb->actual_length -
-		serial->curr_rx_urb_offset;
 	D1("data to push to tty");
-	while (write_length_remaining) {
-		if (tty && test_bit(TTY_THROTTLED, &tty->flags)) {
-			tty_kref_put(tty);
-			return -1;
-		}
-		curr_write_len = tty_insert_flip_string(&serial->port,
-			urb->transfer_buffer + serial->curr_rx_urb_offset,
-			write_length_remaining);
-		serial->curr_rx_urb_offset += curr_write_len;
-		write_length_remaining -= curr_write_len;
+	count = tty_buffer_request_room(&serial->port, urb->actual_length);
+	if (count >= urb->actual_length) {
+		tty_insert_flip_string(&serial->port, urb->transfer_buffer,
+				       urb->actual_length);
 		tty_flip_buffer_push(&serial->port);
+	} else {
+		dev_warn(&serial->parent->usb->dev,
+			 "dropping data, %d bytes lost\n", urb->actual_length);
 	}
+
 	tty_kref_put(tty);
 
-	if (write_length_remaining == 0) {
-		serial->curr_rx_urb_offset = 0;
-		serial->rx_urb_filled[hso_urb_to_index(serial, urb)] = 0;
-	}
-	return write_length_remaining;
+	serial->rx_urb_filled[hso_urb_to_index(serial, urb)] = 0;
+
+	return 0;
 }
 
 
@@ -2205,7 +2202,6 @@ static int hso_stop_serial_device(struct hso_device *hso_dev)
 		}
 	}
 	serial->curr_rx_urb_idx = 0;
-	serial->curr_rx_urb_offset = 0;
 
 	if (serial->tx_urb)
 		usb_kill_urb(serial->tx_urb);
-- 
1.7.9.5

Re: [PATCH v2 2/2] hso: fix deadlock when receiving bursts of data

From: David Miller <davem@davemloft.net>
Date: 2014-07-11 18:36:55

When posting new versions of patches, you must resubmit the entire
series, not just the patch which is changing.

Thank you.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help