[PATCH 1/3] tty: n_gsm: fix broken virtual tty handling

* [PATCH 1/3] tty: n_gsm: fix broken virtual tty handling
@ 2022-04-22  7:10 D. Starke
  2022-04-22  7:10 ` [PATCH 2/3] tty: n_gsm: fix invalid use of MSC in advanced option D. Starke
  2022-04-22  7:10 ` [PATCH 3/3] tty: n_gsm: fix software flow control handling D. Starke
  0 siblings, 2 replies; 4+ messages in thread
From: D. Starke @ 2022-04-22  7:10 UTC (permalink / raw)
  To: linux-serial, gregkh, jirislaby; +Cc: linux-kernel, Daniel Starke

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

Dynamic virtual tty registration was introduced to allow the user to handle
these cases with uevent rules. The following commits relate to this:
Commit 5b87686e3203 ("tty: n_gsm: Modify gsmtty driver register method when config requester")
Commit 0b91b5332368 ("tty: n_gsm: Save dlci address open status when config requester")
Commit 46292622ad73 ("tty: n_gsm: clean up indenting in gsm_queue()")

However, the following behavior can be seen with this implementation:
- n_gsm ldisc is activated via ioctl
- all configuration parameters are set to their default value (initiator=0)
- the mux gets activated and attached and gsmtty0 is being registered in
  in gsm_dlci_open() after DLCI 0 was established (DLCI 0 is the control
  channel)
- the user configures n_gsm via ioctl GSMIOC_SETCONF as initiator
- this re-attaches the n_gsm mux
- no new gsmtty devices are registered in gsmld_attach_gsm() because the
  mux is already active
- the initiator side registered only the control channel as gsmtty0
  (which should never happen) and no user channel tty

The commits above make it impossible to operate the initiator side as no
user channel tty is or will be available.
On the other hand, this behavior will make it also impossible to allow DLCI
parameter negotiation on responder side in the future. The responder side
first needs to provide a device for the application before the application
can set its parameters of the associated DLCI via ioctl.
Note that the user application is still able to detect a link establishment
without relaying to uevent by waiting for DTR open on responder side. This
is the same behavior as on a physical serial interface. And on initiator
side a tty hangup can be detected if a link establishment request failed.

Revert the commits above completely to always register all user channels
and no control channel after mux attachment. No other changes are made.

Fixes: 5b87686e3203 ("tty: n_gsm: Modify gsmtty driver register method when config requester")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 drivers/tty/n_gsm.c | 87 ++++++++-------------------------------------
 1 file changed, 15 insertions(+), 72 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 979dc9151383..99fe54247a87 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -272,10 +272,6 @@ static DEFINE_SPINLOCK(gsm_mux_lock);
 
 static struct tty_driver *gsm_tty_driver;
 
-/* Save dlci open address */
-static int addr_open[256] = { 0 };
-/* Save dlci open count */
-static int addr_cnt;
 /*
  *	This section of the driver logic implements the GSM encodings
  *	both the basic and the 'advanced'. Reliable transport is not
@@ -1185,7 +1181,6 @@ static void gsm_control_rls(struct gsm_mux *gsm, const u8 *data, int clen)
 }
 
 static void gsm_dlci_begin_close(struct gsm_dlci *dlci);
-static void gsm_dlci_close(struct gsm_dlci *dlci);
 
 /**
  *	gsm_control_message	-	DLCI 0 control processing
@@ -1204,28 +1199,15 @@ static void gsm_control_message(struct gsm_mux *gsm, unsigned int command,
 {
 	u8 buf[1];
 	unsigned long flags;
-	struct gsm_dlci *dlci;
-	int i;
-	int address;
 
 	switch (command) {
 	case CMD_CLD: {
-		if (addr_cnt > 0) {
-			for (i = 0; i < addr_cnt; i++) {
-				address = addr_open[i];
-				dlci = gsm->dlci[address];
-				gsm_dlci_close(dlci);
-				addr_open[i] = 0;
-			}
-		}
+		struct gsm_dlci *dlci = gsm->dlci[0];
 		/* Modem wishes to close down */
-		dlci = gsm->dlci[0];
 		if (dlci) {
 			dlci->dead = true;
 			gsm->dead = true;
-			gsm_dlci_close(dlci);
-			addr_cnt = 0;
-			gsm_response(gsm, 0, UA|PF);
+			gsm_dlci_begin_close(dlci);
 		}
 		}
 		break;
@@ -1459,8 +1441,6 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 		wake_up_interruptible(&dlci->port.open_wait);
 	} else
 		dlci->gsm->dead = true;
-	/* Unregister gsmtty driver,report gsmtty dev remove uevent for user */
-	tty_unregister_device(gsm_tty_driver, dlci->addr);
 	wake_up(&dlci->gsm->event);
 	/* A DLCI 0 close is a MUX termination so we need to kick that
 	   back to userspace somehow */
@@ -1482,8 +1462,6 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	dlci->state = DLCI_OPEN;
 	if (debug & 8)
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
-	/* Register gsmtty driver,report gsmtty dev add uevent for user */
-	tty_register_device(gsm_tty_driver, dlci->addr, NULL);
 	/* Send current modem state */
 	if (dlci->addr)
 		gsmtty_modem_update(dlci, 0);
@@ -1794,7 +1772,6 @@ static void gsm_queue(struct gsm_mux *gsm)
 	struct gsm_dlci *dlci;
 	u8 cr;
 	int address;
-	int i, j, k, address_tmp;
 
 	if (gsm->fcs != GOOD_FCS) {
 		gsm->bad_fcs++;
@@ -1826,11 +1803,6 @@ static void gsm_queue(struct gsm_mux *gsm)
 		else {
 			gsm_response(gsm, address, UA|PF);
 			gsm_dlci_open(dlci);
-			/* Save dlci open address */
-			if (address) {
-				addr_open[addr_cnt] = address;
-				addr_cnt++;
-			}
 		}
 		break;
 	case DISC|PF:
@@ -1841,33 +1813,8 @@ static void gsm_queue(struct gsm_mux *gsm)
 			return;
 		}
 		/* Real close complete */
-		if (!address) {
-			if (addr_cnt > 0) {
-				for (i = 0; i < addr_cnt; i++) {
-					address = addr_open[i];
-					dlci = gsm->dlci[address];
-					gsm_dlci_close(dlci);
-					addr_open[i] = 0;
-				}
-			}
-			dlci = gsm->dlci[0];
-			gsm_dlci_close(dlci);
-			addr_cnt = 0;
-			gsm_response(gsm, 0, UA|PF);
-		} else {
-			gsm_response(gsm, address, UA|PF);
-			gsm_dlci_close(dlci);
-			/* clear dlci address */
-			for (j = 0; j < addr_cnt; j++) {
-				address_tmp = addr_open[j];
-				if (address_tmp == address) {
-					for (k = j; k < addr_cnt; k++)
-						addr_open[k] = addr_open[k+1];
-					addr_cnt--;
-					break;
-				}
-			}
-		}
+		gsm_response(gsm, address, UA|PF);
+		gsm_dlci_close(dlci);
 		break;
 	case UA|PF:
 		if (cr == 0 || dlci == NULL)
@@ -2451,19 +2398,17 @@ static int gsmld_attach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 	else {
 		/* Don't register device 0 - this is the control channel and not
 		   a usable tty interface */
-		if (gsm->initiator) {
-			base = mux_num_to_base(gsm); /* Base for this MUX */
-			for (i = 1; i < NUM_DLCI; i++) {
-				struct device *dev;
+		base = mux_num_to_base(gsm); /* Base for this MUX */
+		for (i = 1; i < NUM_DLCI; i++) {
+			struct device *dev;
 
-				dev = tty_register_device(gsm_tty_driver,
+			dev = tty_register_device(gsm_tty_driver,
 							base + i, NULL);
-				if (IS_ERR(dev)) {
-					for (i--; i >= 1; i--)
-						tty_unregister_device(gsm_tty_driver,
-									base + i);
-					return PTR_ERR(dev);
-				}
+			if (IS_ERR(dev)) {
+				for (i--; i >= 1; i--)
+					tty_unregister_device(gsm_tty_driver,
+								base + i);
+				return PTR_ERR(dev);
 			}
 		}
 	}
@@ -2485,10 +2430,8 @@ static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
 	int i;
 
 	WARN_ON(tty != gsm->tty);
-	if (gsm->initiator) {
-		for (i = 1; i < NUM_DLCI; i++)
-			tty_unregister_device(gsm_tty_driver, base + i);
-	}
+	for (i = 1; i < NUM_DLCI; i++)
+		tty_unregister_device(gsm_tty_driver, base + i);
 	tty_kref_put(gsm->tty);
 	gsm->tty = NULL;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread