tty: n_gsm: fix closing multiplexer mode

tty: n_gsm: fix closing multiplexer mode

[Sascha Hauer]

Properly closing the n_gsm multiplexer mode is currently not possible.
n_gsm used to send the disconnect command frame in the ttys close
path, but in recent kernels it's no longer possible to do so.

To fix this, remove the sending of disconnect frames from the n_gsm
close path. This means that the modem is still in multiplexing mode
though, so this may lead to problems while re-opening the modem later.
Document the possibilities we have to close the connection in the
Documentation/serial/n_gsm.txt document.

One possibility to close the multiplexing mode is to manually send a
disconnect frame before initialising multiplex mode. Since it's not so
nice that userspace has to know the layout of a disconnect frame,
the second patch introduces a disconnect ioctl which can be issued
right before closing the physical port during the first session.
This ioctl is only useful when during the second session it is known
the the first session has been closed properly, so I'm not sure
how useful it is to introduce such an ioctl.

Sascha

----------------------------------------------------------------
Sascha Hauer (2):
      tty: n_gsm: do not send/receive in ldisc close path
      tty: n_gsm: Add GSMIOC_DISCONNECT ioctl to disconnect the multiplexer

 Documentation/serial/n_gsm.txt | 13 ++++++++++
 drivers/tty/n_gsm.c            | 56 ++++++++++++++++++++++++++++--------------
 include/uapi/linux/gsmmux.h    |  1 +
 3 files changed, 51 insertions(+), 19 deletions(-)

[PATCH 1/2] tty: n_gsm: do not send/receive in ldisc close path

[Sascha Hauer]

gsm_cleanup_mux() is called in the line discipline close path which
is called at tty_release() time. At this stage the tty is no longer
operational enough to send any frames. Sending close frames is
therefore not possible and waiting for their answers always times
out.

This patch removes sending close messages and waiting for their answers
from the tty_release path.

This patch makes explicit what previously implicitly had been the case
already: We are not able to tell the modem that we are about to close
the multiplexer on our side. This means the modem will stay in
multiplexer mode and re-establishing the multiplexer later fails. The
only way for userspace to work around this is to manually send a close
frame in N_TTY mode after closing the mux.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/serial/n_gsm.txt |  7 ++++++
 drivers/tty/n_gsm.c            | 54 +++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/Documentation/serial/n_gsm.txt b/Documentation/serial/n_gsm.txt
index a5d91126a8f7..875361bb7cb4 100644
--- a/Documentation/serial/n_gsm.txt
+++ b/Documentation/serial/n_gsm.txt
@@ -77,6 +77,13 @@ for example, it's possible :
 
 6- first close all virtual ports before closing the physical port.
 
+Note that after closing the physical port the modem is still in multiplexing
+mode. This may prevent a successful re-opening of the port later. To avoid
+this situation either reset the modem if your hardware allows that or send
+a disconnect command frame manually before initializing the multiplexing mode
+for the second time. The byte sequence for the disconnect command frame is:
+0xf9, 0x03, 0xef, 0x03, 0xc3, 0x16, 0xf9.
+
 Additional Documentation
 ------------------------
 More practical details on the protocol and how it's supported by industrial
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 2667a205a5ab..363a8ca824ad 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2015,6 +2015,33 @@ static void gsm_error(struct gsm_mux *gsm,
 	gsm->io_error++;
 }
 
+static int gsm_disconnect(struct gsm_mux *gsm)
+{
+	struct gsm_dlci *dlci = gsm->dlci[0];
+	struct gsm_control *gc;
+
+	if (!dlci)
+		return 0;
+
+	/* In theory disconnecting DLCI 0 is sufficient but for some
+	   modems this is apparently not the case. */
+	gc = gsm_control_send(gsm, CMD_CLD, NULL, 0);
+	if (gc)
+		gsm_control_wait(gsm, gc);
+
+	del_timer_sync(&gsm->t2_timer);
+	/* Now we are sure T2 has stopped */
+
+	gsm_dlci_begin_close(dlci);
+	wait_event_interruptible(gsm->event,
+				dlci->state == DLCI_CLOSED);
+
+	if (signal_pending(current))
+		return -EINTR;
+
+	return 0;
+}
+
 /**
  *	gsm_cleanup_mux		-	generic GSM protocol cleanup
  *	@gsm: our mux
@@ -2029,7 +2056,6 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 	int i;
 	struct gsm_dlci *dlci = gsm->dlci[0];
 	struct gsm_msg *txq, *ntxq;
-	struct gsm_control *gc;
 
 	gsm->dead = 1;
 
@@ -2045,21 +2071,11 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm)
 	if (i == MAX_MUX)
 		return;
 
-	/* In theory disconnecting DLCI 0 is sufficient but for some
-	   modems this is apparently not the case. */
-	if (dlci) {
-		gc = gsm_control_send(gsm, CMD_CLD, NULL, 0);
-		if (gc)
-			gsm_control_wait(gsm, gc);
-	}
 	del_timer_sync(&gsm->t2_timer);
 	/* Now we are sure T2 has stopped */
-	if (dlci) {
+	if (dlci)
 		dlci->dead = 1;
-		gsm_dlci_begin_close(dlci);
-		wait_event_interruptible(gsm->event,
-					dlci->state == DLCI_CLOSED);
-	}
+
 	/* Free up any link layer users */
 	mutex_lock(&gsm->mutex);
 	for (i = 0; i < NUM_DLCI; i++)
@@ -2519,12 +2535,12 @@ static int gsmld_config(struct tty_struct *tty, struct gsm_mux *gsm,
 	 */
 
 	if (need_close || need_restart) {
-		gsm_dlci_begin_close(gsm->dlci[0]);
-		/* This will timeout if the link is down due to N2 expiring */
-		wait_event_interruptible(gsm->event,
-				gsm->dlci[0]->state == DLCI_CLOSED);
-		if (signal_pending(current))
-			return -EINTR;
+		int ret;
+
+		ret = gsm_disconnect(gsm);
+
+		if (ret)
+			return ret;
 	}
 	if (need_restart)
 		gsm_cleanup_mux(gsm);
-- 
2.11.0

[PATCH 2/2] tty: n_gsm: Add GSMIOC_DISCONNECT ioctl to disconnect the multiplexer

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 Documentation/serial/n_gsm.txt | 14 ++++++++++----
 drivers/tty/n_gsm.c            |  2 ++
 include/uapi/linux/gsmmux.h    |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/serial/n_gsm.txt b/Documentation/serial/n_gsm.txt
index 875361bb7cb4..f81bb5110557 100644
--- a/Documentation/serial/n_gsm.txt
+++ b/Documentation/serial/n_gsm.txt
@@ -79,10 +79,16 @@ for example, it's possible :
 
 Note that after closing the physical port the modem is still in multiplexing
 mode. This may prevent a successful re-opening of the port later. To avoid
-this situation either reset the modem if your hardware allows that or send
-a disconnect command frame manually before initializing the multiplexing mode
-for the second time. The byte sequence for the disconnect command frame is:
-0xf9, 0x03, 0xef, 0x03, 0xc3, 0x16, 0xf9.
+this situation you can
+a) reset the modem if your hardware allows that
+b) send a disconnect command frame manually before initializing the
+   multiplexing mode for the second time
+or
+c) close the connection with the GSMIOC_DISCONNECT ioctl before closing the
+   physical port.
+
+The byte sequence for the disconnect command frame is: 0xf9, 0x03, 0xef, 0x03,
+0xc3, 0x16, 0xf9.
 
 Additional Documentation
 ------------------------
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 363a8ca824ad..31b2cc3c18ea 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2602,6 +2602,8 @@ static int gsmld_ioctl(struct tty_struct *tty, struct file *file,
 		if (copy_from_user(&c, (void *)arg, sizeof(c)))
 			return -EFAULT;
 		return gsmld_config(tty, gsm, &c);
+	case GSMIOC_DISCONNECT:
+		return gsm_disconnect(gsm);
 	default:
 		return n_tty_ioctl_helper(tty, file, cmd, arg);
 	}
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index ab055d8cddef..deb98a2ec626 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -24,6 +24,7 @@ struct gsm_config
 
 #define GSMIOC_GETCONF		_IOR('G', 0, struct gsm_config)
 #define GSMIOC_SETCONF		_IOW('G', 1, struct gsm_config)
+#define GSMIOC_DISCONNECT	_IO('G', 2)
 
 struct gsm_netconfig {
 	unsigned int adaption;  /* Adaption to use in network mode */
-- 
2.11.0

Re: [PATCH 2/2] tty: n_gsm: Add GSMIOC_DISCONNECT ioctl to disconnect the multiplexer

[Greg Kroah-Hartman]

On Mon, May 22, 2017 at 10:15:00AM +0200, Sascha Hauer wrote:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

I do not accept patches without any changelog text at all, sorry.

greg k-h

Re: [PATCH 2/2] tty: n_gsm: Add GSMIOC_DISCONNECT ioctl to disconnect the multiplexer

[Sascha Hauer]

On Mon, May 22, 2017 at 11:25:57AM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 22, 2017 at 10:15:00AM +0200, Sascha Hauer wrote:
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> I do not accept patches without any changelog text at all, sorry.

Fair enough. ATM it's not clear if we want to have this patch at all, so
I would resend it with a proper changelog once that cleared.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: tty: n_gsm: fix closing multiplexer mode

[Alan Cox]

> disconnect frame before initialising multiplex mode. Since it's not so
> nice that userspace has to know the layout of a disconnect frame,
> the second patch introduces a disconnect ioctl which can be issued
> right before closing the physical port during the first session.
> This ioctl is only useful when during the second session it is known
> the the first session has been closed properly, so I'm not sure
> how useful it is to introduce such an ioctl.

I don't think it justifies an ioctl and the likely use case is that the
tty is hung up when the process dies so would already be in N_TTY at the
point anyone tried to clean up.

A more interesting question - and I don't have enough hardware to test
this any more - would be whether we can safely send that sequence if our
initial connects failed a few times so we try disconnect/reconnect.

Alan

Re: tty: n_gsm: fix closing multiplexer mode

[Sascha Hauer]

On Mon, May 22, 2017 at 07:16:17PM +0100, Alan Cox wrote:
> > disconnect frame before initialising multiplex mode. Since it's not so
> > nice that userspace has to know the layout of a disconnect frame,
> > the second patch introduces a disconnect ioctl which can be issued
> > right before closing the physical port during the first session.
> > This ioctl is only useful when during the second session it is known
> > the the first session has been closed properly, so I'm not sure
> > how useful it is to introduce such an ioctl.
> 
> I don't think it justifies an ioctl and the likely use case is that the
> tty is hung up when the process dies so would already be in N_TTY at the
> point anyone tried to clean up.

Ok, so let's drop the second patch.

> 
> A more interesting question - and I don't have enough hardware to test
> this any more - would be whether we can safely send that sequence if our
> initial connects failed a few times so we try disconnect/reconnect.

In my current userspace test program I now unconditionally send the
sequence right before sending the AT+CMUX command. This works fine, no
matter if the modem was still in CMUX mode or not. I can only speak for
a UBLOX modem though, other modems might behave differently here. For
example my UBLOX needs the CLD message to close down the multiplexer,
just closing DLC0 does not work.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |