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.
