The workqueue "retry_unthrottle_workqueue" is not scheduled anywhere
in the code. So, remove it.
Signed-off-by: Olivier Sobrie <[email protected]>
---
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
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 <[email protected]>
---
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
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
> > 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: Olivier Sobrie
> Hi David,
>
> On Mon, Jul 07, 2014 at 09:13:53AM +0000, David Laight wrote:
> > 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.
>
> 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
> > Hi David,
> >
> > On Mon, Jul 07, 2014 at 09:13:53AM +0000, David Laight wrote:
> > > 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.
> >
> > 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
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
> Signed-off-by: Olivier Sobrie <[email protected]>
> ---
> 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) {
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:
> > 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
From: Olivier Sobrie <[email protected]>
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 <[email protected]>
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.
Hi David,
On Tue, Jul 08, 2014 at 04:16:33PM -0700, David Miller wrote:
> From: Olivier Sobrie <[email protected]>
> 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 <[email protected]>
>
> 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: 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
On Thu, 10 Jul 2014 14:37:37 +0000
David Laight <[email protected]> wrote:
> 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.
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: One Thousand Gnomes
> On Thu, 10 Jul 2014 14:37:37 +0000
> David Laight <[email protected]> wrote:
>
> > 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.
>
> 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
> 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 <[email protected]> wrote:
>
> > 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.
>
> 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: Olivier Sobrie Olivier Sobrie
> 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 <[email protected]> wrote:
> >
> > > 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.
> >
> > 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
On Fri, Jul 11, 2014 at 09:28:47AM +0000, David Laight wrote:
> From: Olivier Sobrie Olivier Sobrie
> > 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 <[email protected]> wrote:
> > >
> > > > 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.
> > >
> > > 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 <[email protected]>
Cc: David Laight <[email protected]>
Cc: One Thousand Gnomes <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Jan Dumon <[email protected]>
Signed-off-by: Olivier Sobrie <[email protected]>
---
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
When posting new versions of patches, you must resubmit the entire
series, not just the patch which is changing.
Thank you.