[PATCH 1/3] tty: n_gsm: add keep alive support

[PATCH 1/3] tty: n_gsm: add keep alive 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. Chapters 5.4.6.3.4 and 5.1.8.1.3 describe the test
command which can be used to test the mux connection between both sides.

Currently, no algorithm is implemented to make use of this command. This
requires that each multiplexed upper layer protocol supervises the
underlying muxer connection to handle possible connection losses.

Introduce an ioctl parameter and functions to optionally enable keep alive
handling via the test command as described in chapter 5.4.6.3.4. A single
incrementing octet is being used to distinguish between multiple retries.
Retry count and interval are taken from the general parameters N2 and T2.

Note that support for the test command is mandatory and already present in
the muxer implementation since the very first version.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5783801d6524..98577b54f1fd 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -318,13 +318,19 @@ struct gsm_mux {
 	struct gsm_control *pending_cmd;/* Our current pending command */
 	spinlock_t control_lock;	/* Protects the pending command */
 
+	/* Keep-alive */
+	struct timer_list ka_timer;	/* Keep-alive response timer */
+	u8 ka_num;			/* Keep-alive match pattern */
+	int ka_retries;			/* Keep-alive retry counter */
+
 	/* Configuration */
-	int adaption;		/* 1 or 2 supported */
-	u8 ftype;		/* UI or UIH */
-	int t1, t2;		/* Timers in 1/100th of a sec */
-	unsigned int t3;	/* Power wake-up timer in seconds. */
-	int n2;			/* Retry count */
-	u8 k;			/* Window size */
+	int adaption;			/* 1 or 2 supported */
+	u8 ftype;			/* UI or UIH */
+	int t1, t2;			/* Timers in 1/100th of a sec */
+	unsigned int t3;		/* Power wake-up timer in seconds. */
+	int n2;				/* Retry count */
+	u8 k;				/* Window size */
+	unsigned int keep_alive;	/* Control channel keep-alive in ms */
 
 	/* Statistics (not currently exposed) */
 	unsigned long bad_fcs;
@@ -1897,11 +1903,13 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 						const u8 *data, int clen)
 {
 	struct gsm_control *ctrl;
+	struct gsm_dlci *dlci;
 	unsigned long flags;
 
 	spin_lock_irqsave(&gsm->control_lock, flags);
 
 	ctrl = gsm->pending_cmd;
+	dlci = gsm->dlci[0];
 	command |= 1;
 	/* Does the reply match our command */
 	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
@@ -1916,10 +1924,57 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
 	/* Or did we receive the PN response to our PN command */
 	} else if (command == CMD_PN) {
 		gsm_control_negotiation(gsm, 0, data, clen);
+	/* Or did we receive the TEST response to our TEST command */
+	} else if (command == CMD_TEST && clen == 1 && *data == gsm->ka_num) {
+		gsm->ka_retries = -1; /* trigger new keep-alive message */
+		if (dlci && !dlci->dead)
+			mod_timer(&gsm->ka_timer,
+				  jiffies + gsm->keep_alive * HZ / 100);
 	}
 	spin_unlock_irqrestore(&gsm->control_lock, flags);
 }
 
+/**
+ * gsm_control_keep_alive	-	check timeout or start keep-alive
+ * @t: timer contained in our gsm object
+ *
+ * Called off the keep-alive timer expiry signaling that our link
+ * partner is not responding anymore. Link will be closed.
+ * This is also called to startup our timer.
+ */
+
+static void gsm_control_keep_alive(struct timer_list *t)
+{
+	struct gsm_mux *gsm = from_timer(gsm, t, ka_timer);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gsm->control_lock, flags);
+	if (gsm->ka_num && gsm->ka_retries == 0) {
+		/* Keep-alive expired -> close the link */
+		if (debug & DBG_ERRORS)
+			pr_info("%s keep-alive timed out\n", __func__);
+		spin_unlock_irqrestore(&gsm->control_lock, flags);
+		if (gsm->dlci[0])
+			gsm_dlci_begin_close(gsm->dlci[0]);
+	} else if (gsm->keep_alive && gsm->dlci[0] && !gsm->dlci[0]->dead) {
+		if (gsm->ka_retries > 0) {
+			/* T2 expired for keep-alive -> resend */
+			gsm->ka_retries--;
+		} else {
+			/* Start keep-alive timer */
+			gsm->ka_num++;
+			if (!gsm->ka_num)
+				gsm->ka_num++;
+			gsm->ka_retries = gsm->n2;
+		}
+		gsm_control_command(gsm, CMD_TEST, &gsm->ka_num,
+				    sizeof(gsm->ka_num));
+		mod_timer(&gsm->ka_timer,
+			  jiffies + gsm->t2 * HZ / 100);
+		spin_unlock_irqrestore(&gsm->control_lock, flags);
+	}
+}
+
 /**
  *	gsm_control_transmit	-	send control packet
  *	@gsm: gsm mux
@@ -2061,8 +2116,10 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 		/* Ensure that gsmtty_open() can return. */
 		tty_port_set_initialized(&dlci->port, false);
 		wake_up_interruptible(&dlci->port.open_wait);
