[PATCH 1/9] tty: n_gsm: fix redundant assignment of gsm->encoding

[PATCH 1/9] tty: n_gsm: fix redundant assignment of gsm->encoding

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The function gsmld_open() contains a redundant assignment of gsm->encoding.
The same value of GSM_ADV_OPT is already assigned to it during the
initialization of the struct in gsm_alloc_mux() a few lines earlier.

Fix this by removing the redundant second assignment of gsm->encoding in
gsmld_open().

Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index b7e1369a035c..c42c8b89fd46 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3585,7 +3585,6 @@ static int gsmld_open(struct tty_struct *tty)
 	tty->receive_room = 65536;
 
 	/* Attach the initial passive connection */
-	gsm->encoding = GSM_ADV_OPT;
 	gsmld_attach_gsm(tty, gsm);
 
 	/* The mux will not be activated yet, we wait for correct
-- 
2.34.1

[PATCH 2/9] tty: n_gsm: add restart parameter to DLC specific ioctl config

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

'gsm_config_ext' already allows to force a link reset via 'restart'
parameter. An equivalent parameter for an forced channel reset is still
missing in 'gsm_dlci_config'. Therefore, the user has no means to perform
an automatic channel reset after parameter configuration for
non-conflicting changes. Conflicting changes automatically reset the
channel already in the current implementation.

Add the parameter 'restart' to 'gsm_dlci_config' to force a channel reset
after ioctl setting regardless of whether the changes made require this or
not.

Note that the parameter is limited to the values 0 and 1 to allow future
additions here.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c         | 4 ++++
 include/uapi/linux/gsmmux.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c42c8b89fd46..eb21ca583642 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2531,6 +2531,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
 		return -EINVAL;
 	if (dc->k > 7)
 		return -EINVAL;
+	if (dc->restart > 1)   /* allow future extensions */
+		return -EINVAL;
 
 	/*
 	 * See what is needed for reconfiguration
@@ -2545,6 +2547,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
 	/* Requires care */
 	if (dc->priority != dlci->prio)
 		need_restart = true;
+	if (dc->restart)
+		need_restart = true;
 
 	if ((open && gsm->wait_config) || need_restart)
 		need_open = true;
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index eb67884e5f38..33ee7b857c52 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -58,7 +58,8 @@ struct gsm_dlci_config {
 	__u32 priority;		/* Priority (0 for default value) */
 	__u32 i;		/* Frame type (1 = UIH, 2 = UI) */
 	__u32 k;		/* Window size (0 for default value) */
-	__u32 reserved[8];	/* For future use, must be initialized to zero */
+	__u32 restart;		/* Force DLCI channel reset? */
+	__u32 reserved[7];	/* For future use, must be initialized to zero */
 };
 
 #define GSMIOC_GETCONF_DLCI	_IOWR('G', 7, struct gsm_dlci_config)
-- 
2.34.1

[PATCH 3/9] tty: n_gsm: add missing description to gsm_config

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

Currently, all available structure fields in gsmmux.h except those
for gsm_config are commented.

Fix this by adding appropriate comments to the not commented fields.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 include/uapi/linux/gsmmux.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index 33ee7b857c52..422a52e184b3 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -8,17 +8,17 @@
 
 struct gsm_config
 {
-	unsigned int adaption;
-	unsigned int encapsulation;
-	unsigned int initiator;
-	unsigned int t1;
-	unsigned int t2;
-	unsigned int t3;
-	unsigned int n2;
-	unsigned int mru;
-	unsigned int mtu;
-	unsigned int k;
-	unsigned int i;
+	unsigned int adaption;	/* Convergence layer type */
+	unsigned int encapsulation; /* Framing (0 = basic option, 1 = advanced option) */
+	unsigned int initiator;	/* Initiator or responder */
+	unsigned int t1;	/* Acknowledgment timer */
+	unsigned int t2;	/* Response timer for multiplexer control channel */
+	unsigned int t3;	/* Response timer for wake-up procedure */
+	unsigned int n2;	/* Maximum number of retransmissions */
+	unsigned int mru;	/* Maximum incoming frame payload size */
+	unsigned int mtu;	/* Maximum outgoing frame payload size */
+	unsigned int k;		/* Window size */
+	unsigned int i;		/* Frame type (1 = UIH, 2 = UI) */
 	unsigned int unused[8];	/* Can not be used */
 };
 
-- 
2.34.1

[PATCH 4/9] tty: n_gsm: fix unneeded initialization of ret in gsm_dlci_config

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The variable 'ret' is not used before assignment from gsm_activate_mux().
Still it gets initialized to zero at declaration.

Fix this as remarked in the link below by removing the initialization.

Fixes: edd5f60c3400 ("tty: n_gsm: fix mux activation issues in gsm_config()")
Link: https://lore.kernel.org/all/b42bc4d1-cc9d-d115-c981-aaa053bdc59f@kernel.org/

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index eb21ca583642..d42b92cbae88 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3276,7 +3276,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
 
 static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 {
-	int ret = 0;
+	int ret;
 	int need_close = 0;
 	int need_restart = 0;
 
-- 
2.34.1

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

[D. Starke]

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++;
 	}
 }
 
@@ -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:
-- 
2.34.1

[PATCH 6/9] tty: n_gsm: increase malformed counter for malformed control frames

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The malformed counter in gsm_mux is already increased in case of errors
detected in gsm_queue() and gsm1_receive(). gsm_dlci_command() also
detects a case for a malformed frame but does not increase the malformed
counter yet.

Fix this by also increasing the gsm_mux malformed counter in case of a
malformed frame in gsm_dlci_command().

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 9f6669686c59..317aa67ed169 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2454,8 +2454,10 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len)
 	data += dlen;
 
 	/* Malformed command? */
-	if (clen > len)
+	if (clen > len) {
+		dlci->gsm->malformed++;
 		return;
+	}
 
 	if (command & 1)
 		gsm_control_message(dlci->gsm, command, data, clen);
-- 
2.34.1

[PATCH 7/9] tty: n_gsm: increase gsm_mux unsupported counted where appropriate

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

The structure gsm_mux contains the 'unsupported' field. However, there is
currently no place in the code which increases this counter.

Increase the 'unsupported' statistics counter in the following case:
- an unsupported frame type has been requested by the peer via parameter
  negotiation
- a control frame with an unsupported but known command has been received

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 317aa67ed169..49cb2dbfa233 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1589,6 +1589,7 @@ static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
 		if (debug & DBG_ERRORS)
 			pr_info("%s unsupported I frame request in PN\n",
 				__func__);
+		gsm->unsupported++;
 		return -EINVAL;
 	default:
 		if (debug & DBG_ERRORS)
@@ -1896,6 +1897,8 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 		/* Optional unsupported commands */
 	case CMD_RPN:	/* Remote port negotiation */
 	case CMD_SNC:	/* Service negotiation command */
+		gsm->unsupported++;
+		fallthrough;
 	default:
 		/* Reply to bad commands with an NSC */
 		buf[0] = command;
-- 
2.34.1

[PATCH 8/9] tty: n_gsm: add DLCI specific rx/tx statistics

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

Add counters for the number of data bytes received/transmitted per DLCI in
for preparation for an upcoming patch which will expose these values to the
user.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 49cb2dbfa233..61f9825fde3c 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -185,6 +185,9 @@ struct gsm_dlci {
 	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
 	struct net_device *net; /* network interface, if created */
+	/* Statistics (not currently exposed) */
+	u64 tx;			/* Data bytes sent on this DLCI */
+	u64 rx;			/* Data bytes received on this DLCI */
 };
 
 /*
@@ -1215,6 +1218,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 	tty_port_tty_wakeup(&dlci->port);
 
 	__gsm_data_queue(dlci, msg);
+	dlci->tx += len;
 	/* Bytes of data we used up */
 	return size;
 }
@@ -1459,6 +1463,7 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
 	msg->data[1] = (dlen << 1) | EA;
 	memcpy(msg->data + 2, data, dlen);
 	gsm_data_queue(gsm->dlci[0], msg);
+	gsm->dlci[0]->tx += dlen;
 
 	return 0;
 }
@@ -1485,6 +1490,7 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
 	msg->data[1] = (dlen << 1) | EA;
 	memcpy(msg->data + 2, data, dlen);
 	gsm_data_queue(gsm->dlci[0], msg);
