[PATCH 0/8] tty: n_gsm: Make mux work as a responder station

[PATCH 0/8] tty: n_gsm: Make mux work as a responder station

[Andrej Krpic]

When using n_gsm you have to explicitly set it to work as a initiator
station. This led me to believe that it can also work as a responder.

In a use case where I needed responder station mux I came to the
conclusion that it actually does not work. Second and third patch
fix dealings with frame C/R bit that then enable mux to work as a
responder station.

Next patches in the series then fix bugs that were found after two 
instances of n_gsm connected over null-modem cable were used.

First patch fixes formatting errors, such as space before comma, and
most of the warnings reported by the checkpatch script.


Andrej Krpic (8):
  tty: n_gsm: fix formatting errors
  tty: n_gsm: fix C/R bit when sending as a responder
  tty: n_gsm: make mux work as a responder station
  tty: n_gsm: send DM response when accessing an invalid channel
  tty: n_gsm: replace dead code with a meaningful comment
  tty: n_gsm: add missing length field in control channel commands
  tty: n_gsm: properly format Modem Status Command message
  tty: n_gsm: Enable reception of frames separated with a single SOF
    marker

 drivers/tty/n_gsm.c | 191 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 123 insertions(+), 68 deletions(-)

-- 
2.7.0

[PATCH 1/8] tty: n_gsm: fix formatting errors

[Andrej Krpic]

Minor formatting changes to remove errors and reduce number of
warnings produced by checkpatch.pl script.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 138 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 95 insertions(+), 43 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index c016207..cc3b374 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -431,6 +431,7 @@ static int gsm_read_ea(unsigned int *val, u8 c)
 static u8 gsm_encode_modem(const struct gsm_dlci *dlci)
 {
 	u8 modembits = 0;
+
 	/* FC is true flow control not modem bits */
 	if (dlci->throttled)
 		modembits |= MDM_FC;
@@ -489,7 +490,8 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
 		if (!(control & 0x01)) {
 			pr_cont("I N(S)%d N(R)%d",
 				(control & 0x0E) >> 1, (control & 0xE0) >> 5);
-		} else switch (control & 0x0F) {
+		} else
+			switch (control & 0x0F) {
 			case RR:
 				pr_cont("RR(%d)", (control & 0xE0) >> 5);
 				break;
@@ -511,6 +513,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
 
 	if (dlen) {
 		int ct = 0;
+
 		while (dlen--) {
 			if (ct % 8 == 0) {
 				pr_cont("\n");
@@ -542,6 +545,7 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
 static int gsm_stuff_frame(const u8 *input, u8 *output, int len)
 {
 	int olen = 0;
+
 	while (len--) {
 		if (*input == GSM1_SOF || *input == GSM1_ESCAPE
 		    || *input == XON || *input == XOFF) {
@@ -695,7 +699,7 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 			len += 2;
 		} else {
 			gsm->txframe[0] = GSM0_SOF;
-			memcpy(gsm->txframe + 1 , msg->data, msg->len);
+			memcpy(gsm->txframe + 1, msg->data, msg->len);
 			gsm->txframe[msg->len + 1] = GSM0_SOF;
 			len = msg->len + 2;
 		}
@@ -711,7 +715,8 @@ static void gsm_data_kick(struct gsm_mux *gsm)
 		/* FIXME: Can eliminate one SOF in many more cases */
 		gsm->tx_bytes -= msg->len;
 		/* For a burst of frames skip the extra SOF within the
-		   burst */
+		 * burst
+		 */
 		skip_sof = 1;
 
 		list_del(&msg->list);
@@ -750,7 +755,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 		*--dp = (msg->addr << 2) | 2 | EA;
 	else
 		*--dp = (msg->addr << 2) | EA;
-	*fcs = gsm_fcs_add_block(INIT_FCS, dp , msg->data - dp);
+	*fcs = gsm_fcs_add_block(INIT_FCS, dp, msg->data - dp);
 	/* Ugly protocol layering violation */
 	if (msg->ctrl == UI || msg->ctrl == (UI|PF))
 		*fcs = gsm_fcs_add_block(*fcs, msg->data, msg->len);
@@ -760,7 +765,8 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 							msg->data, msg->len);
 
 	/* Move the header back and adjust the length, also allow for the FCS
-	   now tacked on the end */
+	 * now tacked on the end
+	 */
 	msg->len += (msg->data - dp) + 1;
 	msg->data = dp;
 
@@ -783,6 +789,7 @@ static void __gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 static void gsm_data_queue(struct gsm_dlci *dlci, struct gsm_msg *msg)
 {
 	unsigned long flags;
+
 	spin_lock_irqsave(&dlci->gsm->tx_lock, flags);
 	__gsm_data_queue(dlci, msg);
 	spin_unlock_irqrestore(&dlci->gsm->tx_lock, flags);
@@ -821,7 +828,8 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 
 		msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
 		/* FIXME: need a timer or something to kick this so it can't
-		   get stuck with no work outstanding and no buffer free */
+		 * get stuck with no work outstanding and no buffer free
+		 */
 		if (msg == NULL)
 			return -ENOMEM;
 		dp = msg->data;
@@ -829,11 +837,13 @@ static int gsm_dlci_data_output(struct gsm_mux *gsm, struct gsm_dlci *dlci)
 		case 1:	/* Unstructured */
 			break;
 		case 2:	/* Unstructed with modem bits.
-		Always one byte as we never send inline break data */
+			 * Always one byte as we never send inline break data
+			 */
 			*dp++ = gsm_encode_modem(dlci);
 			break;
 		}
-		WARN_ON(kfifo_out_locked(dlci->fifo, dp , len, &dlci->lock) != len);
+		WARN_ON(kfifo_out_locked(dlci->fifo, dp, len,
+					 &dlci->lock) != len);
 		__gsm_data_queue(dlci, msg);
 		total_size += size;
 	}
@@ -891,7 +901,8 @@ static int gsm_dlci_data_output_framed(struct gsm_mux *gsm,
 	msg = gsm_data_alloc(gsm, dlci->addr, size, gsm->ftype);
 
 	/* FIXME: need a timer or something to kick this so it can't
-	   get stuck with no work outstanding and no buffer free */
+	 * get stuck with no work outstanding and no buffer free
+	 */
 	if (msg == NULL) {
 		skb_queue_tail(&dlci->skb_list, dlci->skb);
 		dlci->skb = NULL;
@@ -1005,6 +1016,7 @@ static void gsm_control_reply(struct gsm_mux *gsm, int cmd, u8 *data,
 					int dlen)
 {
 	struct gsm_msg *msg;
+
 	msg = gsm_data_alloc(gsm, 0, dlen + 2, gsm->ftype);
 	if (msg == NULL)
 		return;
@@ -1032,9 +1044,10 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	int fc;
 
 	/* The modem status command can either contain one octet (v.24 signals)
-	   or two octets (v.24 signals + break signals). The length field will
-	   either be 2 or 3 respectively. This is specified in section
-	   5.4.6.3.7 of the  27.010 mux spec. */
+	 * or two octets (v.24 signals + break signals). The length field will
+	 * either be 2 or 3 respectively. This is specified in section
+	 * 5.4.6.3.7 of the  27.010 mux spec.
+	 */
 
 	if (clen == 2)
 		modem = modem & 0x7f;
@@ -1308,6 +1321,7 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 static void gsm_control_transmit(struct gsm_mux *gsm, struct gsm_control *ctrl)
 {
 	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 1, gsm->ftype);
+
 	if (msg == NULL)
 		return;
 	msg->data[0] = (ctrl->cmd << 1) | 2 | EA;	/* command */
@@ -1331,6 +1345,7 @@ static void gsm_control_retransmit(unsigned long data)
 	struct gsm_mux *gsm = (struct gsm_mux *)data;
 	struct gsm_control *ctrl;
 	unsigned long flags;
+
 	spin_lock_irqsave(&gsm->control_lock, flags);
 	ctrl = gsm->pending_cmd;
 	if (ctrl) {
@@ -1367,6 +1382,7 @@ static struct gsm_control *gsm_control_send(struct gsm_mux *gsm,
 	struct gsm_control *ctrl = kzalloc(sizeof(struct gsm_control),
 						GFP_KERNEL);
 	unsigned long flags;
+
 	if (ctrl == NULL)
 		return NULL;
 retry:
@@ -1400,6 +1416,7 @@ retry:
 static int gsm_control_wait(struct gsm_mux *gsm, struct gsm_control *control)
 {
 	int err;
+
 	wait_event(gsm->event, control->done == 1);
 	err = control->error;
 	kfree(control);
@@ -1436,7 +1453,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 		dlci->gsm->dead = 1;
 	wake_up(&dlci->gsm->event);
 	/* A DLCI 0 close is a MUX termination so we need to kick that
-	   back to userspace somehow */
+	 * back to userspace somehow
+	 */
 }
 
 /**
@@ -1449,7 +1467,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 static void gsm_dlci_open(struct gsm_dlci *dlci)
 {
 	/* Note that SABM UA .. SABM UA first UA lost can mean that we go
-	   open -> open */
+	 * open -> open
+	 */
 	del_timer(&dlci->t1);
 	/* This will let a tty open continue */
 	dlci->state = DLCI_OPEN;
@@ -1507,6 +1526,7 @@ static void gsm_dlci_t1(unsigned long data)
 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)
 		return;
 	dlci->retries = gsm->n2;
@@ -1529,6 +1549,7 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 static void gsm_dlci_begin_close(struct gsm_dlci *dlci)
 {
 	struct gsm_mux *gsm = dlci->gsm;
+
 	if (dlci->state == DLCI_CLOSED || dlci->state == DLCI_CLOSING)
 		return;
 	dlci->retries = gsm->n2;
@@ -1602,12 +1623,15 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, u8 *data, int len)
 {
 	/* See what command is involved */
 	unsigned int command = 0;
+
 	while (len-- > 0) {
 		if (gsm_read_ea(&command, *data++) == 1) {
 			int clen = *data++;
+
 			len--;
 			/* FIXME: this is properly an EA */
 			clen >>= 1;
+
 			/* Malformed command ? */
 			if (clen > len)
 				return;
@@ -1639,6 +1663,7 @@ static void gsm_dlci_command(struct gsm_dlci *dlci, u8 *data, int len)
 static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr)
 {
 	struct gsm_dlci *dlci = kzalloc(sizeof(struct gsm_dlci), GFP_ATOMIC);
+
 	if (dlci == NULL)
 		return NULL;
 	spin_lock_init(&dlci->lock);
@@ -1711,6 +1736,7 @@ static void gsm_destroy_network(struct gsm_dlci *dlci);
 static void gsm_dlci_release(struct gsm_dlci *dlci)
 {
 	struct tty_struct *tty = tty_port_tty_get(&dlci->port);
+
 	if (tty) {
 		mutex_lock(&dlci->mutex);
 		gsm_destroy_network(dlci);
@@ -1745,15 +1771,17 @@ static void gsm_queue(struct gsm_mux *gsm)
 	u8 cr;
 	int address;
 	/* We have to sneak a look at the packet body to do the FCS.
-	   A somewhat layering violation in the spec */
+	 * A somewhat layering violation in the spec
+	 */
 
 	if ((gsm->control & ~PF) == UI)
 		gsm->fcs = gsm_fcs_add_block(gsm->fcs, gsm->buf, gsm->len);
 	if (gsm->encoding == 0) {
 		/* WARNING: gsm->received_fcs is used for
-		gsm->encoding = 0 only.
-		In this case it contain the last piece of data
-		required to generate final CRC */
+		 * gsm->encoding = 0 only.
+		 * In this case it contain the last piece of data
+		 * required to generate final CRC
+		 */
 		gsm->fcs = gsm_fcs_add(gsm->fcs, gsm->received_fcs);
 	}
 	if (gsm->fcs != GOOD_FCS) {
@@ -1840,7 +1868,6 @@ static void gsm_queue(struct gsm_mux *gsm)
 	return;
 invalid:
 	gsm->malformed++;
-	return;
 }
 
 
@@ -1938,7 +1965,8 @@ static void gsm1_receive(struct gsm_mux *gsm, unsigned char c)
 {
 	if (c == GSM1_SOF) {
 		/* EOF is only valid in frame if we have got to the data state
-		   and received at least one byte (the FCS) */
+		 * and received at least one byte (the FCS)
+		 */
 		if (gsm->state == GSM_DATA && gsm->count) {
 			/* Extract the FCS */
 			gsm->count--;
@@ -1954,7 +1982,8 @@ static void gsm1_receive(struct gsm_mux *gsm, unsigned char c)
 			gsm->state = GSM_START;
 		}
 		/* A SOF in GSM_START means we are still reading idling or
-		   framing bytes */
+		 * framing bytes
+		 */
 		return;
 	}
 
@@ -2048,7 +2077,8 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 	WARN_ON(i == MAX_MUX);
 
 	/* In theory disconnecting DLCI 0 is sufficient but for some
-	   modems this is apparently not the case. */
+	 * modems this is apparently not the case.
+	 */
 	if (dlci) {
 		gc = gsm_control_send(gsm, CMD_CLD, NULL, 0);
 		if (gc)
@@ -2140,6 +2170,7 @@ static void gsm_free_mux(struct gsm_mux *gsm)
 static void gsm_free_muxr(struct kref *ref)
 {
 	struct gsm_mux *gsm = container_of(ref, struct gsm_mux, ref);
+
 	gsm_free_mux(gsm);
 }
 
@@ -2162,6 +2193,7 @@ static inline void mux_put(struct gsm_mux *gsm)
 static struct gsm_mux *gsm_alloc_mux(void)
 {
 	struct gsm_mux *gsm = kzalloc(sizeof(struct gsm_mux), GFP_KERNEL);
+
 	if (gsm == NULL)
 		return NULL;
 	gsm->buf = kmalloc(MAX_MRU + 1, GFP_KERNEL);
@@ -2237,7 +2269,8 @@ static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 		tty_kref_put(gsm->tty);
 	else {
 		/* Don't register device 0 - this is the control channel and not
-		   a usable tty interface */
+		 * a usable tty interface
+		 */
 		base = gsm->num << 6; /* Base for this MUX */
 		for (i = 1; i < NUM_DLCI; i++)
 			tty_register_device(gsm_tty_driver, base + i, NULL);
@@ -2392,9 +2425,8 @@ static void gsmld_write_wakeup(struct tty_struct *tty)
 	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
 	spin_lock_irqsave(&gsm->tx_lock, flags);
 	gsm_data_kick(gsm);
-	if (gsm->tx_bytes < TX_THRESH_LO) {
+	if (gsm->tx_bytes < TX_THRESH_LO)
 		gsm_dlci_data_sweep(gsm);
-	}
 	spin_unlock_irqrestore(&gsm->tx_lock, flags);
 }
 
@@ -2437,6 +2469,7 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
 			   const unsigned char *buf, size_t nr)
 {
 	int space = tty_write_room(tty);
+
 	if (space >= nr)
 		return tty->ops->write(tty, buf, nr);
 	set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
@@ -2549,7 +2582,8 @@ static int gsmld_config(struct tty_struct *tty, struct gsm_mux *gsm,
 		gsm->t2 = c->t2;
 
 	/* FIXME: We need to separate activation/deactivation from adding
-	   and removing from the mux array */
+	 * and removing from the mux array
+	 */
 	if (need_restart)
 		gsm_activate_mux(gsm);
 	if (gsm->initiator && need_close)
@@ -2706,12 +2740,12 @@ static void gsm_mux_rx_netchar(struct gsm_dlci *dlci,
 	STATS(net).rx_packets++;
 	STATS(net).rx_bytes += size;
 	muxnet_put(mux_net);
-	return;
 }
 
 static int gsm_change_mtu(struct net_device *net, int new_mtu)
 {
 	struct gsm_mux_net *mux_net = netdev_priv(net);
+
 	if ((new_mtu < 8) || (new_mtu > mux_net->dlci->gsm->mtu))
 		return -EINVAL;
 	net->mtu = new_mtu;
@@ -2852,6 +2886,7 @@ static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk)
 static int gsm_carrier_raised(struct tty_port *port)
 {
 	struct gsm_dlci *dlci = container_of(port, struct gsm_dlci, port);
+
 	/* Not yet open so no carrier info */
 	if (dlci->state != DLCI_OPEN)
 		return 0;
@@ -2864,6 +2899,7 @@ static void gsm_dtr_rts(struct tty_port *port, int onoff)
 {
 	struct gsm_dlci *dlci = container_of(port, struct gsm_dlci, port);
 	unsigned int modem_tx = dlci->modem_tx;
+
 	if (onoff)
 		modem_tx |= TIOCM_DTR | TIOCM_RTS;
 	else
@@ -2902,9 +2938,10 @@ static int gsmtty_install(struct tty_driver *driver, struct tty_struct *tty)
 	if (gsm->dead)
 		return -EL2HLT;
 	/* If DLCI 0 is not yet fully open return an error.
-	This is ok from a locking
-	perspective as we don't have to worry about this
-	if DLCI0 is lost */
+	 * This is ok from a locking
+	 * perspective as we don't have to worry about this
+	 * if DLCI0 is lost
+	 */
 	mutex_lock(&gsm->mutex);
 	if (gsm->dlci[0] && gsm->dlci[0]->state != DLCI_OPEN) {
 		mutex_unlock(&gsm->mutex);
@@ -2946,7 +2983,8 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 
 	dlci->modem_rx = 0;
 	/* We could in theory open and close before we wait - eg if we get
-	   a DM straight back. This is ok as that will have caused a hangup */
+	 * a DM straight back. This is ok as that will have caused a hangup
+	 */
 	set_bit(ASYNCB_INITIALIZED, &port->flags);
 	/* Start sending off SABM messages */
 	gsm_dlci_begin_open(dlci);
@@ -2976,12 +3014,12 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	}
 	tty_port_close_end(&dlci->port, tty);
 	tty_port_tty_set(&dlci->port, NULL);
-	return;
 }
 
 static void gsmtty_hangup(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return;
 	tty_port_hangup(&dlci->port);
@@ -2993,6 +3031,7 @@ static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 {
 	int sent;
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return -EINVAL;
 	/* Stuff the bytes into the fifo queue */
@@ -3005,6 +3044,7 @@ static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
 static int gsmtty_write_room(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return -EINVAL;
 	return TX_SIZE - kfifo_len(dlci->fifo);
@@ -3013,6 +3053,7 @@ static int gsmtty_write_room(struct tty_struct *tty)
 static int gsmtty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return -EINVAL;
 	return kfifo_len(dlci->fifo);
@@ -3021,12 +3062,14 @@ static int gsmtty_chars_in_buffer(struct tty_struct *tty)
 static void gsmtty_flush_buffer(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return;
 	/* Caution needed: If we implement reliable transport classes
-	   then the data being transmitted can't simply be junked once
-	   it has first hit the stack. Until then we can just blow it
-	   away */
+	 * then the data being transmitted can't simply be junked once
+	 * it has first hit the stack. Until then we can just blow it
+	 * away
+	 */
 	kfifo_reset(dlci->fifo);
 	/* Need to unhook this DLCI from the transmit queue logic */
 }
@@ -3034,13 +3077,15 @@ static void gsmtty_flush_buffer(struct tty_struct *tty)
 static void gsmtty_wait_until_sent(struct tty_struct *tty, int timeout)
 {
 	/* The FIFO handles the queue so the kernel will do the right
-	   thing waiting on chars_in_buffer before calling us. No work
-	   to do here */
+	 * thing waiting on chars_in_buffer before calling us. No work
+	 * to do here
+	 */
 }
 
 static int gsmtty_tiocmget(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return -EINVAL;
 	return dlci->modem_rx;
@@ -3101,19 +3146,22 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 static void gsmtty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return;
 	/* For the moment its fixed. In actual fact the speed information
-	   for the virtual channel can be propogated in both directions by
-	   the RPN control message. This however rapidly gets nasty as we
-	   then have to remap modem signals each way according to whether
-	   our virtual cable is null modem etc .. */
+	 * for the virtual channel can be propogated in both directions by
+	 * the RPN control message. This however rapidly gets nasty as we
+	 * then have to remap modem signals each way according to whether
+	 * our virtual cable is null modem etc ..
+	 */
 	tty_termios_copy_hw(&tty->termios, old);
 }
 
 static void gsmtty_throttle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return;
 	if (C_CRTSCTS(tty))
@@ -3126,6 +3174,7 @@ static void gsmtty_throttle(struct tty_struct *tty)
 static void gsmtty_unthrottle(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
+
 	if (dlci->state == DLCI_CLOSED)
 		return;
 	if (C_CRTSCTS(tty))
@@ -3139,11 +3188,12 @@ static int gsmtty_break_ctl(struct tty_struct *tty, int state)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	int encode = 0;	/* Off */
+
 	if (dlci->state == DLCI_CLOSED)
 		return -EINVAL;
 
-	if (state == -1)	/* "On indefinitely" - we can't encode this
-				    properly */
+	if (state == -1)
+		/* "On indefinitely" - we can't encode this properly */
 		encode = 0x0F;
 	else if (state > 0) {
 		encode = state / 200;	/* mS to encoding */
@@ -3190,6 +3240,7 @@ static int __init gsm_init(void)
 {
 	/* Fill in our line protocol discipline, and register it */
 	int status = tty_register_ldisc(N_GSM0710, &tty_ldisc_packet);
+
 	if (status != 0) {
 		pr_err("n_gsm: can't register line discipline (err = %d)\n",
 								status);
@@ -3231,6 +3282,7 @@ static int __init gsm_init(void)
 static void __exit gsm_exit(void)
 {
 	int status = tty_unregister_ldisc(N_GSM0710);
+
 	if (status != 0)
 		pr_err("n_gsm: can't unregister line discipline (err = %d)\n",
 								status);
-- 
2.7.0

Re: [PATCH 1/8] tty: n_gsm: fix formatting errors

[Joe Perches]

On Sun, 2016-02-21 at 22:38 +0100, Andrej Krpic wrote:
> Minor formatting changes to remove errors and reduce number of
> warnings produced by checkpatch.pl script.
[]
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
[]
> @@ -489,7 +490,8 @@ static void gsm_print_packet(const char *hdr, int addr, int cr,
>  		if (!(control & 0x01)) {
>  			pr_cont("I N(S)%d N(R)%d",
>  				(control & 0x0E) >> 1, (control & 0xE0) >> 5);
> -		} else switch (control & 0x0F) {
> +		} else
> +			switch (control & 0x0F) {

Please follow the brace rule for else uses where
if one branch has braces, the other does too.

Re: [PATCH 1/8] tty: n_gsm: fix formatting errors

[Andrej Krpic]

On 21.02.2016 22:30, Joe Perches wrote:
> On Sun, 2016-02-21 at 22:38 +0100, Andrej Krpic wrote:
>> Minor formatting changes to remove errors and reduce number of
>> warnings produced by checkpatch.pl script.
> []
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> []
>> @@ -489,7 +490,8 @@ static void gsm_print_packet(const char *hdr, 
>> int addr, int cr,
>>  		if (!(control & 0x01)) {
>>  			pr_cont("I N(S)%d N(R)%d",
>>  				(control & 0x0E) >> 1, (control & 0xE0) >> 5);
>> -		} else switch (control & 0x0F) {
>> +		} else
>> +			switch (control & 0x0F) {
>
> Please follow the brace rule for else uses where
> if one branch has braces, the other does too.

Thank you for noticing.

Should I resend this as a single patch or wait for more comments for v2 
series?


-Andrej

[PATCH 2/8] tty: n_gsm: fix C/R bit when sending as a responder

[Andrej Krpic]

According to the specification (3GPP TS 27.010 v12.0.0 R1, 5.2.1.2),
C/R bit must be the same for corresponding command and response.
If mux is an initiator (station that initiates DLC 0), valid sent
commands and received responses must have C/R bit set to 1.
For a station that is a responder, valid sent commands and received
responses must have C/R bit set to 0.

Change the value of C/R bit in command and response frames to
depend on whether the station is initator or not.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index cc3b374..a0fb92c 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -622,7 +622,7 @@ static void gsm_send(struct gsm_mux *gsm, int addr, int cr, int control)
 
 static inline void gsm_response(struct gsm_mux *gsm, int addr, int control)
 {
-	gsm_send(gsm, addr, 0, control);
+	gsm_send(gsm, addr, gsm->initiator ? 0 : 1, control);
 }
 
 /**
@@ -636,7 +636,7 @@ static inline void gsm_response(struct gsm_mux *gsm, int addr, int control)
 
 static inline void gsm_command(struct gsm_mux *gsm, int addr, int control)
 {
-	gsm_send(gsm, addr, 1, control);
+	gsm_send(gsm, addr, gsm->initiator ? 1 : 0, control);
 }
 
 /* Data transmission */
-- 
2.7.0

[PATCH 3/8] tty: n_gsm: make mux work as a responder station

[Andrej Krpic]

Comment suggests that cr == 1 represents a received command and cr == 0
a received response. Received frames are then filtered:
 - correctly by rejection of SABM and DISC responses, they are
   command only frame types and
 - incorrectly by rejection of UA (a response only frame type) responses.

Mux as a initiator successfully establishes DLC by receiving UA response
frame to a previously sent open channel command (SABM). Incorrect
equation (eqA) makes UA "reject cr == 0 commands" case correct, but
filters out all received SABM and DISC command frames.

Change eqA to eqB to match the intent and fix filtering of UA frames.
This enables reception of SABM and DISC frames and consequently
makes mux  work as a responder station.

received    receiving as          eqA          eqB       3GPP TS 27.010
 CR bit     initiator (ir)    cr=CR^(1-ir)   cr=CR^ir        5.2.1.2
   0              0                 1            0         0 (response)
   1              0         _\      0            1         1 (command)
   0              1          /      0            1         1 (command)
   1              1                 1            0         0 (response)

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index a0fb92c..05b562d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1798,7 +1798,7 @@ static void gsm_queue(struct gsm_mux *gsm)
 
 	gsm_print_packet("<--", address, cr, gsm->control, gsm->buf, gsm->len);
 
-	cr ^= 1 - gsm->initiator;	/* Flip so 1 always means command */
+	cr ^= gsm->initiator;		/* Flip so 1 always means command */
 	dlci = gsm->dlci[address];
 
 	switch (gsm->control) {
@@ -1829,7 +1829,7 @@ static void gsm_queue(struct gsm_mux *gsm)
 		break;
 	case UA:
 	case UA|PF:
-		if (cr == 0 || dlci == NULL)
+		if (cr || dlci == NULL)
 			break;
 		switch (dlci->state) {
 		case DLCI_CLOSING:
-- 
2.7.0

[PATCH 4/8] tty: n_gsm: send DM response when accessing an invalid channel

[Andrej Krpic]

Change C/R bit in a response to a UI/UIH frame sent to
non-existing/closed channel. As DM frame type is only valid as a
response, it should be sent using gsm_response function.

Signed-off-by: Andrej Krpic <ak77@tnode.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 05b562d..8551fa4 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1857,7 +1857,7 @@ static void gsm_queue(struct gsm_mux *gsm)
 			goto invalid;
 #endif
 		if (dlci == NULL || dlci->state != DLCI_OPEN) {
-			gsm_command(gsm, address, DM|PF);
+			gsm_response(gsm, address, DM|PF);
 			return;
 		}
 		dlci->data(dlci, gsm->buf, gsm->len);
-- 
2.7.0

[PATCH 5/8] tty: n_gsm: replace dead code with a meaningful comment

[Andrej Krpic]

UI/UIH frame can be received as a command from other station or
as a response to a command we issued earlier.

Add this knowledge to the source code as a comment and remove
useless #if 0/#endif block.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 8551fa4..3c4c521 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1852,10 +1852,11 @@ static void gsm_queue(struct gsm_mux *gsm)
 	case UI|PF:
 	case UIH:
 	case UIH|PF:
-#if 0
-		if (cr)
-			goto invalid;
-#endif
+		/* Don't reject frames based on cr value as UI/UIH
+		 * frame can be received as a command from other
+		 * station or as a response to a command we issued
+		 * earlier.
+		 */
 		if (dlci == NULL || dlci->state != DLCI_OPEN) {
 			gsm_response(gsm, address, DM|PF);
 			return;
-- 
2.7.0

[PATCH 6/8] tty: n_gsm: add missing length field in control channel commands

[Andrej Krpic]

Observing debug output while running initator and responder using n_gsm
shows all control channel commands sent by initiator are malformed
 - they don't include length field (3GPP TS 07.10 ver 7.2.0, 5.4.6.1).

Add length field to transmitted control channel commands in the
gsm_control_transmit) as it is done in gsm_control_reply and expected in
gsm_dlci_command.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 3c4c521..8aa90e0 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1320,12 +1320,13 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 
 static void gsm_control_transmit(struct gsm_mux *gsm, struct gsm_control *ctrl)
 {
-	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 1, gsm->ftype);
+	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 2, gsm->ftype);
 
 	if (msg == NULL)
 		return;
-	msg->data[0] = (ctrl->cmd << 1) | 2 | EA;	/* command */
-	memcpy(msg->data + 1, ctrl->data, ctrl->len);
+	msg->data[0] = (ctrl->cmd << 1) | 2 | EA;   /* command */
+	msg->data[1] = ((ctrl->len) << 1) | EA;
+	memcpy(msg->data + 2, ctrl->data, ctrl->len);
 	gsm_data_queue(gsm->dlci[0], msg);
 }
 
-- 
2.7.0

[PATCH 7/8] tty: n_gsm: properly format Modem Status Command message

[Andrej Krpic]

Change format of Modem Status Command (MSC) message that is
sent to the one expected in the receive function gsm_control_modem
and specified in 3GPP TS 27.010 version 12.0.0 Release 12, 5.4.6.3.7.

Wrongly formatted MSC causes DLC to be marked as constipated. A bug
appears after format of transmitted control messages is fixed and
control messages start to be recognized.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 8aa90e0..b0d9edd 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2874,12 +2874,11 @@ static int gsmtty_modem_update(struct gsm_dlci *dlci, u8 brk)
 	if (brk)
 		len++;
 
-	modembits[0] = len << 1 | EA;		/* Data bytes */
-	modembits[1] = dlci->addr << 2 | 3;	/* DLCI, EA, 1 */
-	modembits[2] = gsm_encode_modem(dlci) << 1 | EA;
+	modembits[0] = dlci->addr << 2 | 3;	/* DLCI, EA, 1 */
+	modembits[1] = gsm_encode_modem(dlci) << 1 | EA;
 	if (brk)
-		modembits[3] = brk << 4 | 2 | EA;	/* Valid, EA */
-	ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len + 1);
+		modembits[2] = brk << 4 | 2 | EA;	/* Valid, EA */
+	ctrl = gsm_control_send(dlci->gsm, CMD_MSC, modembits, len);
 	if (ctrl == NULL)
 		return -ENOMEM;
 	return gsm_control_wait(dlci->gsm, ctrl);
-- 
2.7.0

[PATCH 8/8] tty: n_gsm: Enable reception of frames separated with a single SOF marker

[Andrej Krpic]

Frames may be separated with a single SOF (5.2.6.1 of 27.010 mux spec).
While transmission of a single SOF between frames is implemented in
gsm_data_kick, the reception isn't.

As a side effect, it is now possible to receive and ignore a stream of
consecutive SOFs (5.2.5 of 27.010 mux spec).

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index b0d9edd..12b149d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1895,9 +1895,14 @@ static void gsm0_receive(struct gsm_mux *gsm, unsigned char c)
 		}
 		break;
 	case GSM_ADDRESS:	/* Address EA */
-		gsm->fcs = gsm_fcs_add(gsm->fcs, c);
+		/* Ignore (not first) GSM0_SOF as it decodes into
+		 * reserved DLCI 62 (5.6 of the 27.010 mux spec).
+		 */
+		if (c == GSM0_SOF)
+			break;
 		if (gsm_read_ea(&gsm->address, c))
 			gsm->state = GSM_CONTROL;
+		gsm->fcs = gsm_fcs_add(gsm->fcs, c);
 		break;
 	case GSM_CONTROL:	/* Control Byte */
 		gsm->fcs = gsm_fcs_add(gsm->fcs, c);
@@ -1944,13 +1949,10 @@ static void gsm0_receive(struct gsm_mux *gsm, unsigned char c)
 	case GSM_FCS:		/* FCS follows the packet */
 		gsm->received_fcs = c;
 		gsm_queue(gsm);
-		gsm->state = GSM_SSOF;
-		break;
-	case GSM_SSOF:
-		if (c == GSM0_SOF) {
-			gsm->state = GSM_SEARCH;
-			break;
-		}
+		/* Frames can be separated with a single GSM0_SOF.
+		 * Deal with consecutive GSM0_SOFs in GSM_ADDRESS.
+		 */
+		gsm->state = GSM_SEARCH;
 		break;
 	}
 }
-- 
2.7.0

Re: [PATCH 0/8] tty: n_gsm: Make mux work as a responder station

[One Thousand Gnomes]

On Sun, 21 Feb 2016 22:38:29 +0100
Andrej Krpic <ak77@tnode.com> wrote:

> When using n_gsm you have to explicitly set it to work as a initiator
> station. This led me to believe that it can also work as a responder.
> 
> In a use case where I needed responder station mux I came to the
> conclusion that it actually does not work. Second and third patch
> fix dealings with frame C/R bit that then enable mux to work as a
> responder station.
> 
> Next patches in the series then fix bugs that were found after two 
> instances of n_gsm connected over null-modem cable were used.
> 
> First patch fixes formatting errors, such as space before comma, and
> most of the warnings reported by the checkpatch script.

This looks reasonable to me. It was never intended to work as a responder
but it seems clean enough to do so. Have you tested it against some other
modems with these changes applied ?

(I'm always wary of patches to this going in without testing on actual
modems, because the spec is complex and we are not the only ones with
bugs)

Also can you please cc these patches to
	xinhuix.pan@intel.com

Thanks

Alan

Re: [PATCH 0/8] tty: n_gsm: Make mux work as a responder station

[Andrej Krpic]

On 21.02.2016 23:42, One Thousand Gnomes wrote:
> On Sun, 21 Feb 2016 22:38:29 +0100
> Andrej Krpic <ak77@tnode.com> wrote:
>
>> When using n_gsm you have to explicitly set it to work as a 
>> initiator
>> station. This led me to believe that it can also work as a 
>> responder.
snip
> This looks reasonable to me. It was never intended to work as a 
> responder
> but it seems clean enough to do so. Have you tested it against some 
> other
> modems with these changes applied ?

It has been tested against SIM900 (SIMCom) and M66 (Quectel).

> (I'm always wary of patches to this going in without testing on 
> actual
> modems, because the spec is complex and we are not the only ones with
> bugs)

While second and third patch don't change anything for initiator mode 
mux,
others certainly do.

> Also can you please cc these patches to
> 	xinhuix.pan@intel.com

This address got rejected. Yours and LKML's didn't.


-Andrej

[PATCH 0/8] tty: n_gsm: Make mux work as a responder station

[Andrej Krpic]

When using n_gsm you have to explicitly set it to work as a initiator
station. This led me to believe that it can also work as a responder.

In a use case where I needed responder station mux I came to the
conclusion that it actually does not work. Second and third patch
fix dealings with frame C/R bit that then enable mux to work as a
responder station.

Next patches in the series then fix bugs that were found after two 
instances of n_gsm connected over null-modem cable were used.

First patch fixes formatting errors, such as space before comma, and
most of the warnings reported by the checkpatch script.


Andrej Krpic (8):
  tty: n_gsm: fix formatting errors
  tty: n_gsm: fix C/R bit when sending as a responder
  tty: n_gsm: make mux work as a responder station
  tty: n_gsm: send DM response when accessing an invalid channel
  tty: n_gsm: replace dead code with a meaningful comment
  tty: n_gsm: add missing length field in control channel commands
  tty: n_gsm: properly format Modem Status Command message
  tty: n_gsm: Enable reception of frames separated with a single SOF
    marker

 drivers/tty/n_gsm.c | 191 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 123 insertions(+), 68 deletions(-)

-- 
2.7.0

[PATCH 6/8] tty: n_gsm: add missing length field in control channel commands

[Andrej Krpic]

Observing debug output while running initator and responder using n_gsm
shows all control channel commands sent by initiator are malformed
 - they don't include length field (3GPP TS 07.10 ver 7.2.0, 5.4.6.1).

Add length field to transmitted control channel commands in the
gsm_control_transmit) as it is done in gsm_control_reply and expected in
gsm_dlci_command.

Signed-off-by: Andrej Krpic <ak77@tnode.com>
---
 drivers/tty/n_gsm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 3c4c521..8aa90e0 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1320,12 +1320,13 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 
 static void gsm_control_transmit(struct gsm_mux *gsm, struct gsm_control *ctrl)
 {
-	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 1, gsm->ftype);
+	struct gsm_msg *msg = gsm_data_alloc(gsm, 0, ctrl->len + 2, gsm->ftype);
 
 	if (msg == NULL)
 		return;
-	msg->data[0] = (ctrl->cmd << 1) | 2 | EA;	/* command */
-	memcpy(msg->data + 1, ctrl->data, ctrl->len);
+	msg->data[0] = (ctrl->cmd << 1) | 2 | EA;   /* command */
+	msg->data[1] = ((ctrl->len) << 1) | EA;
+	memcpy(msg->data + 2, ctrl->data, ctrl->len);
 	gsm_data_queue(gsm->dlci[0], msg);
 }
 
-- 
2.7.0