[PATCH 0/3] TTY improve n_gsm support

[PATCH 0/3] TTY improve n_gsm support

[Gregory CLEMENT]

Hello,

This series if patch improve n_gms support especially with TELIT
LE910. However the fix should benefit to any modem supporting cmux.

The first patch is just about improving debugging output.

The second one removes a tty optimization which make the LE910 hang.

The last one fixes an issue observed on the LE910 but should benefit
to all the modem. We observed that pretty quickly the transfer done
using the virtual tty were blocked. We found that it was due of a
wakeup to the real tty. Without this fix, the real tty wait for
indefinitely.

Gregory

Gregory CLEMENT (3):
  tty: n_gsm: Improve debug output
  tty: n_gsm: Fix SOF skipping
  tty: n_gsm: Fix waking up upper tty layer when room available

 drivers/tty/n_gsm.c | 59 +++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 24 deletions(-)

-- 
2.26.1

[PATCH 1/3] tty: n_gsm: Improve debug output

[Gregory CLEMENT]

Use appropriate print helpers for debug messages.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/tty/n_gsm.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index d77ed82a4840..4965e39e0223 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -459,7 +459,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
 	if (!(debug & 1))
 		return;
 
-	pr_info("%s %d) %c: ", hdr, addr, "RC"[cr]);
+	pr_debug("%s %d) %c: ", hdr, addr, "RC"[cr]);
 
 	switch (control & ~PF) {
 	case SABM:
@@ -504,18 +504,10 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
 	else
 		pr_cont("(F)");
 
-	if (dlen) {
-		int ct = 0;
-		while (dlen--) {
-			if (ct % 8 == 0) {
-				pr_cont("\n");
-				pr_debug("    ");
-			}
-			pr_cont("%02X ", *data++);
-			ct++;
-		}
-	}
-	pr_cont("\n");
+	if (dlen)
+		print_hex_dump_bytes("", DUMP_PREFIX_NONE, data, dlen);
+
+	pr_debug("\n");
 }
 
 
-- 
2.26.1

Re: [PATCH 1/3] tty: n_gsm: Improve debug output

[Jiri Slaby]

On 30. 04. 20, 13:34, Gregory CLEMENT wrote:
> Use appropriate print helpers for debug messages.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  drivers/tty/n_gsm.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index d77ed82a4840..4965e39e0223 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -459,7 +459,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
>  	if (!(debug & 1))
>  		return;
>  
> -	pr_info("%s %d) %c: ", hdr, addr, "RC"[cr]);
> +	pr_debug("%s %d) %c: ", hdr, addr, "RC"[cr]);

Now, you need both debug=1 module parameter *and* fiddling with
dynamic_debug, if enabled. And it is enabled in most distros…

We don't have any unconditional KERN_DEBUG printk helper, unfortunately.

