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.