Re: [PATCH] tty: n_gsm: improve software flowcontrol

[Jiri Slaby <jslaby@suse.cz>]

Hi,

On 06/20/2013 11:08 AM, Sander Bosma wrote:
> 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 <s.bosma@interay.com>
> ---
>  drivers/tty/n_gsm.c |   44 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 4a43ef5d7..0cad08fa 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
...
> @@ -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?

>  	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.

>  		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?

>  	/* 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...

> +			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.

> +			dlci->gsm->tx_bytes += msg->len;
> +		}
> +		spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
>  		gsm_dlci_data_kick(dlci);
>  	}
>  
...
> @@ -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,
-- 
js
suse labs