From: Daniel Starke <[email protected]>
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: [email protected]
Signed-off-by: Daniel Starke <[email protected]>
---
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
On 19. 05. 22, 9:07, D. Starke wrote:
> From: Daniel Starke <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Daniel Starke <[email protected]>
> ---
> 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
> > From: Daniel Starke <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Daniel Starke <[email protected]>
> > ---
> > 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