-	} else
+	} else {
+		del_timer(&dlci->gsm->ka_timer);
 		dlci->gsm->dead = true;
+	}
 	/* A DLCI 0 close is a MUX termination so we need to kick that
 	   back to userspace somehow */
 	gsm_dlci_data_kick(dlci);
@@ -2078,6 +2135,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
 
 static void gsm_dlci_open(struct gsm_dlci *dlci)
 {
+	struct gsm_mux *gsm = dlci->gsm;
+
 	/* Note that SABM UA .. SABM UA first UA lost can mean that we go
 	   open -> open */
 	del_timer(&dlci->t1);
@@ -2087,8 +2146,15 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
 	if (debug & DBG_ERRORS)
 		pr_debug("DLCI %d goes open.\n", dlci->addr);
 	/* Send current modem state */
-	if (dlci->addr)
+	if (dlci->addr) {
 		gsm_modem_update(dlci, 0);
+	} else {
+		/* Start keep-alive control */
+		gsm->ka_num = 0;
+		gsm->ka_retries = -1;
+		mod_timer(&gsm->ka_timer,
+			  jiffies + gsm->keep_alive * HZ / 100);
+	}
 	gsm_dlci_data_kick(dlci);
 	wake_up(&dlci->gsm->event);
 }
@@ -2840,6 +2906,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
 	/* Finish outstanding timers, making sure they are done */
 	del_timer_sync(&gsm->kick_timer);
 	del_timer_sync(&gsm->t2_timer);
+	del_timer_sync(&gsm->ka_timer);
 
 	/* Finish writing to ldisc */
 	flush_work(&gsm->tx_work);
@@ -2987,6 +3054,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	INIT_LIST_HEAD(&gsm->tx_data_list);
 	timer_setup(&gsm->kick_timer, gsm_kick_timer, 0);
 	timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
+	timer_setup(&gsm->ka_timer, gsm_control_keep_alive, 0);
 	INIT_WORK(&gsm->tx_work, gsmld_write_task);
 	init_waitqueue_head(&gsm->event);
 	spin_lock_init(&gsm->control_lock);
@@ -3003,6 +3071,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
 	gsm->mru = 64;	/* Default to encoding 1 so these should be 64 */
 	gsm->mtu = 64;
 	gsm->dead = true;	/* Avoid early tty opens */
+	gsm->keep_alive = 0;	/* Disabled */
 
 	/* Store the instance to the mux array or abort if no space is
 	 * available.
@@ -3046,6 +3115,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
 	c->mru = gsm->mru;
 	c->mtu = gsm->mtu;
 	c->k = gsm->k;
+	c->keep_alive = gsm->keep_alive;
 }
 
 static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
@@ -3094,6 +3164,8 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 		need_restart = 1;
 	if (c->mtu != gsm->mtu)
 		need_restart = 1;
+	if (c->keep_alive != gsm->keep_alive)
+		need_restart = true;
 
 	/*
 	 * Close down what is needed, restart and initiate the new
@@ -3109,6 +3181,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	gsm->encoding = c->encapsulation ? GSM_ADV_OPT : GSM_BASIC_OPT;
 	gsm->adaption = c->adaption;
 	gsm->n2 = c->n2;
+	gsm->keep_alive = c->keep_alive;
 
 	if (c->i == 1)
 		gsm->ftype = UIH;
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index cb8693b39cb7..b64360aca1f9 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -19,7 +19,8 @@ struct gsm_config
 	unsigned int mtu;
 	unsigned int k;
 	unsigned int i;
-	unsigned int unused[8];		/* Padding for expansion without
+	unsigned int keep_alive;
+	unsigned int unused[7];		/* Padding for expansion without
 					   breaking stuff */
 };
 
-- 
2.34.1

[PATCH 2/3] tty: n_gsm: add RING/CD control support

[D. Starke]

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

The status lines ring and carrier detect are used by the modem to signal
incoming calls (RING) or an established connection (CD). This is
implemented as physical lines on a standard RS232 connection. However,
the muxer protocol encodes these status lines as modem bits IC and DV.
These incoming lines are masked by tty driver (see tty_io.c) and cannot be
set by a user application.

Allow setting RING via TIOCM_OUT1 and CD via TIOCM_OUT2 to allow
implementation of a modem or modem emulator.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 98577b54f1fd..118511c1fa37 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -546,6 +546,11 @@ static u8 gsm_encode_modem(const struct gsm_dlci *dlci)
 		modembits |= MDM_IC;
 	if (dlci->modem_tx & TIOCM_CD || dlci->gsm->initiator)
 		modembits |= MDM_DV;
