All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case)
@ 2011-09-23 15:22 Mikhail Kshevetskiy
  2011-09-23 15:22 ` [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output Mikhail Kshevetskiy
  2011-09-27 10:58 ` [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Alan Cox
  0 siblings, 2 replies; 4+ messages in thread
From: Mikhail Kshevetskiy @ 2011-09-23 15:22 UTC (permalink / raw)
  To: linux-serial; +Cc: gregkh, alan, linux-kernel

in adaption=2 case we should put 1 or 2 byte with modem status bits
at the beginning of a buffer pointed by "dp". n_gsm use 1 byte case,
so it allocate a buffer of len + 1 size. As result we should:
  * put 1 byte of modem status bits
  * increase data pointer
  * put "len" bytes of data
but actually we have:
  * increase first byte with the value of modem status bits
  * decrease "len"
  * put orig_len - 1 bytes of data starting from the buffer beggining
This is evidently wrong.

Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com>
---
 drivers/tty/n_gsm.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 8a50e4e..143dc5e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -834,8 +834,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 		break;
 	case 2:	/* Unstructed with modem bits. Always one byte as we never
 		   send inline break data */
-		*dp += gsm_encode_modem(dlci);
-		len--;
+		*dp++ = gsm_encode_modem(dlci);
 		break;
 	}
 	WARN_ON(kfifo_out_locked(dlci->fifo, dp , len, &dlci->lock) != len);
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output
  2011-09-23 15:22 [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Mikhail Kshevetskiy
@ 2011-09-23 15:22 ` Mikhail Kshevetskiy
  2011-09-27 10:59   ` Alan Cox
  2011-09-27 10:58 ` [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Alan Cox
  1 sibling, 1 reply; 4+ messages in thread
From: Mikhail Kshevetskiy @ 2011-09-23 15:22 UTC (permalink / raw)
  To: linux-serial; +Cc: gregkh, alan, linux-kernel

n_gsm use a simple approach: every writing to fifo correspond exactly one
reading from fifo. There are no problem in this approach until we read
less bytes then we write. As result fifo may owerflow. This leads to packet
loss and very slow responce.

For example, this happens with ping packets (about 96 byte each) and default
gsm->mtu = 64. As result we get 50 sec ping timeout and 20% packet loss.

Fix the problem by reading and sending all data from the fifo

Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com>
---
 drivers/tty/n_gsm.c |   58 +++++++++++++++++++++++++++-----------------------
 1 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 143dc5e..be77fb5 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -810,37 +810,41 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 {
 	struct gsm_msg *msg;
 	u8 *dp;
-	int len, size;
+	int len, total_size, size;
 	int h = dlci->adaption - 1;
 
-	len = kfifo_len(dlci->fifo);
-	if (len == 0)
-		return 0;
-
-	/* MTU/MRU count only the data bits */
-	if (len > gsm->mtu)
-		len = gsm->mtu;
-
-	size = len + h;
-
-	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
-	/* FIXME: need a timer or something to kick this so it can't
-	   get stuck with no work outstanding and no buffer free */
-	if (msg == NULL)
-		return -ENOMEM;
-	dp = msg->data;
-	switch (dlci->adaption) {
-	case 1:	/* Unstructured */
-		break;
-	case 2:	/* Unstructed with modem bits. Always one byte as we never
-		   send inline break data */
-		*dp++ = gsm_encode_modem(dlci);
-		break;
+	total_size = 0;
+	while(1) {
+		len = kfifo_len(dlci->fifo);
+		if (len == 0)
+			return total_size;
+
+		/* MTU/MRU count only the data bits */
+		if (len > gsm->mtu)
+			len = gsm->mtu;
+
+		size = len + h;
+
+		msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+		/* FIXME: need a timer or something to kick this so it can't
+		   get stuck with no work outstanding and no buffer free */
+		if (msg == NULL)
+			return -ENOMEM;
+		dp = msg->data;
+		switch (dlci->adaption) {
+		case 1:	/* Unstructured */
+			break;
+		case 2:	/* Unstructed with modem bits. Always one byte as we never
+			   send inline break data */
+			*dp++ = gsm_encode_modem(dlci);
+			break;
+		}
+		WARN_ON(kfifo_out_locked(dlci->fifo, dp , len, &dlci->lock) != len);
+		__gsm_data_queue(dlci, msg);
+		total_size += size;
 	}
-	WARN_ON(kfifo_out_locked(dlci->fifo, dp , len, &dlci->lock) != len);
-	__gsm_data_queue(dlci, msg);
 	/* Bytes of data we used up */
-	return size;
+	return total_size;
 }
 
 /**
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case)
  2011-09-23 15:22 [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Mikhail Kshevetskiy
  2011-09-23 15:22 ` [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output Mikhail Kshevetskiy
@ 2011-09-27 10:58 ` Alan Cox
  1 sibling, 0 replies; 4+ messages in thread
From: Alan Cox @ 2011-09-27 10:58 UTC (permalink / raw)
  To: Mikhail Kshevetskiy; +Cc: linux-serial, gregkh, linux-kernel

On Fri, 23 Sep 2011 19:22:55 +0400
Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:

> in adaption=2 case we should put 1 or 2 byte with modem status bits
> at the beginning of a buffer pointed by "dp". n_gsm use 1 byte case,
> so it allocate a buffer of len + 1 size. As result we should:
>   * put 1 byte of modem status bits
>   * increase data pointer
>   * put "len" bytes of data
> but actually we have:
>   * increase first byte with the value of modem status bits
>   * decrease "len"
>   * put orig_len - 1 bytes of data starting from the buffer beggining
> This is evidently wrong.
> 
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com>

Acked-by: Alan Cox <alan@linux.intel.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output
  2011-09-23 15:22 ` [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output Mikhail Kshevetskiy
@ 2011-09-27 10:59   ` Alan Cox
  0 siblings, 0 replies; 4+ messages in thread
From: Alan Cox @ 2011-09-27 10:59 UTC (permalink / raw)
  To: Mikhail Kshevetskiy; +Cc: linux-serial, gregkh, linux-kernel

On Fri, 23 Sep 2011 19:22:56 +0400
Mikhail Kshevetskiy <mikhail.kshevetskiy@gmail.com> wrote:

> n_gsm use a simple approach: every writing to fifo correspond exactly
> one reading from fifo. There are no problem in this approach until we
> read less bytes then we write. As result fifo may owerflow. This
> leads to packet loss and very slow responce.

We do need to do something here but your change means a single DLCI
will hog the channel, so I think the while loop needs to be one layer
up.

Alan

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2011-09-27 10:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-23 15:22 [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Mikhail Kshevetskiy
2011-09-23 15:22 ` [PATCH 2/2] tty/n_gsm: avoid fifo overflow in gsm_dlci_data_output Mikhail Kshevetskiy
2011-09-27 10:59   ` Alan Cox
2011-09-27 10:58 ` [PATCH 1/2] tty/n_gsm: fix a bug in gsm_dlci_data_output (adaption = 2 case) Alan Cox

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.