tty: n_gsm: improve software flowcontrol
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
@@ -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
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.
Signed-off-by Sander Bosma <[email protected]>--- drivers/tty/n_gsm.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-)drivers/tty/n_gsm.c +0
-0
...
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
-0while (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
-1if (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