>  	switch (control & ~PF) {
>  	case SABM:
> @@ -504,18 +504,10 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
>  	else
>  		pr_cont("(F)");
>  
> -	if (dlen) {
> -		int ct = 0;
> -		while (dlen--) {
> -			if (ct % 8 == 0) {
> -				pr_cont("\n");
> -				pr_debug("    ");
> -			}
> -			pr_cont("%02X ", *data++);
> -			ct++;
> -		}
> -	}
> -	pr_cont("\n");
> +	if (dlen)

print_hex_dump* handle zero len quite well. No need for the if.

> +		print_hex_dump_bytes("", DUMP_PREFIX_NONE, data, dlen);
> +
> +	pr_debug("\n");

This is superfluous too. It was intended as the last \n in the previous
code.

thanks,
-- 
js
suse labs

[PATCH 2/3] tty: n_gsm: Fix SOF skipping

[Gregory CLEMENT]

For at least some modems like the TELIT LE910, skipping SOF makes
transfers blocking indefinitely after a short amount of data
transferred.

Given the small improvement provided by skipping the SOF (just one
byte on about 100 bytes), it seems better to completely remove this
"feature" than make it optional.

Fixes: 96fd7ce58ffb ("TTY: create drivers/tty and move the tty core files there")
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/tty/n_gsm.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4965e39e0223..58950b33e5ac 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -669,7 +669,6 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 {
 	struct gsm_msg *msg, *nmsg;
 	int len;
-	int skip_sof = 0;
 
 	list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
 		if (gsm->constipated && msg->addr)
@@ -691,15 +690,10 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 			print_hex_dump_bytes("gsm_data_kick: ",
 					     DUMP_PREFIX_OFFSET,
 					     gsm->txframe, len);
-
-		if (gsm->output(gsm, gsm->txframe + skip_sof,
-						len - skip_sof) < 0)
+		if (gsm->output(gsm, gsm->txframe, len) < 0)
 			break;
 		/* FIXME: Can eliminate one SOF in many more cases */
 		gsm->tx_bytes -= msg->len;
-		/* For a burst of frames skip the extra SOF within the
-		   burst */
-		skip_sof = 1;
 
 		list_del(&msg->list);
 		kfree(msg);
-- 
2.26.1

Re: [PATCH 2/3] tty: n_gsm: Fix SOF skipping

[Jiri Slaby]

On 30. 04. 20, 13:34, Gregory CLEMENT wrote:
> For at least some modems like the TELIT LE910, skipping SOF makes
> transfers blocking indefinitely after a short amount of data
> transferred.
> 
> Given the small improvement provided by skipping the SOF (just one
> byte on about 100 bytes), it seems better to completely remove this
> "feature" than make it optional.
> 
> Fixes: 96fd7ce58ffb ("TTY: create drivers/tty and move the tty core files there")

Again, this is unlikely a correct "fixes" commit.

thanks,
-- 
js
suse labs

[PATCH 3/3] tty: n_gsm: Fixe waking up upper tty layer when room available

[Gregory CLEMENT]

Warn the upper layer when n_gms is ready to receive data
again. Without this the associated virtual tty remain blocked
indefinitely.

Fixes: 96fd7ce58ffb ("TTY: create drivers/tty and move the tty core files there")
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/tty/n_gsm.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 58950b33e5ac..4ff2b981aa7e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -665,10 +665,12 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
  *	FIXME: lock against link layer control transmissions
  */
 
-static void gsm_data_kick(struct gsm_mux *gsm)
+static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 {
 	struct gsm_msg *msg, *nmsg;
 	int len;
+	struct tty_struct *tty_dlci = NULL;
+
 
 	list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
 		if (gsm->constipated && msg->addr)
@@ -697,6 +699,29 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 
 		list_del(&msg->list);
 		kfree(msg);
+
+		if (dlci) {
+			tty_dlci = tty_port_tty_get(&dlci->port);
+			if (tty_dlci)
+				tty_wakeup(tty_dlci);
+		} else {
+			int i = 0;
+
+			while (i < NUM_DLCI) {
+				struct gsm_dlci *dlci;
+
+				dlci = gsm->dlci[i];
+				if (dlci == NULL) {
+					i++;
+					continue;
+				}
+
+				tty_dlci = tty_port_tty_get(&dlci->port);
+				if (tty_dlci)
+					tty_wakeup(tty_dlci);
+				i++;
+			}
+		}
 	}
 }
 
@@ -748,7 +773,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 	/* Add to the actual output queue */
 	list_add_tail(&msg->list, &gsm->tx_list);
 	gsm->tx_bytes += msg->len;
-	gsm_data_kick(gsm);
+	gsm_data_kick(gsm, dlci);
 }
 
 /**
@@ -1209,7 +1234,7 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 		gsm_control_reply(gsm, CMD_FCON, NULL, 0);
 		/* Kick the link in case it is idling */
 		spin_lock_irqsave(&gsm->tx_lock, flags);
-		gsm_data_kick(gsm);
+		gsm_data_kick(gsm, NULL);
 		spin_unlock_irqrestore(&gsm->tx_lock, flags);
 		break;
 	case CMD_FCOFF:
@@ -2531,7 +2556,7 @@ static void gsmld_write_wakeup(struct tty_struct *tty)
 	/* Queue poll */
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	spin_lock_irqsave(&gsm->tx_lock, flags);
-	gsm_data_kick(gsm);
+	gsm_data_kick(gsm, NULL);
 	if (gsm->tx_bytes < TX_THRESH_LO) {
 		gsm_dlci_data_sweep(gsm);
 	}
-- 
2.26.1

[PATCH 3/3] tty: n_gsm: Fix waking up upper tty layer when room available

[Gregory CLEMENT]

Warn the upper layer when n_gms is ready to receive data
again. Without this the associated virtual tty remain blocked
indefinitely.

Fixes: 96fd7ce58ffb ("TTY: create drivers/tty and move the tty core files there")
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/tty/n_gsm.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 58950b33e5ac..4ff2b981aa7e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -665,10 +665,12 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
  *	FIXME: lock against link layer control transmissions
  */
 
