Skip to contentSkip to conversation

tty: n_gsm: improve software flowcontrol

PATCH In review Sander Bosma 2 participants 2 messages Jun 20, 2013 – Jun 21, 2013 TTY layer and serial drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tty: n_gsm: improve software flowcontrol

When a channel got constipated, all messages in the messagequeue were still sent to the device. This can cause a bufferoverflow in some devices such as the SIM900 modem. This patch removes all pending messages belonging to the channel from the messagequeue when a channel gets constipated. Those messages are stored in a list within the DLCI. Once the channel is no longer constipated the stored messages are moved back into the global messagequeue. This significantly decreases the chance of bufferoverflows happening on receiving devices.

---
 drivers/tty/n_gsm.c |   44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)
drivers/tty/n_gsm.c +40 -4
index 4a43ef5d7..0cad08fa 100644
@@ -152,6 +152,7 @@ struct gsm_dlci {
 	/* Flow control */
 	int throttled;		/* Private copy of throttle state */
 	int constipated;	/* Throttle status for outgoing */
+	struct list_head constipated_list; /* msg's removed from tx queue */
 	/* Packetised I/O */
 	struct sk_buff *skb;	/* Frame being sent */
 	struct sk_buff_head skb_list;	/* Queued frames */
@@ -684,7 +685,8 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 	int skip_sof = 0;
 
 	list_for_each_entry_safe(msg, nmsg, &gsm->tx_list, list) {
-		if (gsm->constipated && msg->addr)
+		if (gsm->constipated && msg->addr ||
+		    gsm->dlci[msg->addr]->constipated)
 			continue;
 		if (gsm->encoding != 0) {
 			gsm->txframe[0] = GSM1_SOF;
@@ -931,7 +933,7 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 	int len;
 	/* Priority ordering: We should do priority with RR of the groups */
 	int i = 1;
-
+	struct tty_struct *tty;
 	while (i < NUM_DLCI) {
 		struct gsm_dlci *dlci;
 
@@ -946,6 +948,13 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 			len = gsm_dlci_data_output(gsm, dlci);
 		else
 			len = gsm_dlci_data_output_framed(gsm, dlci);
+
+		if (len >= 0) {
+			tty = tty_port_tty_get(&dlci->port);
+			if (tty)
+				tty_wakeup(dlci->port.tty);
+		}
+
 		if (len < 0)
 			break;
 		/* DLCI empty - try the next */
@@ -1029,7 +1038,8 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	int  mlines = 0;
 	u8 brk = 0;
 	int fc;
-
+	struct gsm_msg *msg, *nmsg;
+	unsigned long flags;
 	/* The modem status command can either contain one octet (v.24 signals)
 	   or two octets (v.24 signals + break signals). The length field will
 	   either be 2 or 3 respectively. This is specified in section
@@ -1047,8 +1057,27 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	if (fc && !dlci->constipated) {
 		/* Need to throttle our output on this device */
 		dlci->constipated = 1;
+		spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
+		list_for_each_entry_safe(msg, nmsg, &dlci->gsm->tx_list, list) {
+			list_del(&msg->list);	/* remove from msgqueue */
+			/* add to list of messages which could not be sent
+			   because of constipation */
+			list_add_tail(&msg->list, &dlci->constipated_list);
+			dlci->gsm->tx_bytes -= msg->len;
+		}
+		spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
+
 	} else if (!fc && dlci->constipated) {
 		dlci->constipated = 0;
+		spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
+		list_for_each_entry_safe(msg, nmsg, &dlci->constipated_list,
+					 list) {
+			list_del(&msg->list);	/* remove from msgqueue */
+			/* add to global messagequeue */
+			list_add_tail(&msg->list,  &dlci->gsm->tx_list);
+			dlci->gsm->tx_bytes += msg->len;
+		}
+		spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
 		gsm_dlci_data_kick(dlci);
 	}
 
@@ -1640,6 +1669,7 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 		kfree(dlci);
 		return NULL;
 	}
+	INIT_LIST_HEAD(&dlci->constipated_list);
 
 	skb_queue_head_init(&dlci->skb_list);
 	init_timer(&dlci->t1);
@@ -2956,7 +2986,7 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	struct gsm_mux *gsm;
-
+	struct gsm_msg *msg, *nmsg;
 	if (dlci == NULL)
 		return;
 	if (dlci->state == DLCI_CLOSED)
@@ -2965,6 +2995,12 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	gsm_destroy_network(dlci);
 	mutex_unlock(&dlci->mutex);
 	gsm = dlci->gsm;
+
+	list_for_each_entry_safe(msg, nmsg, &dlci->constipated_list, list) {
+		list_del(&msg->list);
+		kfree(msg);
+	}
+
 	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
 		goto out;
 	gsm_dlci_begin_close(dlci);
Signature · 1 line
1.7.9.5
#1 lore raw

Hi,

Quoted 19 lines · Sander Bosma

When a channel got constipated, all messages in the messagequeue were still sent to the device. This can cause a bufferoverflow in some devices such as the SIM900 modem. This patch removes all pending messages belonging to the channel from the messagequeue when a channel gets constipated. Those messages are stored in a list within the DLCI. Once the channel is no longer constipated the stored messages are moved back into the global messagequeue. This significantly decreases the chance of bufferoverflows happening on receiving devices.

---
 drivers/tty/n_gsm.c |   44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)
drivers/tty/n_gsm.c +0 -0
index 4a43ef5d7..0cad08fa 100644

...

drivers/tty/n_gsm.c static void gsm_dlci_data_sweep(struct gsm_mux *gsm) · line 936

Quoted 6 lines
drivers/tty/n_gsm.c +1 -1
@@ -931,7 +933,7 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 	int len;
 	/* Priority ordering: We should do priority with RR of the groups */
 	int i = 1;
-
+	struct tty_struct *tty;

Why did you remove the blank line here?

drivers/tty/n_gsm.c static void gsm_dlci_data_sweep(struct gsm_mux *gsm) · line 956

Quoted 13 lines
drivers/tty/n_gsm.c +6 -0
 	while (i < NUM_DLCI) {
 		struct gsm_dlci *dlci;
 
@@ -946,6 +948,13 @@ static void gsm_dlci_data_sweep(struct gsm_mux *gsm)
 			len = gsm_dlci_data_output(gsm, dlci);
 		else
 			len = gsm_dlci_data_output_framed(gsm, dlci);
+
+		if (len >= 0) {
+			tty = tty_port_tty_get(&dlci->port);
+			if (tty)
+				tty_wakeup(dlci->port.tty);
+		}

This is incorrect. You leak a tty. And this should be tty_port_tty_wakeup anyway.

But please also document addition of wakeup in the commit log.

drivers/tty/n_gsm.c static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci, · line 1042

Quoted 10 lines
drivers/tty/n_gsm.c +2 -1
 		if (len < 0)
 			break;
 		/* DLCI empty - try the next */
@@ -1029,7 +1038,8 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	int  mlines = 0;
 	u8 brk = 0;
 	int fc;
-
+	struct gsm_msg *msg, *nmsg;
+	unsigned long flags;

Again removed blank line?

drivers/tty/n_gsm.c static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci, · line 1065

Quoted 13 lines
drivers/tty/n_gsm.c +6 -0
 	/* The modem status command can either contain one octet (v.24 signals)
 	   or two octets (v.24 signals + break signals). The length field will
 	   either be 2 or 3 respectively. This is specified in section
@@ -1047,8 +1057,27 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	if (fc && !dlci->constipated) {
 		/* Need to throttle our output on this device */
 		dlci->constipated = 1;
+		spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
+		list_for_each_entry_safe(msg, nmsg, &dlci->gsm->tx_list, list) {
+			list_del(&msg->list);	/* remove from msgqueue */
+			/* add to list of messages which could not be sent
+			   because of constipation */
+			list_add_tail(&msg->list, &dlci->constipated_list);

list_move_tail...

drivers/tty/n_gsm.c static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci, · line 1077

Quoted 12 lines
drivers/tty/n_gsm.c +10 -0
+			dlci->gsm->tx_bytes -= msg->len;
+		}
+		spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
+
 	} else if (!fc && dlci->constipated) {
 		dlci->constipated = 0;
+		spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
+		list_for_each_entry_safe(msg, nmsg, &dlci->constipated_list,
+					 list) {
+			list_del(&msg->list);	/* remove from msgqueue */
+			/* add to global messagequeue */
+			list_add_tail(&msg->list,  &dlci->gsm->tx_list);

Here too.

Quoted 6 lines
drivers/tty/n_gsm.c +3 -0
+			dlci->gsm->tx_bytes += msg->len;
+		}
+		spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
 		gsm_dlci_data_kick(dlci);
 	}
 

...

drivers/tty/n_gsm.c static void gsmtty_close(struct tty_struct *tty, struct file *filp) · line 2989

Quoted 6 lines
drivers/tty/n_gsm.c +1 -1
@@ -2956,7 +2986,7 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	struct gsm_mux *gsm;
-
+	struct gsm_msg *msg, *nmsg;

blank.

thanks,

Signature · 2 lines
js
suse labs