+	gsm->dlci[0]->tx += dlen;
 }
 
 /**
@@ -1849,10 +1855,13 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 						const u8 *data, int clen)
 {
 	u8 buf[1];
+	struct gsm_dlci *dlci = gsm->dlci[0];
+
+	if (dlci)
+		dlci->rx += clen;
 
 	switch (command) {
 	case CMD_CLD: {
-		struct gsm_dlci *dlci = gsm->dlci[0];
 		/* Modem wishes to close down */
 		if (dlci) {
 			dlci->dead = true;
@@ -1931,6 +1940,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 
 	ctrl = gsm->pending_cmd;
 	dlci = gsm->dlci[0];
+	if (dlci)
+		dlci->rx += clen;
 	command |= 1;
 	/* Does the reply match our command */
 	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
@@ -2295,6 +2306,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 			need_pn = true;
 	}
 
+	dlci->tx = 0;
+	dlci->rx = 0;
+
 	switch (dlci->state) {
 	case DLCI_CLOSED:
 	case DLCI_WAITING_CONFIG:
@@ -2327,6 +2341,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
  */
 static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
 {
+	dlci->tx = 0;
+	dlci->rx = 0;
+
 	switch (dlci->state) {
 	case DLCI_CLOSED:
 	case DLCI_WAITING_CONFIG:
@@ -2346,6 +2363,9 @@ static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
  */
 static void gsm_dlci_set_wait_config(struct gsm_dlci *dlci)
 {
+	dlci->tx = 0;
+	dlci->rx = 0;
+
 	switch (dlci->state) {
 	case DLCI_CLOSED:
 	case DLCI_CLOSING:
@@ -2422,6 +2442,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
 		fallthrough;
 	case 1:		/* Line state will go via DLCI 0 controls only */
 	default:
+		dlci->rx += clen;
 		tty_insert_flip_string(port, data, clen);
 		tty_flip_buffer_push(port);
 	}
@@ -2782,6 +2803,7 @@ static void gsm_queue(struct gsm_mux *gsm)
 			gsm->open_error++;
 			return;
 		}
+		dlci->rx += gsm->len;
 		if (dlci->dead)
 			gsm_response(gsm, address, DM|PF);
 		else {
-- 
2.34.1

[PATCH 9/9] tty: n_gsm: cleanup gsm_control_command and gsm_control_reply

[D. Starke]

From: Daniel Starke <daniel.starke@siemens.com>

There are multiple places in gsm_control_command and gsm_control_reply that
derive the specific DLCI handle directly out of the DLCI table in gsm.

Add a local variable which holds this handle and use it instead to improve
code readability.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 61f9825fde3c..87720ebc38d7 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1454,16 +1454,17 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
 			       int dlen)
 {
 	struct gsm_msg *msg;
+	struct gsm_dlci *dlci = gsm->dlci[0];
 
-	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
+	msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
 	if (msg == NULL)
 		return -ENOMEM;
 
 	msg->data[0] = (cmd << 1) | CR | EA;	/* Set C/R */
 	msg->data[1] = (dlen << 1) | EA;
 	memcpy(msg->data + 2, data, dlen);
-	gsm_data_queue(gsm->dlci[0], msg);
-	gsm->dlci[0]->tx += dlen;
+	gsm_data_queue(dlci, msg);
+	dlci->tx += dlen;
 
 	return 0;
 }
@@ -1482,15 +1483,16 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
 					int dlen)
 {
 	struct gsm_msg *msg;
+	struct gsm_dlci *dlci = gsm->dlci[0];
 
-	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
+	msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
 	if (msg == NULL)
 		return;
 	msg->data[0] = (cmd & 0xFE) << 1 | EA;	/* Clear C/R */
 	msg->data[1] = (dlen << 1) | EA;
 	memcpy(msg->data + 2, data, dlen);
-	gsm_data_queue(gsm->dlci[0], msg);
-	gsm->dlci[0]->tx += dlen;
+	gsm_data_queue(dlci, msg);
+	dlci->tx += dlen;
 }
 
 /**
-- 
2.34.1

Re: [PATCH 2/9] tty: n_gsm: add restart parameter to DLC specific ioctl config

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> 'gsm_config_ext' already allows to force a link reset via 'restart'
> parameter.

Please be more precise, there a function and struct with this name.

HOWEVER, there is no such parameter in tty-next (am I looking from wrong 
repo??):

static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)

...nor here:

struct gsm_config_ext {
        __u32 keep_alive;       /* Control channel keep-alive in 1/100th of a
                                 * second (0 to disable)
                                 */
        __u32 wait_config;      /* Wait for DLCI config before opening virtual link? */
        __u32 reserved[6];      /* For future use, must be initialized to zero */
};

???


-- 
 i.

> An equivalent parameter for an forced channel reset is still
> missing in 'gsm_dlci_config'. Therefore, the user has no means to perform
> an automatic channel reset after parameter configuration for
> non-conflicting changes. Conflicting changes automatically reset the
> channel already in the current implementation.
> 
> Add the parameter 'restart' to 'gsm_dlci_config' to force a channel reset
> after ioctl setting regardless of whether the changes made require this or
> not.
> 
> Note that the parameter is limited to the values 0 and 1 to allow future
> additions here.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c         | 4 ++++
>  include/uapi/linux/gsmmux.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index c42c8b89fd46..eb21ca583642 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2531,6 +2531,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
>  		return -EINVAL;
>  	if (dc->k > 7)
>  		return -EINVAL;
> +	if (dc->restart > 1)   /* allow future extensions */
> +		return -EINVAL;
>  
>  	/*
>  	 * See what is needed for reconfiguration
> @@ -2545,6 +2547,8 @@ static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, in
>  	/* Requires care */
>  	if (dc->priority != dlci->prio)
>  		need_restart = true;
> +	if (dc->restart)
> +		need_restart = true;
>  
>  	if ((open && gsm->wait_config) || need_restart)
>  		need_open = true;
> diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> index eb67884e5f38..33ee7b857c52 100644
> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -58,7 +58,8 @@ struct gsm_dlci_config {
>  	__u32 priority;		/* Priority (0 for default value) */
>  	__u32 i;		/* Frame type (1 = UIH, 2 = UI) */
>  	__u32 k;		/* Window size (0 for default value) */
> -	__u32 reserved[8];	/* For future use, must be initialized to zero */
> +	__u32 restart;		/* Force DLCI channel reset? */
> +	__u32 reserved[7];	/* For future use, must be initialized to zero */
>  };
>  
>  #define GSMIOC_GETCONF_DLCI	_IOWR('G', 7, struct gsm_dlci_config)
> 

Re: [PATCH 1/9] tty: n_gsm: fix redundant assignment of gsm->encoding

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The function gsmld_open() contains a redundant assignment of gsm->encoding.
> The same value of GSM_ADV_OPT is already assigned to it during the
> initialization of the struct in gsm_alloc_mux() a few lines earlier.
> 
> Fix this by removing the redundant second assignment of gsm->encoding in
> gsmld_open().
> 
> Fixes: e1eaea46bb40 ("tty: n_gsm line discipline")
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index b7e1369a035c..c42c8b89fd46 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -3585,7 +3585,6 @@ static int gsmld_open(struct tty_struct *tty)
>  	tty->receive_room = 65536;
>  
>  	/* Attach the initial passive connection */
> -	gsm->encoding = GSM_ADV_OPT;
>  	gsmld_attach_gsm(tty, gsm);
>  
>  	/* The mux will not be activated yet, we wait for correct
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 3/9] tty: n_gsm: add missing description to gsm_config

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> Currently, all available structure fields in gsmmux.h except those
> for gsm_config are commented.
> 
> Fix this by adding appropriate comments to the not commented fields.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  include/uapi/linux/gsmmux.h | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> index 33ee7b857c52..422a52e184b3 100644
> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -8,17 +8,17 @@
>  
>  struct gsm_config
>  {
> -	unsigned int adaption;
> -	unsigned int encapsulation;
> -	unsigned int initiator;
> -	unsigned int t1;
> -	unsigned int t2;
> -	unsigned int t3;
> -	unsigned int n2;
> -	unsigned int mru;
> -	unsigned int mtu;
> -	unsigned int k;
> -	unsigned int i;
> +	unsigned int adaption;	/* Convergence layer type */
> +	unsigned int encapsulation; /* Framing (0 = basic option, 1 = advanced option) */
> +	unsigned int initiator;	/* Initiator or responder */
> +	unsigned int t1;	/* Acknowledgment timer */
> +	unsigned int t2;	/* Response timer for multiplexer control channel */
> +	unsigned int t3;	/* Response timer for wake-up procedure */
> +	unsigned int n2;	/* Maximum number of retransmissions */
> +	unsigned int mru;	/* Maximum incoming frame payload size */
> +	unsigned int mtu;	/* Maximum outgoing frame payload size */

I'm used to "payload" referring in networking context to "useful" part of 
the frame but MTU/MRU probably includes headers too? Maybe just say "frame 
size"?

> +	unsigned int k;		/* Window size */
> +	unsigned int i;		/* Frame type (1 = UIH, 2 = UI) */
>  	unsigned int unused[8];	/* Can not be used */
>  };
>  
> 

