RE: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled links

RE: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled links

[Starke, Daniel]

> > From: Daniel Starke <daniel.starke@siemens.com>
> > 
> > The current implementation does not handle the situation that no data 
> > is in the internal queue and needs to be sent out while the user tty 
> > fifo is full.
> > Add a timer that moves more data from user tty down to the internal 
> > queue which is then serialized on the ldisc. This timer is triggered 
> > if no data was moved from a user tty to the internal queue within 10 * T1.
> > 
> > Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >   drivers/tty/n_gsm.c | 43 +++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 35 insertions(+), 8 deletions(-)
> > 
> > See patch 6 regarding changes since to v1.
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c index 
> > 0a9924445968..3a4a2394d970 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> ...
> > @@ -833,6 +834,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
> >   	list_add_tail(&msg->list, &gsm->tx_list);
> >   	gsm->tx_bytes += msg->len;
> >   	gsm_data_kick(gsm, dlci);
> > +	mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100);
> 
> The formula deserves an explanation. And why 10 * X / 100, and not X / 10?

T1 is defined as 1/100th of a second (see chapter 5.7.1 of the standard).
Therefore, it is gsm->t1 * HZ / 100. I chose 10x T1 as this case should
usually not occur and only acts as a final countermeasure against a
stalled link. Or are there any other suggestions for a proper kick timer
value?

> > @@ -1062,9 +1058,9 @@ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci,
> >    *	renegotiate DLCI priorities with optional stuff. Needs optimising.
> >    */
> >   
> > -static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
> > +static int gsm_dlci_data_sweep(struct gsm_mux *gsm)
> >   {
> > -	int len;
> > +	int len, ret = 0;
> 
> Why is ret signed?

Many obviously only unsigned values are signed in the code of the original
author. I simply aligned my code to this to believe that int is preferred.
But I can change it to unsigned int if this is preferred here?

Best regards,
Daniel Starke

Re: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled links

[Jiri Slaby]

On 19. 05. 22, 9:07, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The current implementation does not handle the situation that no data is in
> the internal queue and needs to be sent out while the user tty fifo is
> full.
> Add a timer that moves more data from user tty down to the internal queue
> which is then serialized on the ldisc. This timer is triggered if no data
> was moved from a user tty to the internal queue within 10 * T1.
> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>   drivers/tty/n_gsm.c | 43 +++++++++++++++++++++++++++++++++++--------
>   1 file changed, 35 insertions(+), 8 deletions(-)
> 
> See patch 6 regarding changes since to v1.
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 0a9924445968..3a4a2394d970 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
...
> @@ -833,6 +834,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
>   	list_add_tail(&msg->list, &gsm->tx_list);
>   	gsm->tx_bytes += msg->len;
>   	gsm_data_kick(gsm, dlci);
> +	mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100);

The formula deserves an explanation. And why 10 * X / 100, and not X / 10?

> @@ -1062,9 +1058,9 @@ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci,
>    *	renegotiate DLCI priorities with optional stuff. Needs optimising.
>    */
>   
> -static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
> +static int gsm_dlci_data_sweep(struct gsm_mux *gsm)
>   {
> -	int len;
> +	int len, ret = 0;

Why is ret signed?

>   	/* Priority ordering: We should do priority with RR of the groups */
>   	int i = 1;
>   
> @@ -1087,7 +1083,11 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
>   		/* DLCI empty - try the next */
>   		if (len == 0)
>   			i++;
> +		else
> +			ret++;
>   	}
> +
> +	return ret;
>   }
>   
>   /**

thanks,
-- 
js
suse labs

[PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled links

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The current implementation does not handle the situation that no data is in
the internal queue and needs to be sent out while the user tty fifo is
full.
Add a timer that moves more data from user tty down to the internal queue
which is then serialized on the ldisc. This timer is triggered if no data
was moved from a user tty to the internal queue within 10 * T1.

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

See patch 6 regarding changes since to v1.

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 0a9924445968..3a4a2394d970 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -244,6 +244,7 @@ struct gsm_mux {
 	struct list_head tx_list;	/* Pending data packets */
 
 	/* Control messages */
+	struct timer_list kick_timer;	/* Kick TX queuing on timeout */
 	struct timer_list t2_timer;	/* Retransmit timer for commands */
 	int cretries;			/* Command retry counter */
 	struct gsm_control *pending_cmd;/* Our current pending command */
@@ -833,6 +834,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 	list_add_tail(&msg->list, &gsm->tx_list);
 	gsm->tx_bytes += msg->len;
 	gsm_data_kick(gsm, dlci);
+	mod_timer(&gsm->kick_timer, jiffies + 10 * gsm->t1 * HZ / 100);
 }
 
 /**
@@ -885,9 +887,6 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 	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)
 		return -ENOMEM;
 	dp = msg->data;
@@ -964,9 +963,6 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
 
 	size = len + overhead;
 	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) {
 		skb_queue_tail(&dlci->skb_list, dlci->skb);
 		dlci->skb = NULL;
@@ -1062,9 +1058,9 @@ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci,
  *	renegotiate DLCI priorities with optional stuff. Needs optimising.
  */
 
-static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
+static int gsm_dlci_data_sweep(struct gsm_mux *gsm)
 {
-	int len;
+	int len, ret = 0;
 	/* Priority ordering: We should do priority with RR of the groups */
 	int i = 1;
 
@@ -1087,7 +1083,11 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 		/* DLCI empty - try the next */
 		if (len == 0)
 			i++;
+		else
+			ret++;
 	}
+
+	return ret;
 }
 
 /**
@@ -1806,6 +1806,30 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len)
 	}
 }
 
+/**
+ *	gsm_kick_timer	-	transmit if possible
+ *	@t: timer contained in our gsm object
+ *
+ *	Transmit data from DLCIs if the queue is empty. We can't rely on
+ *	a tty wakeup except when we filled the pipe so we need to fire off
+ *	new data ourselves in other cases.
+ */
+static void gsm_kick_timer(struct timer_list *t)
+{
+	struct gsm_mux *gsm = from_timer(gsm, t, kick_timer);
+	unsigned long flags;
+	int sent = 0;
+
+	spin_lock_irqsave(&gsm->tx_lock, flags);
+	/* If we have nothing running then we need to fire up */
+	if (gsm->tx_bytes < TX_THRESH_LO)
+		sent = gsm_dlci_data_sweep(gsm);
+	spin_unlock_irqrestore(&gsm->tx_lock, flags);
+
+	if (sent && debug & 4)
+		pr_info("%s TX queue stalled\n", __func__);
+}
+
 /*
  *	Allocate/Free DLCI channels
  */
@@ -2261,6 +2285,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
 	}
 
 	/* Finish outstanding timers, making sure they are done */
+	del_timer_sync(&gsm->kick_timer);
 	del_timer_sync(&gsm->t2_timer);
 
 	/* Free up any link layer users and finally the control channel */
@@ -2293,6 +2318,7 @@ static int gsm_activate_mux(struct gsm_mux *gsm)
 	struct gsm_dlci *dlci;
 	int ret;
 
+	timer_setup(&gsm->kick_timer, gsm_kick_timer, 0);
 	timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
 	init_waitqueue_head(&gsm->event);
 	spin_lock_init(&gsm->control_lock);
@@ -2699,6 +2725,7 @@ static int gsmld_open(struct tty_struct *tty)
 
 	gsmld_attach_gsm(tty, gsm);
 
+	timer_setup(&gsm->kick_timer, gsm_kick_timer, 0);
 	timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
 
 	return 0;
-- 
2.34.1