Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <999ae7a0-4699-17be-8045-af856237f133@kernel.org>
Date:   Mon, 23 May 2022 13:49:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v2 4/9] tty: n_gsm: fix missing timer to handle stalled
 links
Content-Language: en-US
To:     "D. Starke" <daniel.starke@siemens.com>,
        linux-serial@vger.kernel.org, gregkh@linuxfoundation.org
Cc:     linux-kernel@vger.kernel.org
References: <20220519070757.2096-1-daniel.starke@siemens.com>
 <20220519070757.2096-4-daniel.starke@siemens.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <20220519070757.2096-4-daniel.starke@siemens.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 19. 05. 22, 9:07, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>   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