-- 
 i.

Re: [PATCH 4/9] tty: n_gsm: fix unneeded initialization of ret in gsm_dlci_config

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The variable 'ret' is not used before assignment from gsm_activate_mux().
> Still it gets initialized to zero at declaration.
> 
> Fix this as remarked in the link below by removing the initialization.
> 
> Fixes: edd5f60c3400 ("tty: n_gsm: fix mux activation issues in gsm_config()")

This doesn't "fix" any bug so Fixes tag seems inappropriate unless does it 
fix a compiler warning (in which case you should quote the warning in 
this changelog and state you're fixing this warning from compiler)?

> Link: https://lore.kernel.org/all/b42bc4d1-cc9d-d115-c981-aaa053bdc59f@kernel.org/
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index eb21ca583642..d42b92cbae88 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -3276,7 +3276,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
>  
>  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  {
> -	int ret = 0;
> +	int ret;

While at it, I'd move the declaration into the block where it's used so 
the scope where the variable is used is easier to see on the first glance.

>  	int need_close = 0;
>  	int need_restart = 0;
>  
> 


-- 
 i.

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@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.


-- 
 i.

Re: [PATCH 6/9] tty: n_gsm: increase malformed counter for malformed control frames

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The malformed counter in gsm_mux is already increased in case of errors
> detected in gsm_queue() and gsm1_receive(). gsm_dlci_command() also
> detects a case for a malformed frame but does not increase the malformed
> counter yet.
> 
> Fix this by also increasing the gsm_mux malformed counter in case of a
> malformed frame in gsm_dlci_command().
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 9f6669686c59..317aa67ed169 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2454,8 +2454,10 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, const u8 *data, int len)
>  	data += dlen;
>  
>  	/* Malformed command? */
> -	if (clen > len)
> +	if (clen > len) {
> +		dlci->gsm->malformed++;
>  		return;
> +	}

Should this change have a Fixes: tag? Or is ->malformed just an internal 
variable which is not exposed anywhere (in which case state that in the 
changelog)?


-- 
 i.

Re: [PATCH 7/9] tty: n_gsm: increase gsm_mux unsupported counted where appropriate

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> The structure gsm_mux contains the 'unsupported' field. However, there is
> currently no place in the code which increases this counter.
> 
> Increase the 'unsupported' statistics counter in the following case:
> - an unsupported frame type has been requested by the peer via parameter
>   negotiation
> - a control frame with an unsupported but known command has been received

So inconsistent/unsupported adaptation doesn't fall under the second 
bullet?

(Please excuse my ignorance, I'm trying to review your patches with 
somewhat limited knowledge about how things work).

-- 
 i.

> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 317aa67ed169..49cb2dbfa233 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1589,6 +1589,7 @@ static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
>  		if (debug & DBG_ERRORS)
>  			pr_info("%s unsupported I frame request in PN\n",
>  				__func__);
> +		gsm->unsupported++;
>  		return -EINVAL;
>  	default:
>  		if (debug & DBG_ERRORS)
> @@ -1896,6 +1897,8 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
>  		/* Optional unsupported commands */
>  	case CMD_RPN:	/* Remote port negotiation */
>  	case CMD_SNC:	/* Service negotiation command */
> +		gsm->unsupported++;
> +		fallthrough;
>  	default:
>  		/* Reply to bad commands with an NSC */
>  		buf[0] = command;
> 

Re: [PATCH 8/9] tty: n_gsm: add DLCI specific rx/tx statistics

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> Add counters for the number of data bytes received/transmitted per DLCI in
> for preparation for an upcoming patch which will expose these values to the
> user.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 49cb2dbfa233..61f9825fde3c 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -185,6 +185,9 @@ struct gsm_dlci {
>  	void (*data)(struct gsm_dlci *dlci, const u8 *data, int len);
>  	void (*prev_data)(struct gsm_dlci *dlci, const u8 *data, int len);
>  	struct net_device *net; /* network interface, if created */
> +	/* Statistics (not currently exposed) */
> +	u64 tx;			/* Data bytes sent on this DLCI */
> +	u64 rx;			/* Data bytes received on this DLCI */
>  };
>  
>  /*
> @@ -1215,6 +1218,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
>  	tty_port_tty_wakeup(&dlci->port);
>  
>  	__gsm_data_queue(dlci, msg);
> +	dlci->tx += len;
>  	/* Bytes of data we used up */
>  	return size;
>  }

