Re: [PATCH 5/9] tty: n_gsm: add open_error counter to gsm_mux

[Ilpo Järvinen]

On Wed, 5 Apr 2023, D. Starke wrote:

> From: Daniel Starke <daniel.starke@xxxxxxxxxxx>
> 
> Extend the n_gsm link statistics by a failed link open counter in
> preparation for an upcoming patch which will expose these.
> This counter is increased whenever an attempt to open the control channel
> failed.
> 
> Signed-off-by: Daniel Starke <daniel.starke@xxxxxxxxxxx>
> ---
>  drivers/tty/n_gsm.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index d42b92cbae88..9f6669686c59 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -338,6 +338,7 @@ struct gsm_mux {
>  	unsigned long bad_fcs;
>  	unsigned long malformed;
>  	unsigned long io_error;
> +	unsigned long open_error;
>  	unsigned long bad_size;
>  	unsigned long unsupported;
>  };
> @@ -1729,25 +1730,32 @@ static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
>  	struct gsm_dlci *dlci;
>  	struct gsm_dlci_param_bits *params;
>  
> -	if (dlen < sizeof(struct gsm_dlci_param_bits))
> +	if (dlen < sizeof(struct gsm_dlci_param_bits)) {
> +		gsm->open_error++;
>  		return;
> +	}
>  
>  	/* Invalid DLCI? */
>  	params = (struct gsm_dlci_param_bits *)data;
>  	addr = FIELD_GET(PN_D_FIELD_DLCI, params->d_bits);
> -	if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr])
> +	if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr]) {
> +		gsm->open_error++;
>  		return;
> +	}
>  	dlci = gsm->dlci[addr];
>  
>  	/* Too late for parameter negotiation? */
> -	if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN)
> +	if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN) {
> +		gsm->open_error++;
>  		return;
> +	}
>  
>  	/* Process the received parameters */
>  	if (gsm_process_negotiation(gsm, addr, cr, params) != 0) {
>  		/* Negotiation failed. Close the link. */
>  		if (debug & DBG_ERRORS)
>  			pr_info("%s PN failed\n", __func__);
> +		gsm->open_error++;
>  		gsm_dlci_close(dlci);
>  		return;
>  	}
> @@ -1767,6 +1775,7 @@ static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
>  	} else {
>  		if (debug & DBG_ERRORS)
>  			pr_info("%s PN in invalid state\n", __func__);
> +		gsm->open_error++;
>  	}

I'd use the "rollback" pattern here for all these and goto open_failed;
+ do the open_error increment there only once.

>  }
>  
> @@ -2220,6 +2229,7 @@ static void gsm_dlci_t1(struct timer_list *t)
>  			dlci->retries--;
>  			mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
>  		} else {
> +			gsm->open_error++;
>  			gsm_dlci_begin_close(dlci); /* prevent half open link */
>  		}
>  		break;
> @@ -2235,6 +2245,7 @@ static void gsm_dlci_t1(struct timer_list *t)
>  			dlci->mode = DLCI_MODE_ADM;
>  			gsm_dlci_open(dlci);
>  		} else {
> +			gsm->open_error++;
>  			gsm_dlci_begin_close(dlci); /* prevent half open link */
>  		}
>  
> @@ -2756,12 +2767,16 @@ static void gsm_queue(struct gsm_mux *gsm)
>  
>  	switch (gsm->control) {
>  	case SABM|PF:
> -		if (cr == 1)
> +		if (cr == 1) {
> +			gsm->open_error++;
>  			goto invalid;
> +		}
>  		if (dlci == NULL)
>  			dlci = gsm_dlci_alloc(gsm, address);
> -		if (dlci == NULL)
> +		if (dlci == NULL) {
> +			gsm->open_error++;
>  			return;
> +		}
>  		if (dlci->dead)
>  			gsm_response(gsm, address, DM|PF);
>  		else {
> @@ -3754,8 +3769,10 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
>  		dlci = gsm->dlci[dc.channel];
>  		if (!dlci) {
>  			dlci = gsm_dlci_alloc(gsm, dc.channel);
> -			if (!dlci)
> +			if (!dlci) {
> +				gsm->open_error++;
>  				return -ENOMEM;
> +			}
>  		}
>  		gsm_dlci_copy_config_values(dlci, &dc);
>  		if (copy_to_user((void __user *)arg, &dc, sizeof(dc)))
> @@ -3769,8 +3786,10 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
>  		dlci = gsm->dlci[dc.channel];
>  		if (!dlci) {
>  			dlci = gsm_dlci_alloc(gsm, dc.channel);
> -			if (!dlci)
> +			if (!dlci) {
> +				gsm->open_error++;
>  				return -ENOMEM;
> +			}
>  		}
>  		return gsm_dlci_config(dlci, &dc, 0);
>  	default:
> 

In general, the changelog could be more verbose about state machine 
states, message names which imply that the error is happening during 
"opening" phase/state.


-- 
 i.