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(-)
@@ -1252,14 +1251,6 @@ static void hso_unthrottle(struct tty_struct *tty)tasklet_hi_schedule(&serial->unthrottle_tasklet);}-staticvoidhso_unthrottle_workfunc(structwork_struct*work)-{-structhso_serial*serial=-container_of(work,structhso_serial,-retry_unthrottle_workqueue);-hso_unthrottle_tasklet(serial);-}-/* open the requested serial port */staticinthso_serial_open(structtty_struct*tty,structfile*filp){
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(-)
@@ -1251,6 +1259,11 @@ static void hso_unthrottle(struct tty_struct *tty)tasklet_hi_schedule(&serial->unthrottle_tasklet);}+staticvoidhso_unthrottle_schedule(unsignedlongdata)+{+tasklet_schedule((structtasklet_struct*)data);+}+/* open the requested serial port */staticinthso_serial_open(structtty_struct*tty,structfile*filp){
@@ -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){
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
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
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
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
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
@@ -1251,6 +1259,11 @@ static void hso_unthrottle(struct tty_struct *tty)tasklet_hi_schedule(&serial->unthrottle_tasklet);}+staticvoidhso_unthrottle_schedule(unsignedlongdata)+{+tasklet_schedule((structtasklet_struct*)data);+}+/* open the requested serial port */staticinthso_serial_open(structtty_struct*tty,structfile*filp){
@@ -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){
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
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.
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
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
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
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
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
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
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
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
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(-)
@@ -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;-}-returnwrite_length_remaining;+serial->rx_urb_filled[hso_urb_to_index(serial,urb)]=0;++return0;}
@@ -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);