Reading the function comments and your changelog, I'm left to wonder why 
gsm_dlci_data_output() is supposed to increment ->tx but 
gsm_dlci_data_output_framed() is not?

-- 
 i.

> @@ -1459,6 +1463,7 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
>  	msg->data[1] = (dlen << 1) | EA;
>  	memcpy(msg->data + 2, data, dlen);
>  	gsm_data_queue(gsm->dlci[0], msg);
> +	gsm->dlci[0]->tx += dlen;
>  
>  	return 0;
>  }
> @@ -1485,6 +1490,7 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
>  	msg->data[1] = (dlen << 1) | EA;
>  	memcpy(msg->data + 2, data, dlen);
>  	gsm_data_queue(gsm->dlci[0], msg);
> +	gsm->dlci[0]->tx += dlen;
>  }
>  
>  /**
> @@ -1849,10 +1855,13 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
>  						const u8 *data, int clen)
>  {
>  	u8 buf[1];
> +	struct gsm_dlci *dlci = gsm->dlci[0];
> +
> +	if (dlci)
> +		dlci->rx += clen;
>  
>  	switch (command) {
>  	case CMD_CLD: {
> -		struct gsm_dlci *dlci = gsm->dlci[0];
>  		/* Modem wishes to close down */
>  		if (dlci) {
>  			dlci->dead = true;
> @@ -1931,6 +1940,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  
>  	ctrl = gsm->pending_cmd;
>  	dlci = gsm->dlci[0];
> +	if (dlci)
> +		dlci->rx += clen;
>  	command |= 1;
>  	/* Does the reply match our command */
>  	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
> @@ -2295,6 +2306,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>  			need_pn = true;
>  	}
>  
> +	dlci->tx = 0;
> +	dlci->rx = 0;
> +
>  	switch (dlci->state) {
>  	case DLCI_CLOSED:
>  	case DLCI_WAITING_CONFIG:
> @@ -2327,6 +2341,9 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>   */
>  static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
>  {
> +	dlci->tx = 0;
> +	dlci->rx = 0;
> +
>  	switch (dlci->state) {
>  	case DLCI_CLOSED:
>  	case DLCI_WAITING_CONFIG:
> @@ -2346,6 +2363,9 @@ static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
>   */
>  static void gsm_dlci_set_wait_config(struct gsm_dlci *dlci)
>  {
> +	dlci->tx = 0;
> +	dlci->rx = 0;
> +
>  	switch (dlci->state) {
>  	case DLCI_CLOSED:
>  	case DLCI_CLOSING:
> @@ -2422,6 +2442,7 @@ static void gsm_dlci_data(struct gsm_dlci *dlci, const u8 *data, int clen)
>  		fallthrough;
>  	case 1:		/* Line state will go via DLCI 0 controls only */
>  	default:
> +		dlci->rx += clen;
>  		tty_insert_flip_string(port, data, clen);
>  		tty_flip_buffer_push(port);
>  	}
> @@ -2782,6 +2803,7 @@ static void gsm_queue(struct gsm_mux *gsm)
>  			gsm->open_error++;
>  			return;
>  		}
> +		dlci->rx += gsm->len;
>  		if (dlci->dead)
>  			gsm_response(gsm, address, DM|PF);
>  		else {
> 

Re: [PATCH 9/9] tty: n_gsm: cleanup gsm_control_command and gsm_control_reply

[Ilpo Järvinen]

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

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> There are multiple places in gsm_control_command and gsm_control_reply that
> derive the specific DLCI handle directly out of the DLCI table in gsm.
> 
> Add a local variable which holds this handle and use it instead to improve
> code readability.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 61f9825fde3c..87720ebc38d7 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1454,16 +1454,17 @@ static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
>  			       int dlen)
>  {
>  	struct gsm_msg *msg;
> +	struct gsm_dlci *dlci = gsm->dlci[0];
>  
> -	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
> +	msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
>  	if (msg == NULL)
>  		return -ENOMEM;
>  
>  	msg->data[0] = (cmd << 1) | CR | EA;	/* Set C/R */
>  	msg->data[1] = (dlen << 1) | EA;
>  	memcpy(msg->data + 2, data, dlen);
> -	gsm_data_queue(gsm->dlci[0], msg);
> -	gsm->dlci[0]->tx += dlen;
> +	gsm_data_queue(dlci, msg);
> +	dlci->tx += dlen;
>  
>  	return 0;
>  }
> @@ -1482,15 +1483,16 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
>  					int dlen)
>  {
>  	struct gsm_msg *msg;
> +	struct gsm_dlci *dlci = gsm->dlci[0];
>  
> -	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
> +	msg = gsm_data_alloc(gsm, 0, dlen + 2, dlci->ftype);
>  	if (msg == NULL)
>  		return;
>  	msg->data[0] = (cmd & 0xFE) << 1 | EA;	/* Clear C/R */
>  	msg->data[1] = (dlen << 1) | EA;
>  	memcpy(msg->data + 2, data, dlen);
> -	gsm_data_queue(gsm->dlci[0], msg);
> -	gsm->dlci[0]->tx += dlen;
> +	gsm_data_queue(dlci, msg);
> +	dlci->tx += dlen;
>  }
>  
>  /**
> 

IMO, this patch should be done before patch 8/9.

-- 
 i.

RE: [PATCH 2/9] tty: n_gsm: add restart parameter to DLC specific ioctl config

[Starke, Daniel]

> > From: Daniel Starke <daniel.starke@siemens.com>
> > 
> > 'gsm_config_ext' already allows to force a link reset via 'restart'
> > parameter.
> 
> Please be more precise, there a function and struct with this name.
> 
> HOWEVER, there is no such parameter in tty-next (am I looking from wrong 
> repo??):
> 
> static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)
> 
> ...nor here:
> 
> struct gsm_config_ext {
>         __u32 keep_alive;       /* Control channel keep-alive in 1/100th of a
>                                  * second (0 to disable)
>                                  */
>         __u32 wait_config;      /* Wait for DLCI config before opening virtual link? */
>         __u32 reserved[6];      /* For future use, must be initialized to zero */
> };
> 
> ???

You are right. Looks like I was looking at the wrong branch. I will reword
the commit message and deliver the patch for 'struct gsm_config_ext'
afterwards.

Best regards,
Daniel Starke

RE: [PATCH 3/9] tty: n_gsm: add missing description to gsm_config

[Starke, Daniel]

> > +	unsigned int mru;	/* Maximum incoming frame payload size */
> > +	unsigned int mtu;	/* Maximum outgoing frame payload size */
> 
> I'm used to "payload" referring in networking context to "useful" part of 
> the frame but MTU/MRU probably includes headers too? Maybe just say
> "frame size"?

Just frame size is incorrect, as the basic/advanced option mode header and
byte stuffing do not count against this number. See 'gsm0_receive' and
'gsm1_receive' and also ch. 5.7.2 of the standard. The outgoing limit is
applied in 'gsm_dlci_data_output' and 'gsm_dlci_data_output_framed'.

Link: https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516

Best regards,
Daniel Starke

RE: [PATCH 4/9] tty: n_gsm: fix unneeded initialization of ret in gsm_dlci_config

[Starke, Daniel]

> > From: Daniel Starke <daniel.starke@siemens.com>
> > 
> > The variable 'ret' is not used before assignment from gsm_activate_mux().
> > Still it gets initialized to zero at declaration.
> > 
> > Fix this as remarked in the link below by removing the initialization.
> > 
> > Fixes: edd5f60c3400 ("tty: n_gsm: fix mux activation issues in gsm_config()")
> 
> This doesn't "fix" any bug so Fixes tag seems inappropriate unless does it 
> fix a compiler warning (in which case you should quote the warning in 
> this changelog and state you're fixing this warning from compiler)?

No, it does not change any compiler warning. I will remove the tag.

> > Link: https://lore.kernel.org/all/b42bc4d1-cc9d-d115-c981-aaa053bdc59f@kernel.org/
> > 
> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >  drivers/tty/n_gsm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index eb21ca583642..d42b92cbae88 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -3276,7 +3276,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
> >  
> >  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
> >  {
> > -	int ret = 0;
> > +	int ret;
> 
> While at it, I'd move the declaration into the block where it's used so 
> the scope where the variable is used is easier to see on the first glance.

I will move it accordingly.

Best regards,
Daniel Starke

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

[Starke, Daniel]

> > @@ -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

RE: [PATCH 6/9] tty: n_gsm: increase malformed counter for malformed control frames

[Starke, Daniel]

> > +		dlci->gsm->malformed++;
> >  		return;
> > +	}
> 
> Should this change have a Fixes: tag? Or is ->malformed just an internal 
> variable which is not exposed anywhere (in which case state that in the 
> changelog)?

Correct, the variable is not yet exposed anywhere. I will add a remark for
this in the changelog.

Best regards,
Daniel Starke

RE: [PATCH 7/9] tty: n_gsm: increase gsm_mux unsupported counted where appropriate

[Starke, Daniel]

> > From: Daniel Starke <daniel.starke@siemens.com>
> > 
> > The structure gsm_mux contains the 'unsupported' field. However, there is
> > currently no place in the code which increases this counter.
> > 
> > Increase the 'unsupported' statistics counter in the following case:
> > - an unsupported frame type has been requested by the peer via parameter
> >   negotiation
> > - a control frame with an unsupported but known command has been received
> 
> So inconsistent/unsupported adaptation doesn't fall under the second 
> bullet?
> 
> (Please excuse my ignorance, I'm trying to review your patches with 
> somewhat limited knowledge about how things work).

A wrong adaption is nothing we can detect with sufficient accuracy as this
changes the structure of the UI/UIH frames. E.g. a one byte header is added
in case of convergence layer type 2 instead of 1 and contains the modem
signal octet with the state of the signal lines. There is no checksum or
other value which indicates of this field is correct or should be present.
Therefore, we can only assume protocol correctness here.
See also function 'gsm_dlci_data' where this is handled.

Best regards,
Daniel Starke

RE: [PATCH 8/9] tty: n_gsm: add DLCI specific rx/tx statistics

[Starke, Daniel]

> > @@ -1215,6 +1218,7 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
> >  	tty_port_tty_wakeup(&dlci->port);
> >  
> >  	__gsm_data_queue(dlci, msg);
> > +	dlci->tx += len;
> >  	/* Bytes of data we used up */
> >  	return size;
> >  }
> 
> Reading the function comments and your changelog, I'm left to wonder why 
> gsm_dlci_data_output() is supposed to increment ->tx but 
> gsm_dlci_data_output_framed() is not?

Thank you for pointing this out. I will add the part for
gsm_dlci_data_output_framed() it in the next version of this patch.

Best regards,
Daniel Starke

RE: [PATCH 9/9] tty: n_gsm: cleanup gsm_control_command and gsm_control_reply

[Starke, Daniel]

> IMO, this patch should be done before patch 8/9.

I will change the order in the next version of this patch.

Best regards,
Daniel Starke