-static void gsm_data_kick(struct gsm_mux *gsm)
+static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 {
 	struct gsm_msg *msg, *nmsg;
 	int len;
+	struct tty_struct *tty_dlci = NULL;
+
 
 	list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
 		if (gsm->constipated && msg->addr)
@@ -697,6 +699,29 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 
 		list_del(&msg->list);
 		kfree(msg);
+
+		if (dlci) {
+			tty_dlci = tty_port_tty_get(&dlci->port);
+			if (tty_dlci)
+				tty_wakeup(tty_dlci);
+		} else {
+			int i = 0;
+
+			while (i < NUM_DLCI) {
+				struct gsm_dlci *dlci;
+
+				dlci = gsm->dlci[i];
+				if (dlci == NULL) {
+					i++;
+					continue;
+				}
+
+				tty_dlci = tty_port_tty_get(&dlci->port);
+				if (tty_dlci)
+					tty_wakeup(tty_dlci);
+				i++;
+			}
+		}
 	}
 }
 
@@ -748,7 +773,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 	/* Add to the actual output queue */
 	list_add_tail(&msg->list, &gsm->tx_list);
 	gsm->tx_bytes += msg->len;
-	gsm_data_kick(gsm);
+	gsm_data_kick(gsm, dlci);
 }
 
 /**
@@ -1209,7 +1234,7 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 		gsm_control_reply(gsm, CMD_FCON, NULL, 0);
 		/* Kick the link in case it is idling */
 		spin_lock_irqsave(&gsm->tx_lock, flags);
-		gsm_data_kick(gsm);
+		gsm_data_kick(gsm, NULL);
 		spin_unlock_irqrestore(&gsm->tx_lock, flags);
 		break;
 	case CMD_FCOFF:
@@ -2531,7 +2556,7 @@ static void gsmld_write_wakeup(struct tty_struct *tty)
 	/* Queue poll */
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	spin_lock_irqsave(&gsm->tx_lock, flags);
-	gsm_data_kick(gsm);
+	gsm_data_kick(gsm, NULL);
 	if (gsm->tx_bytes < TX_THRESH_LO) {
 		gsm_dlci_data_sweep(gsm);
 	}
-- 
2.26.1

Re: [PATCH 3/3] tty: n_gsm: Fix waking up upper tty layer when room available

[Jiri Slaby]

On 30. 04. 20, 13:34, Gregory CLEMENT wrote:
> Warn the upper layer when n_gms is ready to receive data
> again. Without this the associated virtual tty remain blocked

s/remain/&s/

> indefinitely.
> 
> Fixes: 96fd7ce58ffb ("TTY: create drivers/tty and move the tty core files there")

This looks invalid. Did you use git blame? Or git log, with --follow -M?

> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  drivers/tty/n_gsm.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 58950b33e5ac..4ff2b981aa7e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -665,10 +665,12 @@ static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
>   *	FIXME: lock against link layer control transmissions
>   */
>  
> -static void gsm_data_kick(struct gsm_mux *gsm)
> +static void gsm_data_kick(struct gsm_mux *gsm, struct gsm_dlci *dlci)
>  {
>  	struct gsm_msg *msg, *nmsg;
>  	int len;
> +	struct tty_struct *tty_dlci = NULL;
> +
>  
>  	list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
>  		if (gsm->constipated && msg->addr)
> @@ -697,6 +699,29 @@ static void gsm_data_kick(struct gsm_mux *gsm)
>  
>  		list_del(&msg->list);
>  		kfree(msg);
> +
> +		if (dlci) {
> +			tty_dlci = tty_port_tty_get(&dlci->port);
> +			if (tty_dlci)
> +				tty_wakeup(tty_dlci);

No tty_port_put looks wrong to me?

Why not to use tty_port_tty_wakeup?

> +		} else {
> +			int i = 0;
> +
> +			while (i < NUM_DLCI) {

Hmm, feels like 'for' loop fits better here.

> +				struct gsm_dlci *dlci;
> +
> +				dlci = gsm->dlci[i];
> +				if (dlci == NULL) {
> +					i++;
> +					continue;
> +				}
> +
> +				tty_dlci = tty_port_tty_get(&dlci->port);
> +				if (tty_dlci)
> +					tty_wakeup(tty_dlci);

Dtto

> +				i++;
> +			}
> +		}
>  	}
>  }
>  
> @@ -748,7 +773,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
>  	/* Add to the actual output queue */
>  	list_add_tail(&msg->list, &gsm->tx_list);
>  	gsm->tx_bytes += msg->len;
> -	gsm_data_kick(gsm);
> +	gsm_data_kick(gsm, dlci);

thanks,
-- 
js
suse labs