[5/9] tty: n_gsm: add open_error counter to gsm_mux
Commit Message
From: Daniel Starke <daniel.starke@siemens.com>
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@siemens.com>
---
drivers/tty/n_gsm.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
Comments
On Wed, 5 Apr 2023, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
>
> 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@siemens.com>
> ---
> 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.
> > @@ -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.
True, that would be a more elegant way to handle this. However, it does
more than just adding this counter. Therefore, I would prefer to do this
in a later cleanup.
...
> 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 will add a more detailed description.
Best regards,
Daniel Starke
@@ -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++;
}
}
@@ -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: