[PATCH 1/3] tty: n_gsm: introduce macro for minimal unit size

[PATCH 1/3] tty: n_gsm: introduce macro for minimal unit size

[D. Starke]

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

n_gsm has a minimal protocol overhead of 7 bytes. The current code already
checks whether the configured MRU/MTU size is at least one byte more than
this.

Introduce the macro MIN_UNIT_SIZE to make this value more obvious.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5e516f5cac5a..8e039f2a0427 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -89,6 +89,7 @@ module_param(debug, int, 0600);
  */
 #define MAX_MRU 1500
 #define MAX_MTU 1500
+#define MIN_UNIT_SIZE 8
 /* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
 #define PROT_OVERHEAD 7
 #define	GSM_NET_TX_TIMEOUT (HZ*10)
@@ -2712,7 +2713,9 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	if ((c->adaption != 1 && c->adaption != 2) || c->k)
 		return -EOPNOTSUPP;
 	/* Check the MRU/MTU range looks sane */
-	if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
+	if (c->mru < MIN_UNIT_SIZE || c->mtu < MIN_UNIT_SIZE)
+		return -EINVAL;
+	if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
 		return -EINVAL;
 	if (c->n2 > 255)
 		return -EINVAL;
@@ -3296,7 +3299,7 @@ static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
 		return -ENOMEM;
 	}
 	net->mtu = dlci->gsm->mtu;
-	net->min_mtu = 8;
+	net->min_mtu = MIN_UNIT_SIZE;
 	net->max_mtu = dlci->gsm->mtu;
 	mux_net = netdev_priv(net);
 	mux_net->dlci = dlci;
-- 
2.34.1

[PATCH 2/3] tty: n_gsm: add parameters used with parameter negotiation

[D. Starke]

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

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.4.6.3.1 describes the encoding of the
parameter negotiation messages.

Add the parameters used there to 'gsm_mux' and 'gsm_dlci' and initialize both
according to the value ranges and recommended defaults defined in chapter 5.7.

Replace the use of the DLC default values from the 'gsm_mux' fields with the DLC
specific values from the 'gsm_dlci' fields where applicable.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 8e039f2a0427..b6813a134c18 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -75,7 +75,9 @@ module_param(debug, int, 0600);
 
 #define T1	10		/* 100mS */
 #define T2	34		/* 333mS */
+#define T3	10		/* 10s */
 #define N2	3		/* Retry 3 times */
+#define K	2		/* outstanding I frames */
 
 /* Use long timers for testing at low speed with debug on */
 #ifdef DEBUG_TIMING
@@ -160,7 +162,12 @@ struct gsm_dlci {
 	int prev_adaption;
 	u32 modem_rx;		/* Our incoming virtual modem lines */
 	u32 modem_tx;		/* Our outgoing modem lines */
+	unsigned int mtu;
 	bool dead;		/* Refuse re-open */
+	/* Configuration */
+	u8 prio;		/* Priority */
+	u8 ftype;		/* Frame type */
+	u8 k;			/* Window size */
 	/* Flow control */
 	bool throttled;		/* Private copy of throttle state */
 	bool constipated;	/* Throttle status for outgoing */
@@ -282,8 +289,9 @@ struct gsm_mux {
 	/* Configuration */
 	int adaption;		/* 1 or 2 supported */
 	u8 ftype;		/* UI or UIH */
-	int t1, t2;		/* Timers in 1/100th of a sec */
+	int t1, t2, t3;		/* Timers in 1/100th of a sec */
 	int n2;			/* Retry count */
+	u8 k;			/* Window size */
 
 	/* Statistics (not currently exposed) */
 	unsigned long bad_fcs;
@@ -1075,12 +1083,12 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 		return 0;
 
 	/* MTU/MRU count only the data bits but watch adaption mode */
-	if ((len + h) > gsm->mtu)
-		len = gsm->mtu - h;
+	if ((len + h) > dlci->mtu)
+		len = dlci->mtu - h;
 
 	size = len + h;
 
-	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+	msg = gsm_data_alloc(gsm, dlci->addr, size, dlci->ftype);
 	if (!msg)
 		return -ENOMEM;
 	dp = msg->data;
@@ -1144,19 +1152,19 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
 	len = dlci->skb->len + overhead;
 
 	/* MTU/MRU count only the data bits */
-	if (len > gsm->mtu) {
+	if (len > dlci->mtu) {
 		if (dlci->adaption == 3) {
 			/* Over long frame, bin it */
 			dev_kfree_skb_any(dlci->skb);
 			dlci->skb = NULL;
 			return 0;
 		}
-		len = gsm->mtu;
+		len = dlci->mtu;
 	} else
 		last = 1;
 
 	size = len + overhead;
-	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+	msg = gsm_data_alloc(gsm, dlci->addr, size, dlci->ftype);
 	if (msg == NULL) {
 		skb_queue_tail(&dlci->skb_list, dlci->skb);
 		dlci->skb = NULL;
@@ -1213,7 +1221,7 @@ static int gsm_dlci_modem_output(struct gsm_mux *gsm, struct gsm_dlci *dlci,
 		return -EINVAL;
 	}
 
-	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
+	msg = gsm_data_alloc(gsm, dlci->addr, size, dlci->ftype);
 	if (!msg) {
 		pr_err("%s: gsm_data_alloc error", __func__);
 		return -ENOMEM;
@@ -1338,8 +1346,9 @@ static void gsm_dlci_data_kick(struct gsm_dlci *dlci)
 static int gsm_control_command(struct gsm_mux *gsm, int cmd, const u8 *data,
 			       int dlen)
 {
-	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->ftype);
+	struct gsm_msg *msg;
 
+	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
 	if (msg == NULL)
 		return -ENOMEM;
 
@@ -1365,7 +1374,8 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, const u8 *data,
 					int dlen)
 {
 	struct gsm_msg *msg;
-	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->ftype);
+
+	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->dlci[0]->ftype);
 	if (msg == NULL)
 		return;
 	msg->data[0] = (cmd & 0xFE) << 1 | EA;	/* Clear C/R */
@@ -2075,6 +2085,13 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 	dlci->gsm = gsm;
 	dlci->addr = addr;
 	dlci->adaption = gsm->adaption;
+	dlci->mtu = gsm->mtu;
+	if (addr == 0)
+		dlci->prio = 0;
+	else
+		dlci->prio = ((((addr / 8) + 1) * 8) - 1);
+	dlci->ftype = gsm->ftype;
+	dlci->k = gsm->k;
 	dlci->state = DLCI_CLOSED;
 	if (addr) {
 		dlci->data = gsm_dlci_data;
@@ -2650,7 +2667,9 @@ static struct gsm_mux *gsm_alloc_mux(void)
 
 	gsm->t1 = T1;
 	gsm->t2 = T2;
+	gsm->t3 = T3;
 	gsm->n2 = N2;
+	gsm->k = K;
 	gsm->ftype = UIH;
 	gsm->adaption = 1;
 	gsm->encoding = GSM_ADV_OPT;
@@ -2691,7 +2710,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
 	c->initiator = gsm->initiator;
 	c->t1 = gsm->t1;
 	c->t2 = gsm->t2;
-	c->t3 = 0;	/* Not supported */
+	c->t3 = gsm->t3;
 	c->n2 = gsm->n2;
 	if (gsm->ftype == UIH)
 		c->i = 1;
@@ -2700,7 +2719,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
 	pr_debug("Ftype %d i %d\n", gsm->ftype, c->i);
 	c->mru = gsm->mru;
 	c->mtu = gsm->mtu;
-	c->k = 0;
+	c->k = gsm->k;
 }
 
 static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
@@ -2717,12 +2736,16 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 		return -EINVAL;
 	if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
 		return -EINVAL;
+	if (c->t3 > 255)
+		return -EINVAL;
 	if (c->n2 > 255)
 		return -EINVAL;
 	if (c->encapsulation > 1)	/* Basic, advanced, no I */
 		return -EINVAL;
 	if (c->initiator > 1)
 		return -EINVAL;
+	if (c->k > 7)
+		return -EINVAL;
 	if (c->i == 0 || c->i > 2)	/* UIH and UI only */
 		return -EINVAL;
 	/*
@@ -2770,6 +2793,10 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 		gsm->t1 = c->t1;
 	if (c->t2)
 		gsm->t2 = c->t2;
+	if (c->t3)
+		gsm->t3 = c->t3;
+	if (c->k)
+		gsm->k = c->k;
 
 	/*
 	 * FIXME: We need to separate activation/deactivation from adding
@@ -3298,9 +3325,9 @@ static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
 		pr_err("alloc_netdev failed\n");
 		return -ENOMEM;
 	}
-	net->mtu = dlci->gsm->mtu;
+	net->mtu = dlci->mtu;
 	net->min_mtu = MIN_UNIT_SIZE;
-	net->max_mtu = dlci->gsm->mtu;
+	net->max_mtu = dlci->mtu;
 	mux_net = netdev_priv(net);
 	mux_net->dlci = dlci;
 	kref_init(&mux_net->ref);
-- 
2.34.1

[PATCH 3/3] tty: n_gsm: add parameter negotiation support

[D. Starke]

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

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 5.1.8.1.1 describes the parameter negotiation
messages and parameters. Chapter 5.4.1 states that the default parameters
are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
the encoding of the parameter negotiation message. The meaning of the
parameters and allowed value ranges can be found in chapter 5.7.

Add parameter negotiation support accordingly. DLCI specific parameter
configuration by the user requires additional ioctls. This is subject to another
patch.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index b6813a134c18..c6fe00afe1b2 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -123,6 +123,7 @@ struct gsm_msg {
 
 enum gsm_dlci_state {
 	DLCI_CLOSED,
+	DLCI_CONFIGURE,		/* Sending PN (for adaption > 1) */
 	DLCI_OPENING,		/* Sending SABM not seen UA */
 	DLCI_OPEN,		/* SABM/UA complete */
 	DLCI_CLOSING,		/* Sending DISC not seen UA/DM */
@@ -406,6 +407,7 @@ static const u8 gsm_fcs8[256] = {
 #define INIT_FCS	0xFF
 #define GOOD_FCS	0xCF
 
+static void gsm_dlci_close(struct gsm_dlci *dlci);
 static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
 static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
 static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
@@ -528,6 +530,55 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
 	kfree(prefix);
 }
 
+/**
+ * gsm_encode_params	-	encode DLCI parameters
+ * @dlci: DLCI to encode from
+ * @data: 8 byte buffer for encoded data
+ * @dlen: length of buffer
+ *
+ * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
+ * table 3.
+ */
+static int gsm_encode_params(const struct gsm_dlci *dlci, u8 *data,
+			     unsigned int dlen)
+{
+	struct gsm_mux *gsm = dlci->gsm;
+
+	if (dlen < 8)
+		return -EINVAL;
+
+	data[0] = dlci->addr;
+	data[1] = 0x00; /* UIH, convergence layer type 1 */
+	data[2] = dlci->prio;
+	data[3] = gsm->t1;
+	data[4] = dlci->mtu & 0xFF;
+	data[5] = (dlci->mtu >> 8) & 0xFF;
+	data[6] = gsm->n2;
+	data[7] = dlci->k;
+
+	if (dlci->ftype == UI) {
+		data[1] = 0x01; /* UI */
+	} else if (dlci->ftype != UIH) {
+		pr_err("%s: unsupported frame type %d\n", __func__,
+		       dlci->ftype);
+		return -EINVAL;
+	}
+
+	switch (dlci->adaption) {
+	case 1: /* Unstructured */
+		break;
+	case 2: /* Unstructured with modem bits. */
+		data[1] |= 0x10; /* convergence layer type 2 */
+		break;
+	default:
+		pr_err("%s: unsupported adaption %d\n", __func__,
+		       dlci->adaption);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  *	gsm_register_devices	-	register all tty devices for a given mux index
  *
@@ -1445,6 +1496,122 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	dlci->modem_rx = mlines;
 }
 
+/**
+ * gsm_process_negotiation	-	process received parameters
+ * @gsm: GSM channel
+ * @addr: DLCI address
+ * @cr: command/response
+ * @data: data following command
+ * @clen: length of data
+ *
+ * Used when the response for our parameter negotiation command was
+ * received.
+ */
+static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
+				   unsigned int cr, const u8 *data,
+				   unsigned int clen)
+{
+	struct gsm_dlci *dlci = gsm->dlci[addr];
+	unsigned int ftype, i, adaption, prio, n1, k;
+
+	if (clen < 8)
+		return -EINVAL;
+
+	i = data[1] & 0x0F;
+	adaption = ((data[1] >> 4) & 0x0F) + 1;
+	prio = data[2] & 0x3F;
+	/* t1 = data[3]; */
+	n1 = data[4] | (data[5] << 8);
+	/* n2 = data[6]; */
+	k = data[7] & 0x07;
+
+	if (n1 < MIN_UNIT_SIZE) {
+		if (debug & DBG_ERRORS)
+			pr_info("%s N1 out of range in PN\n", __func__);
+		return -EINVAL;
+	}
+
+	switch (i) {
+	case 0x00:
+		ftype = UIH;
+		break;
+	case 0x01:
+		ftype = UI;
+		break;
+	case 0x02: /* I frames are not supported */
+		if (debug & DBG_ERRORS)
+			pr_info("%s unsupported I frame request in PN\n",
+				__func__);
+		return -EINVAL;
+	default:
+		if (debug & DBG_ERRORS)
+			pr_info("%s i out of range in PN\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!cr && gsm->initiator) {
+		if (adaption != dlci->adaption) {
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid adaption %d in PN\n",
+					__func__, adaption);
+			return -EINVAL;
+		}
+		if (prio != dlci->prio) {
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid priority %d in PN",
+					__func__, prio);
+			return -EINVAL;
+		}
+		if (n1 > gsm->mru || n1 > dlci->mtu) {
+			/* We requested a frame size but the other party wants
+			 * to send larger frames. The standard allows only a
+			 * smaller response value than requested (5.4.6.3.1).
+			 */
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid N1 %d in PN\n", __func__,
+					n1);
+			return -EINVAL;
+		}
+		dlci->mtu = n1;
+		if (ftype != dlci->ftype) {
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid i %d in PN\n", __func__, i);
+			return -EINVAL;
+		}
+		if (ftype != UI && ftype != UIH && k > dlci->k) {
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid k %d in PN\n", __func__, k);
+			return -EINVAL;
+		}
+		dlci->k = k;
+	} else if (cr && !gsm->initiator) {
+		/* Only convergence layer type 1 and 2 are supported. */
+		if (adaption != 1 && adaption != 2) {
+			if (debug & DBG_ERRORS)
+				pr_info("%s invalid adaption %d in PN\n",
+					__func__, adaption);
+			return -EINVAL;
+		}
+		dlci->adaption = adaption;
+		if (n1 > gsm->mru) {
+			/* Propose a smaller value */
+			dlci->mtu = gsm->mru;
+		} else if (n1 > MAX_MTU) {
+			/* Propose a smaller value */
+			dlci->mtu = MAX_MTU;
+		} else {
+			dlci->mtu = n1;
+		}
+		dlci->prio = prio;
+		dlci->ftype = ftype;
+		dlci->k = k;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  *	gsm_control_modem	-	modem status received
  *	@gsm: GSM channel
@@ -1498,6 +1665,62 @@ static void gsm_control_modem(struct gsm_mux *gsm, const u8 *data, int clen)
 	gsm_control_reply(gsm, CMD_MSC, data, clen);
 }
 
+/**
+ * gsm_control_negotiation	-	parameter negotiation received
+ * @gsm: GSM channel
+ * @cr: command/response flag
+ * @data: data following command
+ * @dlen: data length
+ *
+ * We have received a parameter negotiation message. This is used by
+ * the GSM mux protocol to configure protocol parameters for a new DLCI.
+ */
+static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
+				    const u8 *data, unsigned int dlen)
+{
+	unsigned int addr;
+	u8 params[8];
+	struct gsm_dlci *dlci;
+
+	if (dlen < 8)
+		return;
+
+	/* Invalid DLCI? */
+	addr = data[0] & 0x3F;
+	if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr])
+		return;
+	dlci = gsm->dlci[addr];
+
+	/* Too late for parameter negotiation? */
+	if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN)
+		return;
+
+	/* Process the received parameters */
+	if (gsm_process_negotiation(gsm, addr, cr, data, dlen) != 0) {
+		/* Negotiation failed. Close the link. */
+		if (debug & DBG_ERRORS)
+			pr_info("%s PN failed\n", __func__);
+		gsm_dlci_close(dlci);
+		return;
+	}
+
+	if (cr) {
+		/* Reply command with accepted parameters. */
+		if (gsm_encode_params(dlci, params, sizeof(params)) == 0)
+			gsm_control_reply(gsm, CMD_PN, params, sizeof(params));
+		else if (debug & DBG_ERRORS)
+			pr_info("%s PN invalid\n", __func__);
+	} else if (dlci->state == DLCI_CONFIGURE) {
+		/* Proceed with link setup by sending SABM before UA */
+		dlci->state = DLCI_OPENING;
+		gsm_command(gsm, dlci->addr, SABM|PF);
+		mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+	} else {
+		if (debug & DBG_ERRORS)
+			pr_info("%s PN in invalid state\n", __func__);
+	}
+}
+
 /**
  *	gsm_control_rls		-	remote line status
  *	@gsm: GSM channel
@@ -1607,8 +1830,12 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 		/* Modem wishes to enter power saving state */
 		gsm_control_reply(gsm, CMD_PSC, NULL, 0);
 		break;
+		/* Optional commands */
+	case CMD_PN:
+		/* Modem sends a parameter negotiation command */
+		gsm_control_negotiation(gsm, 1, data, clen);
+		break;
 		/* Optional unsupported commands */
-	case CMD_PN:	/* Parameter negotiation */
 	case CMD_RPN:	/* Remote port negotiation */
 	case CMD_SNC:	/* Service negotiation command */
 	default:
@@ -1641,8 +1868,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 	spin_lock_irqsave(&gsm->control_lock, flags);
 
 	ctrl = gsm->pending_cmd;
-	/* Does the reply match our command */
 	command |= 1;
+	/* Does the reply match our command */
 	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
 		/* Our command was replied to, kill the retry timer */
 		del_timer(&gsm->t2_timer);
@@ -1652,6 +1879,9 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 			ctrl->error = -EOPNOTSUPP;
 		ctrl->done = 1;
 		wake_up(&gsm->event);
+	/* Or did we receive the PN response to our PN command */
+	} else if (command == CMD_PN) {
+		gsm_control_negotiation(gsm, 0, data, clen);
 	}
 	spin_unlock_irqrestore(&gsm->control_lock, flags);
 }
@@ -1829,6 +2059,31 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	wake_up(&dlci->gsm->event);
 }
 
+/**
+ * gsm_dlci_negotiate	-	start parameter negotiation
+ * @dlci: DLCI to open
+ *
+ * Starts the parameter negotiation for the new DLCI. This needs to be done
+ * before the DLCI initialized the channel via SABM.
+ */
+static int gsm_dlci_negotiate(struct gsm_dlci *dlci)
+{
+	struct gsm_mux *gsm = dlci->gsm;
+	u8 params[8];
+	int ret;
+
+	ret = gsm_encode_params(dlci, params, sizeof(params));
+	if (ret != 0)
+		return ret;
+
+	/* We cannot asynchronous wait for the command response with
+	 * gsm_command() and gsm_control_wait() at this point.
+	 */
+	ret = gsm_control_command(gsm, CMD_PN, params, sizeof(params));
+
+	return ret;
+}
+
 /**
  *	gsm_dlci_t1		-	T1 timer expiry
  *	@t: timer contained in the DLCI that opened
@@ -1850,6 +2105,14 @@ static void gsm_dlci_t1(struct timer_list *t)
 	struct gsm_mux *gsm = dlci->gsm;
 
 	switch (dlci->state) {
+	case DLCI_CONFIGURE:
+		if (dlci->retries && gsm_dlci_negotiate(dlci) == 0) {
+			dlci->retries--;
+			mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+		} else {
+			gsm_dlci_begin_close(dlci); /* prevent half open link */
+		}
+		break;
 	case DLCI_OPENING:
 		if (dlci->retries) {
 			dlci->retries--;
@@ -1888,17 +2151,46 @@ static void gsm_dlci_t1(struct timer_list *t)
  *	to the modem which should then reply with a UA or ADM, at which point
  *	we will move into open state. Opening is done asynchronously with retry
  *	running off timers and the responses.
+ *	Parameter negotiation is performed before SABM if required.
  */
 
 static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 {
-	struct gsm_mux *gsm = dlci->gsm;
-	if (dlci->state == DLCI_OPEN || dlci->state == DLCI_OPENING)
+	struct gsm_mux *gsm = dlci ? dlci->gsm : NULL;
+	bool need_pn = false;
+
+	if (!gsm)
 		return;
-	dlci->retries = gsm->n2;
-	dlci->state = DLCI_OPENING;
-	gsm_command(dlci->gsm, dlci->addr, SABM|PF);
-	mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+
+	if (dlci->addr != 0) {
+		if (gsm->adaption != 1 || gsm->adaption != dlci->adaption)
+			need_pn = true;
+		if (dlci->prio != ((((dlci->addr / 8) + 1) * 8) - 1))
+			need_pn = true;
+		if (gsm->ftype != dlci->ftype)
+			need_pn = true;
+	}
+
+	switch (dlci->state) {
+	case DLCI_CLOSED:
+	case DLCI_CLOSING:
+		dlci->retries = gsm->n2;
+		if (!need_pn) {
+			dlci->state = DLCI_OPENING;
+			gsm_command(gsm, dlci->addr, SABM|PF);
+		} else {
+			/* Configure DLCI before setup */
+			dlci->state = DLCI_CONFIGURE;
+			if (gsm_dlci_negotiate(dlci) != 0) {
+				gsm_dlci_close(dlci);
+				return;
+			}
+		}
+		mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
+		break;
+	default:
+		break;
+	}
 }
 
 /**
-- 
2.34.1

Re: [PATCH 1/3] tty: n_gsm: introduce macro for minimal unit size

[Ilpo Järvinen]

On Fri, 21 Oct 2022, D. Starke wrote:

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> n_gsm has a minimal protocol overhead of 7 bytes. The current code already
> checks whether the configured MRU/MTU size is at least one byte more than
> this.
> 
> Introduce the macro MIN_UNIT_SIZE to make this value more obvious.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5e516f5cac5a..8e039f2a0427 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -89,6 +89,7 @@ module_param(debug, int, 0600);
>   */
>  #define MAX_MRU 1500
>  #define MAX_MTU 1500
> +#define MIN_UNIT_SIZE 8
>  /* SOF, ADDR, CTRL, LEN1, LEN2, ..., FCS, EOF */
>  #define PROT_OVERHEAD 7

Why not call it just MIN_MTU?

I know you check it also against MRU but that seems so minor problem to me 
it's not worth even noting because of course MRU is related MTU, it's just 
the other end of the pipe. To be honest, I don't understand why MAX_MRU is 
even defined there when is just the same as MAX_MTU :-).

You could do this btw:

#define MIN_MTU	(PROT_OVERHEAD + 1)

To make it even more obvious where it comes from (matching to what you 
describe in your commit message).

The change looks otherwise ok.

-- 
 i.


>  #define	GSM_NET_TX_TIMEOUT (HZ*10)
> @@ -2712,7 +2713,9 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  	if ((c->adaption != 1 && c->adaption != 2) || c->k)
>  		return -EOPNOTSUPP;
>  	/* Check the MRU/MTU range looks sane */
> -	if (c->mru > MAX_MRU || c->mtu > MAX_MTU || c->mru < 8 || c->mtu < 8)
> +	if (c->mru < MIN_UNIT_SIZE || c->mtu < MIN_UNIT_SIZE)
> +		return -EINVAL;
> +	if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
>  		return -EINVAL;
>  	if (c->n2 > 255)
>  		return -EINVAL;
> @@ -3296,7 +3299,7 @@ static int gsm_create_network(struct gsm_dlci *dlci, struct gsm_netconfig *nc)
>  		return -ENOMEM;
>  	}
>  	net->mtu = dlci->gsm->mtu;
> -	net->min_mtu = 8;
> +	net->min_mtu = MIN_UNIT_SIZE;
>  	net->max_mtu = dlci->gsm->mtu;
>  	mux_net = netdev_priv(net);
>  	mux_net->dlci = dlci;
> 

Re: [PATCH 2/3] tty: n_gsm: add parameters used with parameter negotiation

[Ilpo Järvinen]

On Fri, 21 Oct 2022, D. Starke wrote:

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapter 5.4.6.3.1 describes the encoding of the
> parameter negotiation messages.
> 
> Add the parameters used there to 'gsm_mux' and 'gsm_dlci' and initialize both
> according to the value ranges and recommended defaults defined in chapter 5.7.
> 
> Replace the use of the DLC default values from the 'gsm_mux' fields with the DLC
> specific values from the 'gsm_dlci' fields where applicable.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---

> @@ -2075,6 +2085,13 @@ static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
>  	dlci->gsm = gsm;
>  	dlci->addr = addr;
>  	dlci->adaption = gsm->adaption;
> +	dlci->mtu = gsm->mtu;
> +	if (addr == 0)
> +		dlci->prio = 0;
> +	else
> +		dlci->prio = ((((addr / 8) + 1) * 8) - 1);

Is this doing roundup(addr, 8) - 1 (in linux/math.h)?

> +	dlci->ftype = gsm->ftype;
> +	dlci->k = gsm->k;
>  	dlci->state = DLCI_CLOSED;
>  	if (addr) {
>  		dlci->data = gsm_dlci_data;
> @@ -2650,7 +2667,9 @@ static struct gsm_mux *gsm_alloc_mux(void)
>  
>  	gsm->t1 = T1;
>  	gsm->t2 = T2;
> +	gsm->t3 = T3;
>  	gsm->n2 = N2;
> +	gsm->k = K;
>  	gsm->ftype = UIH;
>  	gsm->adaption = 1;
>  	gsm->encoding = GSM_ADV_OPT;
> @@ -2691,7 +2710,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
>  	c->initiator = gsm->initiator;
>  	c->t1 = gsm->t1;
>  	c->t2 = gsm->t2;
> -	c->t3 = 0;	/* Not supported */
> +	c->t3 = gsm->t3;
>  	c->n2 = gsm->n2;
>  	if (gsm->ftype == UIH)
>  		c->i = 1;
> @@ -2700,7 +2719,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
>  	pr_debug("Ftype %d i %d\n", gsm->ftype, c->i);
>  	c->mru = gsm->mru;
>  	c->mtu = gsm->mtu;
> -	c->k = 0;
> +	c->k = gsm->k;
>  }
>  
>  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
> @@ -2717,12 +2736,16 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  		return -EINVAL;
>  	if (c->mru > MAX_MRU || c->mtu > MAX_MTU)
>  		return -EINVAL;
> +	if (c->t3 > 255)
> +		return -EINVAL;
>  	if (c->n2 > 255)
>  		return -EINVAL;
>  	if (c->encapsulation > 1)	/* Basic, advanced, no I */
>  		return -EINVAL;
>  	if (c->initiator > 1)
>  		return -EINVAL;
> +	if (c->k > 7)

Please don't add non-obvious literal such as this. Is this xx_MAX_WINDOW 
or something along those lines?

-- 
 i.

Re: [PATCH 3/3] tty: n_gsm: add parameter negotiation support

[Ilpo Järvinen]

On Fri, 21 Oct 2022, D. Starke wrote:

> From: Daniel Starke <daniel.starke@siemens.com>
> 
> n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
> See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
> The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
> the newer 27.010 here. Chapter 5.1.8.1.1 describes the parameter negotiation
> messages and parameters. Chapter 5.4.1 states that the default parameters
> are to be used if no negotiation is performed. Chapter 5.4.6.3.1 describes
> the encoding of the parameter negotiation message. The meaning of the
> parameters and allowed value ranges can be found in chapter 5.7.
> 
> Add parameter negotiation support accordingly. DLCI specific parameter
> configuration by the user requires additional ioctls. This is subject to another
> patch.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 308 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 300 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index b6813a134c18..c6fe00afe1b2 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -123,6 +123,7 @@ struct gsm_msg {
>  
>  enum gsm_dlci_state {
>  	DLCI_CLOSED,
> +	DLCI_CONFIGURE,		/* Sending PN (for adaption > 1) */
>  	DLCI_OPENING,		/* Sending SABM not seen UA */
>  	DLCI_OPEN,		/* SABM/UA complete */
>  	DLCI_CLOSING,		/* Sending DISC not seen UA/DM */
> @@ -406,6 +407,7 @@ static const u8 gsm_fcs8[256] = {
>  #define INIT_FCS	0xFF
>  #define GOOD_FCS	0xCF
>  
> +static void gsm_dlci_close(struct gsm_dlci *dlci);
>  static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len);
>  static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
>  static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
> @@ -528,6 +530,55 @@ static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
>  	kfree(prefix);
>  }
>  
> +/**
> + * gsm_encode_params	-	encode DLCI parameters
> + * @dlci: DLCI to encode from
> + * @data: 8 byte buffer for encoded data
> + * @dlen: length of buffer
> + *
> + * Encodes the parameters according to GSM 07.10 section 5.4.6.3.1
> + * table 3.
> + */
> +static int gsm_encode_params(const struct gsm_dlci *dlci, u8 *data,
> +			     unsigned int dlen)
> +{
> +	struct gsm_mux *gsm = dlci->gsm;
> +
> +	if (dlen < 8)
> +		return -EINVAL;

Should this be MIN_MTU?

> +	data[0] = dlci->addr;
> +	data[1] = 0x00; /* UIH, convergence layer type 1 */
> +	data[2] = dlci->prio;
> +	data[3] = gsm->t1;
> +	data[4] = dlci->mtu & 0xFF;
> +	data[5] = (dlci->mtu >> 8) & 0xFF;
> +	data[6] = gsm->n2;
> +	data[7] = dlci->k;

Magic offsets, shouldn't you define a struct and assign to the named 
fields (and use the correct endian type + accessors for that two-byte 
field).

> +	if (dlci->ftype == UI) {
> +		data[1] = 0x01; /* UI */
> +	} else if (dlci->ftype != UIH) {
> +		pr_err("%s: unsupported frame type %d\n", __func__,
> +		       dlci->ftype);
> +		return -EINVAL;
> +	}
> +
> +	switch (dlci->adaption) {
> +	case 1: /* Unstructured */
> +		break;
> +	case 2: /* Unstructured with modem bits. */
> +		data[1] |= 0x10; /* convergence layer type 2 */
> +		break;
> +	default:
> +		pr_err("%s: unsupported adaption %d\n", __func__,
> +		       dlci->adaption);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   *	gsm_register_devices	-	register all tty devices for a given mux index
>   *
> @@ -1445,6 +1496,122 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
>  	dlci->modem_rx = mlines;
>  }
>  
> +/**
> + * gsm_process_negotiation	-	process received parameters
> + * @gsm: GSM channel
> + * @addr: DLCI address
> + * @cr: command/response
> + * @data: data following command
> + * @clen: length of data
> + *
> + * Used when the response for our parameter negotiation command was
> + * received.
> + */
> +static int gsm_process_negotiation(struct gsm_mux *gsm, unsigned int addr,
> +				   unsigned int cr, const u8 *data,
> +				   unsigned int clen)
> +{
> +	struct gsm_dlci *dlci = gsm->dlci[addr];
> +	unsigned int ftype, i, adaption, prio, n1, k;
> +
> +	if (clen < 8)
> +		return -EINVAL;
> +
> +	i = data[1] & 0x0F;
> +	adaption = ((data[1] >> 4) & 0x0F) + 1;
> +	prio = data[2] & 0x3F;
> +	/* t1 = data[3]; */
> +	n1 = data[4] | (data[5] << 8);
> +	/* n2 = data[6]; */
> +	k = data[7] & 0x07;

Magic offsets.

Define these fields properly too with names and GENMASK. Use FIELD_GET() 
to extract values.

> +	if (n1 < MIN_UNIT_SIZE) {
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s N1 out of range in PN\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	switch (i) {
> +	case 0x00:
> +		ftype = UIH;
> +		break;
> +	case 0x01:
> +		ftype = UI;
> +		break;
> +	case 0x02: /* I frames are not supported */
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s unsupported I frame request in PN\n",
> +				__func__);
> +		return -EINVAL;
> +	default:
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s i out of range in PN\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!cr && gsm->initiator) {
> +		if (adaption != dlci->adaption) {
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid adaption %d in PN\n",
> +					__func__, adaption);
> +			return -EINVAL;
> +		}
> +		if (prio != dlci->prio) {
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid priority %d in PN",
> +					__func__, prio);
> +			return -EINVAL;
> +		}
> +		if (n1 > gsm->mru || n1 > dlci->mtu) {
> +			/* We requested a frame size but the other party wants
> +			 * to send larger frames. The standard allows only a
> +			 * smaller response value than requested (5.4.6.3.1).
> +			 */
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid N1 %d in PN\n", __func__,
> +					n1);
> +			return -EINVAL;
> +		}
> +		dlci->mtu = n1;
> +		if (ftype != dlci->ftype) {
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid i %d in PN\n", __func__, i);
> +			return -EINVAL;
> +		}
> +		if (ftype != UI && ftype != UIH && k > dlci->k) {
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid k %d in PN\n", __func__, k);
> +			return -EINVAL;
> +		}
> +		dlci->k = k;
> +	} else if (cr && !gsm->initiator) {
> +		/* Only convergence layer type 1 and 2 are supported. */
> +		if (adaption != 1 && adaption != 2) {
> +			if (debug & DBG_ERRORS)
> +				pr_info("%s invalid adaption %d in PN\n",
> +					__func__, adaption);
> +			return -EINVAL;
> +		}
> +		dlci->adaption = adaption;
> +		if (n1 > gsm->mru) {
> +			/* Propose a smaller value */
> +			dlci->mtu = gsm->mru;
> +		} else if (n1 > MAX_MTU) {
> +			/* Propose a smaller value */
> +			dlci->mtu = MAX_MTU;
> +		} else {
> +			dlci->mtu = n1;
> +		}
> +		dlci->prio = prio;
> +		dlci->ftype = ftype;
> +		dlci->k = k;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   *	gsm_control_modem	-	modem status received
>   *	@gsm: GSM channel
> @@ -1498,6 +1665,62 @@ static void gsm_control_modem(struct gsm_mux *gsm, const u8 *data, int clen)
>  	gsm_control_reply(gsm, CMD_MSC, data, clen);
>  }
>  
> +/**
> + * gsm_control_negotiation	-	parameter negotiation received
> + * @gsm: GSM channel
> + * @cr: command/response flag
> + * @data: data following command
> + * @dlen: data length
> + *
> + * We have received a parameter negotiation message. This is used by
> + * the GSM mux protocol to configure protocol parameters for a new DLCI.
> + */
> +static void gsm_control_negotiation(struct gsm_mux *gsm, unsigned int cr,
> +				    const u8 *data, unsigned int dlen)
> +{
> +	unsigned int addr;
> +	u8 params[8];
> +	struct gsm_dlci *dlci;
> +
> +	if (dlen < 8)
> +		return;

MIN_MTU?

> +
> +	/* Invalid DLCI? */
> +	addr = data[0] & 0x3F;

#define + FIELD_GET()

> +	if (addr == 0 || addr >= NUM_DLCI || !gsm->dlci[addr])
> +		return;
> +	dlci = gsm->dlci[addr];
> +
> +	/* Too late for parameter negotiation? */
> +	if ((!cr && dlci->state == DLCI_OPENING) || dlci->state == DLCI_OPEN)
> +		return;
> +
> +	/* Process the received parameters */
> +	if (gsm_process_negotiation(gsm, addr, cr, data, dlen) != 0) {
> +		/* Negotiation failed. Close the link. */
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s PN failed\n", __func__);
> +		gsm_dlci_close(dlci);
> +		return;
> +	}
> +
> +	if (cr) {
> +		/* Reply command with accepted parameters. */
> +		if (gsm_encode_params(dlci, params, sizeof(params)) == 0)
> +			gsm_control_reply(gsm, CMD_PN, params, sizeof(params));
> +		else if (debug & DBG_ERRORS)
> +			pr_info("%s PN invalid\n", __func__);
> +	} else if (dlci->state == DLCI_CONFIGURE) {
> +		/* Proceed with link setup by sending SABM before UA */
> +		dlci->state = DLCI_OPENING;
> +		gsm_command(gsm, dlci->addr, SABM|PF);
> +		mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> +	} else {
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s PN in invalid state\n", __func__);
> +	}
> +}
> +
>  /**
>   *	gsm_control_rls		-	remote line status
>   *	@gsm: GSM channel
> @@ -1607,8 +1830,12 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
>  		/* Modem wishes to enter power saving state */
>  		gsm_control_reply(gsm, CMD_PSC, NULL, 0);
>  		break;
> +		/* Optional commands */
> +	case CMD_PN:
> +		/* Modem sends a parameter negotiation command */
> +		gsm_control_negotiation(gsm, 1, data, clen);
> +		break;
>  		/* Optional unsupported commands */
> -	case CMD_PN:	/* Parameter negotiation */
>  	case CMD_RPN:	/* Remote port negotiation */
>  	case CMD_SNC:	/* Service negotiation command */
>  	default:
> @@ -1641,8 +1868,8 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  	spin_lock_irqsave(&gsm->control_lock, flags);
>  
>  	ctrl = gsm->pending_cmd;
> -	/* Does the reply match our command */
>  	command |= 1;
> +	/* Does the reply match our command */
>  	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
>  		/* Our command was replied to, kill the retry timer */
>  		del_timer(&gsm->t2_timer);
> @@ -1652,6 +1879,9 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  			ctrl->error = -EOPNOTSUPP;
>  		ctrl->done = 1;
>  		wake_up(&gsm->event);
> +	/* Or did we receive the PN response to our PN command */
> +	} else if (command == CMD_PN) {
> +		gsm_control_negotiation(gsm, 0, data, clen);
>  	}
>  	spin_unlock_irqrestore(&gsm->control_lock, flags);
>  }
> @@ -1829,6 +2059,31 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
>  	wake_up(&dlci->gsm->event);
>  }
>  
> +/**
> + * gsm_dlci_negotiate	-	start parameter negotiation
> + * @dlci: DLCI to open
> + *
> + * Starts the parameter negotiation for the new DLCI. This needs to be done
> + * before the DLCI initialized the channel via SABM.
> + */
> +static int gsm_dlci_negotiate(struct gsm_dlci *dlci)
> +{
> +	struct gsm_mux *gsm = dlci->gsm;
> +	u8 params[8];
> +	int ret;
> +
> +	ret = gsm_encode_params(dlci, params, sizeof(params));
> +	if (ret != 0)
> +		return ret;
> +
> +	/* We cannot asynchronous wait for the command response with
> +	 * gsm_command() and gsm_control_wait() at this point.
> +	 */
> +	ret = gsm_control_command(gsm, CMD_PN, params, sizeof(params));
> +
> +	return ret;
> +}
> +
>  /**
>   *	gsm_dlci_t1		-	T1 timer expiry
>   *	@t: timer contained in the DLCI that opened
> @@ -1850,6 +2105,14 @@ static void gsm_dlci_t1(struct timer_list *t)
>  	struct gsm_mux *gsm = dlci->gsm;
>  
>  	switch (dlci->state) {
> +	case DLCI_CONFIGURE:
> +		if (dlci->retries && gsm_dlci_negotiate(dlci) == 0) {
> +			dlci->retries--;
> +			mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);

I'd tend to think a helper for this wouldn't hurt. There are already a 
few of them.

> +		} else {
> +			gsm_dlci_begin_close(dlci); /* prevent half open link */
> +		}
> +		break;
>  	case DLCI_OPENING:
>  		if (dlci->retries) {
>  			dlci->retries--;
> @@ -1888,17 +2151,46 @@ static void gsm_dlci_t1(struct timer_list *t)
>   *	to the modem which should then reply with a UA or ADM, at which point
>   *	we will move into open state. Opening is done asynchronously with retry
>   *	running off timers and the responses.
> + *	Parameter negotiation is performed before SABM if required.
>   */
>  
>  static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>  {
> -	struct gsm_mux *gsm = dlci->gsm;
> -	if (dlci->state == DLCI_OPEN || dlci->state == DLCI_OPENING)
> +	struct gsm_mux *gsm = dlci ? dlci->gsm : NULL;
> +	bool need_pn = false;
> +
> +	if (!gsm)
>  		return;
> -	dlci->retries = gsm->n2;
> -	dlci->state = DLCI_OPENING;
> -	gsm_command(dlci->gsm, dlci->addr, SABM|PF);
> -	mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> +
> +	if (dlci->addr != 0) {
> +		if (gsm->adaption != 1 || gsm->adaption != dlci->adaption)
> +			need_pn = true;
> +		if (dlci->prio != ((((dlci->addr / 8) + 1) * 8) - 1))

roundup(addr, 8) - 1 again?

-- 
 i.


> +			need_pn = true;
> +		if (gsm->ftype != dlci->ftype)
> +			need_pn = true;
> +	}
> +
> +	switch (dlci->state) {
> +	case DLCI_CLOSED:
> +	case DLCI_CLOSING:
> +		dlci->retries = gsm->n2;
> +		if (!need_pn) {
> +			dlci->state = DLCI_OPENING;
> +			gsm_command(gsm, dlci->addr, SABM|PF);
> +		} else {
> +			/* Configure DLCI before setup */
> +			dlci->state = DLCI_CONFIGURE;
> +			if (gsm_dlci_negotiate(dlci) != 0) {
> +				gsm_dlci_close(dlci);
> +				return;
> +			}
> +		}
> +		mod_timer(&dlci->t1, jiffies + gsm->t1 * HZ / 100);
> +		break;
> +	default:
> +		break;
> +	}
>  }
>  
>  /**
>