+	/* special mappings for passive side to operate as UE */
+	if (dlci->modem_tx & TIOCM_OUT1)
+		modembits |= MDM_IC;
+	if (dlci->modem_tx & TIOCM_OUT2)
+		modembits |= MDM_DV;
 	return modembits;
 }
 
-- 
2.34.1

[PATCH 3/3] tty: n_gsm: add TIOCMIWAIT support

[D. Starke]

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

Add support for the TIOCMIWAIT ioctl on the virtual ttys. This enables the
user to wait for virtual modem signals like RING.

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

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 118511c1fa37..48fb7dad44cd 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1542,6 +1542,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
 	if (brk & 0x01)
 		tty_insert_flip_char(&dlci->port, 0, TTY_BREAK);
 	dlci->modem_rx = mlines;
+	wake_up_interruptible(&dlci->gsm->event);
 }
 
 /**
@@ -3848,6 +3849,35 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
 	return -EPROTONOSUPPORT;
 }
 
+/**
+ * gsm_wait_modem_change	-	wait for modem status line change
+ * @dlci: channel
+ * @mask: modem status line bits
+ */
+
+static int gsm_wait_modem_change(struct gsm_dlci *dlci, u32 mask)
+{
+	struct gsm_mux *gsm = dlci->gsm;
+	u32 old = dlci->modem_rx & mask;
+	int ret;
+
+	if (gsm->dead)
+		return -ENODEV;
+
+	do {
+		ret = wait_event_interruptible(gsm->event, gsm->dead
+					      || old ^ (dlci->modem_rx & mask));
+		if (ret)
+			return ret;
+		if (dlci->state != DLCI_OPEN)
+			return -EL2NSYNC;
+		if (gsm->dead)
+			return -ENODEV;
+	} while ((old ^ (dlci->modem_rx & mask)) == 0);
+
+	return 0;
+}
+
 static bool gsm_carrier_raised(struct tty_port *port)
 {
 	struct gsm_dlci *dlci = container_of(port, struct gsm_dlci, port);
@@ -4107,6 +4137,8 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 		gsm_destroy_network(dlci);
 		mutex_unlock(&dlci->mutex);
 		return 0;
+	case TIOCMIWAIT:
+		return gsm_wait_modem_change(dlci, arg);
 	default:
 		return -ENOIOCTLCMD;
 	}
-- 
2.34.1

Re: [PATCH 1/3] tty: n_gsm: add keep alive support

[Greg KH]

On Wed, Feb 01, 2023 at 09:01:49AM +0100, 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. Chapters 5.4.6.3.4 and 5.1.8.1.3 describe the test
> command which can be used to test the mux connection between both sides.
> 
> Currently, no algorithm is implemented to make use of this command. This
> requires that each multiplexed upper layer protocol supervises the
> underlying muxer connection to handle possible connection losses.
> 
> Introduce an ioctl parameter and functions to optionally enable keep alive
> handling via the test command as described in chapter 5.4.6.3.4. A single
> incrementing octet is being used to distinguish between multiple retries.
> Retry count and interval are taken from the general parameters N2 and T2.
> 
> Note that support for the test command is mandatory and already present in
> the muxer implementation since the very first version.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c         | 89 +++++++++++++++++++++++++++++++++----
>  include/uapi/linux/gsmmux.h |  3 +-
>  2 files changed, 83 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5783801d6524..98577b54f1fd 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -318,13 +318,19 @@ struct gsm_mux {
>  	struct gsm_control *pending_cmd;/* Our current pending command */
>  	spinlock_t control_lock;	/* Protects the pending command */
>  
> +	/* Keep-alive */
> +	struct timer_list ka_timer;	/* Keep-alive response timer */
> +	u8 ka_num;			/* Keep-alive match pattern */
> +	int ka_retries;			/* Keep-alive retry counter */
> +
>  	/* Configuration */
> -	int adaption;		/* 1 or 2 supported */
> -	u8 ftype;		/* UI or UIH */
> -	int t1, t2;		/* Timers in 1/100th of a sec */
> -	unsigned int t3;	/* Power wake-up timer in seconds. */
> -	int n2;			/* Retry count */
> -	u8 k;			/* Window size */
> +	int adaption;			/* 1 or 2 supported */
> +	u8 ftype;			/* UI or UIH */
> +	int t1, t2;			/* Timers in 1/100th of a sec */
> +	unsigned int t3;		/* Power wake-up timer in seconds. */
> +	int n2;				/* Retry count */
> +	u8 k;				/* Window size */
> +	unsigned int keep_alive;	/* Control channel keep-alive in ms */
>  
>  	/* Statistics (not currently exposed) */
>  	unsigned long bad_fcs;
> @@ -1897,11 +1903,13 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  						const u8 *data, int clen)
>  {
>  	struct gsm_control *ctrl;
> +	struct gsm_dlci *dlci;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&gsm->control_lock, flags);
>  
>  	ctrl = gsm->pending_cmd;
> +	dlci = gsm->dlci[0];
>  	command |= 1;
>  	/* Does the reply match our command */
>  	if (ctrl != NULL && (command == ctrl->cmd || command == CMD_NSC)) {
> @@ -1916,10 +1924,57 @@ static void gsm_control_response(struct gsm_mux *gsm, unsigned int command,
>  	/* Or did we receive the PN response to our PN command */
>  	} else if (command == CMD_PN) {
>  		gsm_control_negotiation(gsm, 0, data, clen);
> +	/* Or did we receive the TEST response to our TEST command */
> +	} else if (command == CMD_TEST && clen == 1 && *data == gsm->ka_num) {
> +		gsm->ka_retries = -1; /* trigger new keep-alive message */
> +		if (dlci && !dlci->dead)
> +			mod_timer(&gsm->ka_timer,
> +				  jiffies + gsm->keep_alive * HZ / 100);
>  	}
>  	spin_unlock_irqrestore(&gsm->control_lock, flags);
>  }
>  
> +/**
> + * gsm_control_keep_alive	-	check timeout or start keep-alive
> + * @t: timer contained in our gsm object
> + *
> + * Called off the keep-alive timer expiry signaling that our link
> + * partner is not responding anymore. Link will be closed.
> + * This is also called to startup our timer.
> + */
> +
> +static void gsm_control_keep_alive(struct timer_list *t)
> +{
> +	struct gsm_mux *gsm = from_timer(gsm, t, ka_timer);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&gsm->control_lock, flags);
> +	if (gsm->ka_num && gsm->ka_retries == 0) {
> +		/* Keep-alive expired -> close the link */
> +		if (debug & DBG_ERRORS)
> +			pr_info("%s keep-alive timed out\n", __func__);
> +		spin_unlock_irqrestore(&gsm->control_lock, flags);
> +		if (gsm->dlci[0])
> +			gsm_dlci_begin_close(gsm->dlci[0]);
> +	} else if (gsm->keep_alive && gsm->dlci[0] && !gsm->dlci[0]->dead) {
> +		if (gsm->ka_retries > 0) {
> +			/* T2 expired for keep-alive -> resend */
> +			gsm->ka_retries--;
> +		} else {
> +			/* Start keep-alive timer */
> +			gsm->ka_num++;
> +			if (!gsm->ka_num)
> +				gsm->ka_num++;
> +			gsm->ka_retries = gsm->n2;
> +		}
> +		gsm_control_command(gsm, CMD_TEST, &gsm->ka_num,
> +				    sizeof(gsm->ka_num));
> +		mod_timer(&gsm->ka_timer,
> +			  jiffies + gsm->t2 * HZ / 100);
> +		spin_unlock_irqrestore(&gsm->control_lock, flags);
> +	}
> +}
> +
>  /**
>   *	gsm_control_transmit	-	send control packet
>   *	@gsm: gsm mux
> @@ -2061,8 +2116,10 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
>  		/* Ensure that gsmtty_open() can return. */
>  		tty_port_set_initialized(&dlci->port, false);
>  		wake_up_interruptible(&dlci->port.open_wait);
> -	} else
> +	} else {
> +		del_timer(&dlci->gsm->ka_timer);
>  		dlci->gsm->dead = true;
> +	}
>  	/* A DLCI 0 close is a MUX termination so we need to kick that
>  	   back to userspace somehow */
>  	gsm_dlci_data_kick(dlci);
> @@ -2078,6 +2135,8 @@ static void gsm_dlci_close(struct gsm_dlci *dlci)
>  
>  static void gsm_dlci_open(struct gsm_dlci *dlci)
>  {
> +	struct gsm_mux *gsm = dlci->gsm;
> +
>  	/* Note that SABM UA .. SABM UA first UA lost can mean that we go
>  	   open -> open */
>  	del_timer(&dlci->t1);
> @@ -2087,8 +2146,15 @@ static void gsm_dlci_open(struct gsm_dlci *dlci)
>  	if (debug & DBG_ERRORS)
>  		pr_debug("DLCI %d goes open.\n", dlci->addr);
>  	/* Send current modem state */
> -	if (dlci->addr)
> +	if (dlci->addr) {
>  		gsm_modem_update(dlci, 0);
> +	} else {
> +		/* Start keep-alive control */
> +		gsm->ka_num = 0;
> +		gsm->ka_retries = -1;
> +		mod_timer(&gsm->ka_timer,
> +			  jiffies + gsm->keep_alive * HZ / 100);
> +	}
>  	gsm_dlci_data_kick(dlci);
>  	wake_up(&dlci->gsm->event);
>  }
> @@ -2840,6 +2906,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
>  	/* Finish outstanding timers, making sure they are done */
>  	del_timer_sync(&gsm->kick_timer);
>  	del_timer_sync(&gsm->t2_timer);
> +	del_timer_sync(&gsm->ka_timer);
>  
>  	/* Finish writing to ldisc */
>  	flush_work(&gsm->tx_work);
> @@ -2987,6 +3054,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
>  	INIT_LIST_HEAD(&gsm->tx_data_list);
>  	timer_setup(&gsm->kick_timer, gsm_kick_timer, 0);
>  	timer_setup(&gsm->t2_timer, gsm_control_retransmit, 0);
> +	timer_setup(&gsm->ka_timer, gsm_control_keep_alive, 0);
>  	INIT_WORK(&gsm->tx_work, gsmld_write_task);
>  	init_waitqueue_head(&gsm->event);
>  	spin_lock_init(&gsm->control_lock);
> @@ -3003,6 +3071,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
>  	gsm->mru = 64;	/* Default to encoding 1 so these should be 64 */
>  	gsm->mtu = 64;
>  	gsm->dead = true;	/* Avoid early tty opens */
> +	gsm->keep_alive = 0;	/* Disabled */
>  
>  	/* Store the instance to the mux array or abort if no space is
>  	 * available.
> @@ -3046,6 +3115,7 @@ static void gsm_copy_config_values(struct gsm_mux *gsm,
>  	c->mru = gsm->mru;
>  	c->mtu = gsm->mtu;
>  	c->k = gsm->k;
> +	c->keep_alive = gsm->keep_alive;
>  }
>  
>  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
> @@ -3094,6 +3164,8 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  		need_restart = 1;
>  	if (c->mtu != gsm->mtu)
>  		need_restart = 1;
> +	if (c->keep_alive != gsm->keep_alive)
> +		need_restart = true;
>  
>  	/*
>  	 * Close down what is needed, restart and initiate the new
> @@ -3109,6 +3181,7 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
>  	gsm->encoding = c->encapsulation ? GSM_ADV_OPT : GSM_BASIC_OPT;
>  	gsm->adaption = c->adaption;
>  	gsm->n2 = c->n2;
> +	gsm->keep_alive = c->keep_alive;
>  
>  	if (c->i == 1)
>  		gsm->ftype = UIH;
> diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
> index cb8693b39cb7..b64360aca1f9 100644
> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -19,7 +19,8 @@ struct gsm_config
>  	unsigned int mtu;
>  	unsigned int k;
>  	unsigned int i;
> -	unsigned int unused[8];		/* Padding for expansion without
> +	unsigned int keep_alive;
> +	unsigned int unused[7];		/* Padding for expansion without

"unsigned int" is not really a valid uapi variable type.

Shouldn't this be __u32 instead?

Should you document this field as to what the value is and the units as
you are creating a new user/kernel api here.

And finally, "unused" here is being properly checked to be all 0, right?
If not, then this change can't happen for obvious reasons :(

thanks,

greg k-h

Re: [PATCH 3/3] tty: n_gsm: add TIOCMIWAIT support

[Greg KH]

On Wed, Feb 01, 2023 at 09:01:51AM +0100, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> Add support for the TIOCMIWAIT ioctl on the virtual ttys. This enables the
> user to wait for virtual modem signals like RING.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> ---
>  drivers/tty/n_gsm.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 118511c1fa37..48fb7dad44cd 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1542,6 +1542,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
>  	if (brk & 0x01)
>  		tty_insert_flip_char(&dlci->port, 0, TTY_BREAK);
>  	dlci->modem_rx = mlines;
> +	wake_up_interruptible(&dlci->gsm->event);
>  }
>  
>  /**
> @@ -3848,6 +3849,35 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
>  	return -EPROTONOSUPPORT;
>  }
>  
> +/**
> + * gsm_wait_modem_change	-	wait for modem status line change

No need for tabs.

Or for kernel doc for static functions, but that's not a big deal.

> + * @dlci: channel
> + * @mask: modem status line bits
> + */
> +

No blank line please, didn't checkpatch warn about that?

> +static int gsm_wait_modem_change(struct gsm_dlci *dlci, u32 mask)
> +{
> +	struct gsm_mux *gsm = dlci->gsm;
> +	u32 old = dlci->modem_rx & mask;
> +	int ret;
> +
> +	if (gsm->dead)
> +		return -ENODEV;
> +
> +	do {
> +		ret = wait_event_interruptible(gsm->event, gsm->dead
> +					      || old ^ (dlci->modem_rx & mask));
> +		if (ret)
> +			return ret;
> +		if (dlci->state != DLCI_OPEN)
> +			return -EL2NSYNC;
> +		if (gsm->dead)
> +			return -ENODEV;
> +	} while ((old ^ (dlci->modem_rx & mask)) == 0);

No way to break out of the loop if it goes for forever?

thanks,

greg k-h

RE: [PATCH 1/3] tty: n_gsm: add keep alive support

[Starke, Daniel]

> > index cb8693b39cb7..b64360aca1f9 100644
> > --- a/include/uapi/linux/gsmmux.h
> > +++ b/include/uapi/linux/gsmmux.h
> > @@ -19,7 +19,8 @@ struct gsm_config
> >  	unsigned int mtu;
> >  	unsigned int k;
> >  	unsigned int i;
> > -	unsigned int unused[8];		/* Padding for expansion without
> > +	unsigned int keep_alive;
> > +	unsigned int unused[7];		/* Padding for expansion without
> 
> "unsigned int" is not really a valid uapi variable type.
> 
> Shouldn't this be __u32 instead?

I know but changing it to a fixed size data type may break compatibility
as this may change the overall size of the structure. This is why I
took a field out of the "unused" array for the "keep_alive" parameter.
A value of zero disables keep-alive polling.

> Should you document this field as to what the value is and the units as
> you are creating a new user/kernel api here.

I will add a comment here. Comments for the other fields remain subject to
another patch.

> And finally, "unused" here is being properly checked to be all 0, right?
> If not, then this change can't happen for obvious reasons :(

This was not the case until now. I assumed there was some coding guideline
that unused fields need to be initialized to zero. Obviously, checking it
prevents misuse here. I will add relevant checks for this.

Best regards,
Daniel Starke

Re: [PATCH 1/3] tty: n_gsm: add keep alive support

[Greg KH]

On Wed, Feb 01, 2023 at 09:17:31AM +0000, Starke, Daniel wrote:
> > > index cb8693b39cb7..b64360aca1f9 100644
> > > --- a/include/uapi/linux/gsmmux.h
> > > +++ b/include/uapi/linux/gsmmux.h
> > > @@ -19,7 +19,8 @@ struct gsm_config
> > >  	unsigned int mtu;
> > >  	unsigned int k;
> > >  	unsigned int i;
> > > -	unsigned int unused[8];		/* Padding for expansion without
> > > +	unsigned int keep_alive;
> > > +	unsigned int unused[7];		/* Padding for expansion without
> > 
> > "unsigned int" is not really a valid uapi variable type.
> > 
> > Shouldn't this be __u32 instead?
> 
> I know but changing it to a fixed size data type may break compatibility
> as this may change the overall size of the structure.

Will it?  It shouldn't that's why using the correct data types is
essencial.

> This is why I
> took a field out of the "unused" array for the "keep_alive" parameter.
> A value of zero disables keep-alive polling.
> 
> > Should you document this field as to what the value is and the units as
> > you are creating a new user/kernel api here.
> 
> I will add a comment here. Comments for the other fields remain subject to
> another patch.
> 
> > And finally, "unused" here is being properly checked to be all 0, right?
> > If not, then this change can't happen for obvious reasons :(
> 
> This was not the case until now. I assumed there was some coding guideline
> that unused fields need to be initialized to zero. Obviously, checking it
> prevents misuse here. I will add relevant checks for this.

If the value was not checked previously, then you can not use the field
now, otherwise things will break, sorry.  Those are useless fields and
should be marked as such :(

sorry,

greg k-h

RE: [PATCH 3/3] tty: n_gsm: add TIOCMIWAIT support

[Starke, Daniel]

> > Add support for the TIOCMIWAIT ioctl on the virtual ttys. This enables the
> > user to wait for virtual modem signals like RING.
> > 
> > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > ---
> >  drivers/tty/n_gsm.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 118511c1fa37..48fb7dad44cd 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -1542,6 +1542,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
> >  	if (brk & 0x01)
> >  		tty_insert_flip_char(&dlci->port, 0, TTY_BREAK);
> >  	dlci->modem_rx = mlines;
> > +	wake_up_interruptible(&dlci->gsm->event);
> >  }
> >  
> >  /**
> > @@ -3848,6 +3849,35 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
> >  	return -EPROTONOSUPPORT;
> >  }
> >  
> > +/**
> > + * gsm_wait_modem_change	-	wait for modem status line change
> 
> No need for tabs.
> 
> Or for kernel doc for static functions, but that's not a big deal.

All other functions follow the same style here. Nevertheless, I will
replace the tabs with spaces. Originally, I had planned a code clean-up
after committing all the changes from my original RFC.

> > + * @dlci: channel
> > + * @mask: modem status line bits
> > + */
> > +
> 
> No blank line please, didn't checkpatch warn about that?

No, checkpatch did not warn about this and all other functions follow the
same style. I will remove the blank line.

> > +static int gsm_wait_modem_change(struct gsm_dlci *dlci, u32 mask)
> > +{
> > +	struct gsm_mux *gsm = dlci->gsm;
> > +	u32 old = dlci->modem_rx & mask;
> > +	int ret;
> > +
> > +	if (gsm->dead)
> > +		return -ENODEV;
> > +
> > +	do {
> > +		ret = wait_event_interruptible(gsm->event, gsm->dead
> > +					      || old ^ (dlci->modem_rx & mask));
> > +		if (ret)
> > +			return ret;
> > +		if (dlci->state != DLCI_OPEN)
> > +			return -EL2NSYNC;
> > +		if (gsm->dead)
> > +			return -ENODEV;
> > +	} while ((old ^ (dlci->modem_rx & mask)) == 0);
> 
> No way to break out of the loop if it goes for forever?

I assume that this is the expected behavior for TIOCMIWAIT. The functions
returns if:
- the requested modem signal changed
- the wait function got interrupted (e.g. by a signal)
- the underlying DLCI was closed
- the underlying ldisc device was removed

I can add that the function returns immediate if a mask has been passed
which matches no modem signal if this is preferred?

Best regards,
Daniel Starke

RE: [PATCH 1/3] tty: n_gsm: add keep alive support

[Starke, Daniel]

> > > > index cb8693b39cb7..b64360aca1f9 100644
> > > > --- a/include/uapi/linux/gsmmux.h
> > > > +++ b/include/uapi/linux/gsmmux.h
> > > > @@ -19,7 +19,8 @@ struct gsm_config
> > > >  	unsigned int mtu;
> > > >  	unsigned int k;
> > > >  	unsigned int i;
> > > > -	unsigned int unused[8];		/* Padding for expansion without
> > > > +	unsigned int keep_alive;
> > > > +	unsigned int unused[7];		/* Padding for expansion without
> > > 
> > > "unsigned int" is not really a valid uapi variable type.
> > > 
> > > Shouldn't this be __u32 instead?
> > 
> > I know but changing it to a fixed size data type may break compatibility
> > as this may change the overall size of the structure.
> 
> Will it?  It shouldn't that's why using the correct data types is
> essencial.

Well, unsigned int is defined to be at least 16 bit. Using __u32 will break
systems where this is true. I am not sure if the Linux kernel targets any
system which defines unsigned int with 16 bit. But sure, I can change it to
__u32.

> > This is why I
> > took a field out of the "unused" array for the "keep_alive" parameter.
> > A value of zero disables keep-alive polling.
> > 
> > > Should you document this field as to what the value is and the units as
> > > you are creating a new user/kernel api here.
> > 
> > I will add a comment here. Comments for the other fields remain subject to
> > another patch.
> > 
> > > And finally, "unused" here is being properly checked to be all 0, right?
> > > If not, then this change can't happen for obvious reasons :(
> > 
> > This was not the case until now. I assumed there was some coding guideline
> > that unused fields need to be initialized to zero. Obviously, checking it
> > prevents misuse here. I will add relevant checks for this.
> 
> If the value was not checked previously, then you can not use the field
> now, otherwise things will break, sorry.  Those are useless fields and
> should be marked as such :(

What is the way forward here? Should I introduce a complete new ioctl?
Or should I use a different size for this structure to break existing code
intentionally? Does this mean that we cannot extend this structure at all
in the future? I had planned another extension here to properly support
parameter negotiation.
In case we need to keep the structure as it is: Would a comment be
sufficient to mark this field accordingly?

Best regards,
Daniel Starke

Re: [PATCH 1/3] tty: n_gsm: add keep alive support

[Greg KH]

On Wed, Feb 01, 2023 at 09:48:58AM +0000, Starke, Daniel wrote:
> > > > > index cb8693b39cb7..b64360aca1f9 100644
> > > > > --- a/include/uapi/linux/gsmmux.h
> > > > > +++ b/include/uapi/linux/gsmmux.h
> > > > > @@ -19,7 +19,8 @@ struct gsm_config
> > > > >  	unsigned int mtu;
> > > > >  	unsigned int k;
> > > > >  	unsigned int i;
> > > > > -	unsigned int unused[8];		/* Padding for expansion without
> > > > > +	unsigned int keep_alive;
> > > > > +	unsigned int unused[7];		/* Padding for expansion without
> > > > 
> > > > "unsigned int" is not really a valid uapi variable type.
> > > > 
> > > > Shouldn't this be __u32 instead?
> > > 
> > > I know but changing it to a fixed size data type may break compatibility
> > > as this may change the overall size of the structure.
> > 
> > Will it?  It shouldn't that's why using the correct data types is
> > essencial.
> 
> Well, unsigned int is defined to be at least 16 bit.

In the kernel, it should just be 32 bits, but as this is a userspace
header, it's pretty undefined :(

> Using __u32 will break
> systems where this is true. I am not sure if the Linux kernel targets any
> system which defines unsigned int with 16 bit. But sure, I can change it to
> __u32.

Please do, that's what those types are for.

> > > This is why I
> > > took a field out of the "unused" array for the "keep_alive" parameter.
> > > A value of zero disables keep-alive polling.
> > > 
> > > > Should you document this field as to what the value is and the units as
> > > > you are creating a new user/kernel api here.
> > > 
> > > I will add a comment here. Comments for the other fields remain subject to
> > > another patch.
> > > 
> > > > And finally, "unused" here is being properly checked to be all 0, right?
> > > > If not, then this change can't happen for obvious reasons :(
> > > 
> > > This was not the case until now. I assumed there was some coding guideline
> > > that unused fields need to be initialized to zero. Obviously, checking it
> > > prevents misuse here. I will add relevant checks for this.
> > 
> > If the value was not checked previously, then you can not use the field
> > now, otherwise things will break, sorry.  Those are useless fields and
> > should be marked as such :(
> 
> What is the way forward here? Should I introduce a complete new ioctl?

You are going to have to, as if the unused fields were never verified to
be 0, you will have bugs where old userspace code could have data in
that location that previously worked just fine, and now it does not at
all.  We can not cause regressions like that, sorry.

> Or should I use a different size for this structure to break existing code
> intentionally?

You can not break userspace, sorry.

> Does this mean that we cannot extend this structure at all in the
> future?

That is correct, it is a broken interface it seems :(

> I had planned another extension here to properly support parameter
> negotiation.
> In case we need to keep the structure as it is: Would a comment be
> sufficient to mark this field accordingly?

Yes, mark it as "can not be used" or something like that, and then just
create a new ioctl, sorry.

thanks,

greg k-h

Re: [PATCH 3/3] tty: n_gsm: add TIOCMIWAIT support

[Greg KH]

On Wed, Feb 01, 2023 at 09:30:28AM +0000, Starke, Daniel wrote:
> > > Add support for the TIOCMIWAIT ioctl on the virtual ttys. This enables the
> > > user to wait for virtual modem signals like RING.
> > > 
> > > Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
> > > ---
> > >  drivers/tty/n_gsm.c | 32 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > > index 118511c1fa37..48fb7dad44cd 100644
> > > --- a/drivers/tty/n_gsm.c
> > > +++ b/drivers/tty/n_gsm.c
> > > @@ -1542,6 +1542,7 @@ static void gsm_process_modem(struct tty_struct *tty, struct gsm_dlci *dlci,
> > >  	if (brk & 0x01)
> > >  		tty_insert_flip_char(&dlci->port, 0, TTY_BREAK);
> > >  	dlci->modem_rx = mlines;
> > > +	wake_up_interruptible(&dlci->gsm->event);
> > >  }
> > >  
> > >  /**
> > > @@ -3848,6 +3849,35 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk)
> > >  	return -EPROTONOSUPPORT;
> > >  }
> > >  
> > > +/**
> > > + * gsm_wait_modem_change	-	wait for modem status line change
> > 
> > No need for tabs.
> > 
> > Or for kernel doc for static functions, but that's not a big deal.
> 
> All other functions follow the same style here. Nevertheless, I will
> replace the tabs with spaces. Originally, I had planned a code clean-up
> after committing all the changes from my original RFC.

Ah, ok, nevermind then, I didn't see that as it's not in the diffs.

Cleaning up later is fine.

> > > +static int gsm_wait_modem_change(struct gsm_dlci *dlci, u32 mask)
> > > +{
> > > +	struct gsm_mux *gsm = dlci->gsm;
> > > +	u32 old = dlci->modem_rx & mask;
> > > +	int ret;
> > > +
> > > +	if (gsm->dead)
> > > +		return -ENODEV;
> > > +
> > > +	do {
> > > +		ret = wait_event_interruptible(gsm->event, gsm->dead
> > > +					      || old ^ (dlci->modem_rx & mask));
> > > +		if (ret)
> > > +			return ret;
> > > +		if (dlci->state != DLCI_OPEN)
> > > +			return -EL2NSYNC;
> > > +		if (gsm->dead)
> > > +			return -ENODEV;
> > > +	} while ((old ^ (dlci->modem_rx & mask)) == 0);
> > 
> > No way to break out of the loop if it goes for forever?
> 
> I assume that this is the expected behavior for TIOCMIWAIT. The functions
> returns if:
> - the requested modem signal changed
> - the wait function got interrupted (e.g. by a signal)
> - the underlying DLCI was closed
> - the underlying ldisc device was removed

Hm, I guess you are right.  But wow, reading that
wait_event_interruptible() condition is crazy, please document that
really well to explain this properly so you will be able to understand
it in a year when you next have to fix it up :)

> I can add that the function returns immediate if a mask has been passed
> which matches no modem signal if this is preferred?

I don't think that would work, try it on some existing serial ports to
see what they do.

thanks,

greg k-h