[PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration

[PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration

[D. Starke]

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

Parameter negotiation has been introduced with
commit 92f1f0c3290d ("tty: n_gsm: add parameter negotiation support")

However, means to set individual parameters per DLCI are not yet
implemented. Furthermore, it is currently not possible to keep a DLCI half
open until the user application sets the right parameters for it. This is
required to allow a user application to set its specific parameters before
the underlying link is established. Otherwise, the link is opened and
re-established right afterwards if the user application sets incompatible
parameters. This may be an unexpected behavior for the peer.

Add parameter 'wait_config' to 'gsm_config' to support setups where the
DLCI specific user application sets its specific parameters after open()
and before the link gets fully established. Setting this to zero disables
the user application specific DLCI configuration option.

Add the ioctls 'GSMIOC_GETCONF_DLCI' and 'GSMIOC_SETCONF_DLCI' for the
ldisc and virtual ttys. This gets/sets the DLCI specific parameters and may
trigger a reconnect of the DLCI if incompatible values have been set. Only
the parameters for the DLCI associated with the virtual tty can be set or
retrieved if called on these.

Add remark within the documentation to introduce the new ioctls.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 Documentation/driver-api/tty/n_gsm.rst |   4 +
 drivers/tty/n_gsm.c                    | 193 ++++++++++++++++++++++++-
 include/uapi/linux/gsmmux.h            |  17 ++-
 3 files changed, 208 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/tty/n_gsm.rst b/Documentation/driver-api/tty/n_gsm.rst
index 9447b8a3b8e2..c2624546fb8f 100644
--- a/Documentation/driver-api/tty/n_gsm.rst
+++ b/Documentation/driver-api/tty/n_gsm.rst
@@ -29,6 +29,8 @@ Config Initiator
 
 #. Configure the mux using ``GSMIOC_GETCONF``/``GSMIOC_SETCONF`` ioctl.
 
+#. Configure DLCs using ``GSMIOC_GETCONF_DLCI``/``GSMIOC_SETCONF_DLCI`` ioctl for non-defaults.
+
 #. Obtain base gsmtty number for the used serial port.
 
    Major parts of the initialization program
@@ -120,6 +122,8 @@ Config Requester
 
 #. Configure the mux using ``GSMIOC_GETCONF``/``GSMIOC_SETCONF`` ioctl.
 
+#. Configure DLCs using ``GSMIOC_GETCONF_DLCI``/``GSMIOC_SETCONF_DLCI`` ioctl for non-defaults.
+
 #. Obtain base gsmtty number for the used serial port::
 
         #include <stdio.h>
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 7aa05ebed8e1..3ca33149c339 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -128,6 +128,7 @@ struct gsm_msg {
 
 enum gsm_dlci_state {
 	DLCI_CLOSED,
+	DLCI_WAITING_CONFIG,	/* Waiting for DLCI configuration from user */
 	DLCI_CONFIGURE,		/* Sending PN (for adaption > 1) */
 	DLCI_OPENING,		/* Sending SABM not seen UA */
 	DLCI_OPEN,		/* SABM/UA complete */
@@ -330,6 +331,7 @@ struct gsm_mux {
 	unsigned int t3;	/* Power wake-up timer in seconds. */
 	int n2;			/* Retry count */
 	u8 k;			/* Window size */
+	bool wait_config;	/* Wait for configuration by ioctl before DLCI open */
 	u32 keep_alive;		/* Control channel keep-alive in 10ms */
 
 	/* Statistics (not currently exposed) */
@@ -451,6 +453,7 @@ static int gsm_modem_update(struct gsm_dlci *dlci, u8 brk);
 static struct gsm_msg *gsm_data_alloc(struct gsm_mux *gsm, u8 addr, int len,
 								u8 ctrl);
 static int gsm_send_packet(struct gsm_mux *gsm, struct gsm_msg *msg);
+static struct gsm_dlci *gsm_dlci_alloc(struct gsm_mux *gsm, int addr);
 static void gsmld_write_trigger(struct gsm_mux *gsm);
 static void gsmld_write_task(struct work_struct *work);
 
@@ -2280,6 +2283,7 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 
 	switch (dlci->state) {
 	case DLCI_CLOSED:
+	case DLCI_WAITING_CONFIG:
 	case DLCI_CLOSING:
 		dlci->retries = gsm->n2;
 		if (!need_pn) {
@@ -2311,6 +2315,7 @@ static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
 {
 	switch (dlci->state) {
 	case DLCI_CLOSED:
+	case DLCI_WAITING_CONFIG:
 	case DLCI_CLOSING:
 		dlci->state = DLCI_OPENING;
 		break;
@@ -2319,6 +2324,24 @@ static void gsm_dlci_set_opening(struct gsm_dlci *dlci)
 	}
 }
 
+/**
+ * gsm_dlci_set_wait_config	-	wait for channel configuration
+ * @dlci: DLCI to configure
+ *
+ * Wait for a DLCI configuration from the application.
+ */
+static void gsm_dlci_set_wait_config(struct gsm_dlci *dlci)
+{
+	switch (dlci->state) {
+	case DLCI_CLOSED:
+	case DLCI_CLOSING:
+		dlci->state = DLCI_WAITING_CONFIG;
+		break;
+	default:
+		break;
+	}
+}
+
 /**
  *	gsm_dlci_begin_close	-	start channel open procedure
  *	@dlci: DLCI to open
@@ -2453,6 +2476,128 @@ static void gsm_kick_timer(struct timer_list *t)
 		pr_info("%s TX queue stalled\n", __func__);
 }
 
+/**
+ * gsm_dlci_copy_config_values	-	copy DLCI configuration
+ * @dlci: source DLCI
+ * @dc: configuration structure to fill
+ */
+static void gsm_dlci_copy_config_values(struct gsm_dlci *dlci, struct gsm_dlci_config *dc)
+{
+	memset(dc, 0, sizeof(*dc));
+	dc->channel = (u32)dlci->addr;
+	dc->adaption = (u32)dlci->adaption;
+	dc->mtu = (u32)dlci->mtu;
+	dc->priority = (u32)dlci->prio;
+	if (dlci->ftype == UIH)
+		dc->i = 1;
+	else
+		dc->i = 2;
+	dc->k = (u32)dlci->k;
+}
+
+/**
+ * gsm_dlci_config	-	configure DLCI from configuration
+ * @dlci: DLCI to configure
+ * @dc: DLCI configuration
+ * @open: open DLCI after configuration?
+ */
+static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, int open)
+{
+	struct gsm_mux *gsm;
+	bool need_restart = false;
+	bool need_open = false;
+	unsigned int i;
+
+	/*
+	 * Check that userspace doesn't put stuff in here to prevent breakages
+	 * in the future.
+	 */
+	for (i = 0; i < ARRAY_SIZE(dc->reserved); i++)
+		if (dc->reserved[i])
+			return -EINVAL;
+
+	if (!dlci)
+		return -EINVAL;
+	gsm = dlci->gsm;
+
+	/* Stuff we don't support yet - I frame transport */
+	if (dc->adaption != 1 && dc->adaption != 2)
+		return -EOPNOTSUPP;
+	if (dc->mtu > MAX_MTU || dc->mtu < MIN_MTU || (unsigned int)dc->mtu > gsm->mru)
+		return -EINVAL;
+	if (dc->priority >= 64)
+		return -EINVAL;
+	if (dc->i == 0 || dc->i > 2)  /* UIH and UI only */
+		return -EINVAL;
+	if (dc->k > 7)
+		return -EINVAL;
+
+	/*
+	 * See what is needed for reconfiguration
+	 */
+	/* Framing fields */
+	if ((int)dc->adaption != dlci->adaption)
+		need_restart = true;
+	if ((unsigned int)dc->mtu != dlci->mtu)
+		need_restart = true;
+	if ((u8)dc->i != dlci->ftype)
+		need_restart = true;
+	/* Requires care */
+	if ((u8)dc->priority != (u32)dlci->prio)
+		need_restart = true;
+
+	if ((open && gsm->wait_config) || need_restart)
+		need_open = true;
+	if (dlci->state == DLCI_WAITING_CONFIG) {
+		need_restart = false;
+		need_open = true;
+	}
+
+	/*
+	 * Close down what is needed, restart and initiate the new
+	 * configuration.
+	 */
+	if (need_restart) {
+		gsm_dlci_begin_close(dlci);
+		wait_event_interruptible(gsm->event, dlci->state == DLCI_CLOSED);
+		if (signal_pending(current))
+			return -EINTR;
+	}
+	/*
+	 * Setup the new configuration values
+	 */
+	dlci->adaption = (int)dc->adaption;
+
+	if (dlci->mtu)
+		dlci->mtu = (unsigned int)dc->mtu;
+	else
+		dlci->mtu = gsm->mtu;
+
+	if (dc->priority)
+		dlci->prio = (u8)dc->priority;
+	else
+		dlci->prio = roundup(dlci->addr + 1, 8) - 1;
+
+	if (dc->i == 1)
+		dlci->ftype = UIH;
+	else if (dc->i == 2)
+		dlci->ftype = UI;
+
+	if (dc->k)
+		dlci->k = (u8)dc->k;
+	else
+		dlci->k = gsm->k;
+
+	if (need_open) {
+		if (gsm->initiator)
+			gsm_dlci_begin_open(dlci);
+		else
+			gsm_dlci_set_opening(dlci);
+	}
+
+	return 0;
+}
+
 /*
  *	Allocate/Free DLCI channels
  */
@@ -3078,6 +3223,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->wait_config = false; /* Disabled */
 	gsm->keep_alive = 0;	/* Disabled */
 
 	/* Store the instance to the mux array or abort if no space is
@@ -3218,6 +3364,7 @@ static void gsm_copy_config_ext_values(struct gsm_mux *gsm,
 				       struct gsm_config_ext *ce)
 {
 	memset(ce, 0, sizeof(*ce));
+	ce->wait_config = gsm->wait_config ? 1 : 0;
 	ce->keep_alive = gsm->keep_alive;
 }
 
@@ -3233,7 +3380,12 @@ static int gsm_config_ext(struct gsm_mux *gsm, struct gsm_config_ext *ce)
 		if (ce->reserved[i])
 			return -EINVAL;
 
+	/*
+	 * Setup the new configuration values
+	 */
+	gsm->wait_config = ce->wait_config ? true : false;
 	gsm->keep_alive = ce->keep_alive;
+
 	return 0;
 }
 
@@ -3436,6 +3588,14 @@ static int gsmld_open(struct tty_struct *tty)
 	gsm->encoding = GSM_ADV_OPT;
 	gsmld_attach_gsm(tty, gsm);
 
+	/* The mux will not be activated yet, we wait for correct
+	 * configuration first.
+	 */
+	if (gsm->encoding == GSM_BASIC_OPT)
+		gsm->receive = gsm0_receive;
+	else
+		gsm->receive = gsm1_receive;
+
 	return 0;
 }
 
@@ -3557,6 +3717,7 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
 	struct gsm_config c;
 	struct gsm_config_ext ce;
 	struct gsm_mux *gsm = tty->disc_data;
+	struct gsm_dlci *dlci;
 	unsigned int base;
 
 	switch (cmd) {
@@ -4008,7 +4169,6 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	struct tty_port *port = &dlci->port;
-	struct gsm_mux *gsm = dlci->gsm;
 
 	port->count++;
 	tty_port_tty_set(port, tty);
@@ -4018,10 +4178,15 @@ static int gsmtty_open(struct tty_struct *tty, struct file *filp)
 	   a DM straight back. This is ok as that will have caused a hangup */
 	tty_port_set_initialized(port, true);
 	/* Start sending off SABM messages */
-	if (gsm->initiator)
-		gsm_dlci_begin_open(dlci);
-	else
-		gsm_dlci_set_opening(dlci);
+	if (!dlci->gsm->wait_config) {
+		/* Start sending off SABM messages */
+		if (dlci->gsm->initiator)
+			gsm_dlci_begin_open(dlci);
+		else
+			gsm_dlci_set_opening(dlci);
+	} else {
+		gsm_dlci_set_wait_config(dlci);
+	}
 	/* And wait for virtual carrier */
 	return tty_port_block_til_ready(port, tty, filp);
 }
@@ -4142,6 +4307,7 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	struct gsm_netconfig nc;
+	struct gsm_dlci_config dc;
 	int index;
 
 	if (dlci->state == DLCI_CLOSED)
@@ -4165,6 +4331,23 @@ static int gsmtty_ioctl(struct tty_struct *tty,
 		gsm_destroy_network(dlci);
 		mutex_unlock(&dlci->mutex);
 		return 0;
+	case GSMIOC_GETCONF_DLCI:
+		if (copy_from_user(&dc, (void __user *)arg, sizeof(dc)))
+			return -EFAULT;
+		if (dc.channel != dlci->addr)
+			return -EPERM;
+		gsm_dlci_copy_config_values(dlci, &dc);
+		if (copy_to_user((void __user *)arg, &dc, sizeof(dc)))
+			return -EFAULT;
+		return 0;
+	case GSMIOC_SETCONF_DLCI:
+		if (copy_from_user(&dc, (void __user *)arg, sizeof(dc)))
+			return -EFAULT;
+		if (dc.channel >= NUM_DLCI)
+			return -EINVAL;
+		if (dc.channel != 0 && dc.channel != dlci->addr)
+			return -EPERM;
+		return gsm_dlci_config(dlci, &dc, 1);
 	case TIOCMIWAIT:
 		return gsm_wait_modem_change(dlci, (u32)arg);
 	default:
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index a703780aa095..eb67884e5f38 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -43,10 +43,25 @@ struct gsm_config_ext {
 	__u32 keep_alive;	/* Control channel keep-alive in 1/100th of a
 				 * second (0 to disable)
 				 */
-	__u32 reserved[7];	/* For future use, must be initialized to zero */
+	__u32 wait_config;	/* Wait for DLCI config before opening virtual link? */
+	__u32 reserved[6];	/* For future use, must be initialized to zero */
 };
 
 #define GSMIOC_GETCONF_EXT	_IOR('G', 5, struct gsm_config_ext)
 #define GSMIOC_SETCONF_EXT	_IOW('G', 6, struct gsm_config_ext)
 
+/* Set channel accordingly before calling GSMIOC_GETCONF_DLCI. */
+struct gsm_dlci_config {
+	__u32 channel;		/* DLCI (0 for the associated DLCI) */
+	__u32 adaption;		/* Convergence layer type */
+	__u32 mtu;		/* Maximum transfer unit */
+	__u32 priority;		/* Priority (0 for default value) */
+	__u32 i;		/* Frame type (1 = UIH, 2 = UI) */
+	__u32 k;		/* Window size (0 for default value) */
+	__u32 reserved[8];	/* For future use, must be initialized to zero */
+};
+
+#define GSMIOC_GETCONF_DLCI	_IOWR('G', 7, struct gsm_dlci_config)
+#define GSMIOC_SETCONF_DLCI	_IOW('G', 8, struct gsm_dlci_config)
+
 #endif
-- 
2.34.1

[PATCH 2/3] tty: n_gsm: allow window size configuration

[D. Starke]

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

n_gsm is based on the 3GPP 07.010 and its newer version is the 3GPP 27.010.
See https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=1516
The changes from 07.010 to 27.010 are non-functional. Therefore, I refer to
the newer 27.010 here. Chapter 6 describes the error recovery mode option
which is based on I frames. The k parameter defines the maximum number of
I frames that a DLC can have unacknowledged as described in chapter 5.7.4.
The current n_gsm implementation does not support the error recovery mode
option. However, the k parameter is also part of the parameter negotiation
message as described in chapter 5.4.6.3.1. Chapter 5.7.4 also notes that
the allowed value range for k is 1-7. That means a 0 is counted as invalid
here. This means that the user needs to configure a valid value here even
if the function itself is not supported. Otherwise, parameter negotiation
may fail.

Allow setting of k via ioctl in gsm_config(). Range checks are already
included.

Signed-off-by: Daniel Starke <daniel.starke@siemens.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 3ca33149c339..494657e8535d 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3276,8 +3276,8 @@ static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	int need_close = 0;
 	int need_restart = 0;
 
-	/* Stuff we don't support yet - UI or I frame transport, windowing */
-	if ((c->adaption != 1 && c->adaption != 2) || c->k)
+	/* Stuff we don't support yet - UI or I frame transport */
+	if (c->adaption != 1 && c->adaption != 2)
 		return -EOPNOTSUPP;
 	/* Check the MRU/MTU range looks sane */
 	if (c->mru < MIN_MTU || c->mtu < MIN_MTU)
-- 
2.34.1

[PATCH 3/3] tty: n_gsm: add ioctl for DLC config via ldisc handle

[D. Starke]

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

The application which initializes the n_gsm ldisc may like to set
individual default parameters for each DLCI or take over of the application
specific DLCI configuration completely. This is currently not possible.
It is either possible to set common default parameters for all DLCIs or let
the user application set its DLCI specific configuration parameters.

Add support of GSMIOC_GETCONF_DLCI and GSMIOC_SETCONF_DLCI for the n_gsm
ldisc handle to support DLCI specific parameter configuration upfront.

Add a code example for this use case to the n_gsm documentation.

Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
---
 Documentation/driver-api/tty/n_gsm.rst | 16 +++++++++++++++
 drivers/tty/n_gsm.c                    | 28 ++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/Documentation/driver-api/tty/n_gsm.rst b/Documentation/driver-api/tty/n_gsm.rst
index c2624546fb8f..120317ec990f 100644
--- a/Documentation/driver-api/tty/n_gsm.rst
+++ b/Documentation/driver-api/tty/n_gsm.rst
@@ -47,6 +47,7 @@ Config Initiator
       int ldisc = N_GSM0710;
       struct gsm_config c;
       struct gsm_config_ext ce;
+      struct gsm_dlci_config dc;
       struct termios configuration;
       uint32_t first;
 
@@ -83,6 +84,13 @@ Config Initiator
       c.mtu = 127;
       /* set the new configuration */
       ioctl(fd, GSMIOC_SETCONF, &c);
+      /* get DLC 1 configuration */
+      dc.channel = 1;
+      ioctl(fd, GSMIOC_GETCONF_DLCI, &dc);
+      /* the first user channel gets a higher priority */
+      dc.priority = 1;
+      /* set the new DLC 1 specific configuration */
+      ioctl(fd, GSMIOC_SETCONF_DLCI, &dc);
       /* get first gsmtty device node */
       ioctl(fd, GSMIOC_GETFIRST, &first);
       printf("first muxed line: /dev/gsmtty%i\n", first);
@@ -136,6 +144,7 @@ Config Requester
 	int ldisc = N_GSM0710;
 	struct gsm_config c;
 	struct gsm_config_ext ce;
+	struct gsm_dlci_config dc;
 	struct termios configuration;
 	uint32_t first;
 
@@ -165,6 +174,13 @@ Config Requester
 	c.mtu = 127;
 	/* set the new configuration */
 	ioctl(fd, GSMIOC_SETCONF, &c);
+	/* get DLC 1 configuration */
+	dc.channel = 1;
+	ioctl(fd, GSMIOC_GETCONF_DLCI, &dc);
+	/* the first user channel gets a higher priority */
+	dc.priority = 1;
+	/* set the new DLC 1 specific configuration */
+	ioctl(fd, GSMIOC_SETCONF_DLCI, &dc);
 	/* get first gsmtty device node */
 	ioctl(fd, GSMIOC_GETFIRST, &first);
 	printf("first muxed line: /dev/gsmtty%i\n", first);
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 494657e8535d..86f106545958 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3716,6 +3716,7 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
 {
 	struct gsm_config c;
 	struct gsm_config_ext ce;
+	struct gsm_dlci_config dc;
 	struct gsm_mux *gsm = tty->disc_data;
 	struct gsm_dlci *dlci;
 	unsigned int base;
@@ -3742,6 +3743,33 @@ static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
 		if (copy_from_user(&ce, (void __user *)arg, sizeof(ce)))
 			return -EFAULT;
 		return gsm_config_ext(gsm, &ce);
+	case GSMIOC_GETCONF_DLCI:
+		if (copy_from_user(&dc, (void __user *)arg, sizeof(dc)))
+			return -EFAULT;
+		if (dc.channel == 0 || dc.channel >= NUM_DLCI)
+			return -EINVAL;
+		dlci = gsm->dlci[dc.channel];
+		if (!dlci) {
+			dlci = gsm_dlci_alloc(gsm, dc.channel);
+			if (!dlci)
+				return -ENOMEM;
+		}
+		gsm_dlci_copy_config_values(dlci, &dc);
+		if (copy_to_user((void __user *)arg, &dc, sizeof(dc)))
+			return -EFAULT;
+		return 0;
+	case GSMIOC_SETCONF_DLCI:
+		if (copy_from_user(&dc, (void __user *)arg, sizeof(dc)))
+			return -EFAULT;
+		if (dc.channel == 0 || dc.channel >= NUM_DLCI)
+			return -EINVAL;
+		dlci = gsm->dlci[dc.channel];
+		if (!dlci) {
+			dlci = gsm_dlci_alloc(gsm, dc.channel);
+			if (!dlci)
+				return -ENOMEM;
+		}
+		return gsm_dlci_config(dlci, &dc, 0);
 	default:
 		return n_tty_ioctl_helper(tty, cmd, arg);
 	}
-- 
2.34.1

Re: [PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration

[Jiri Slaby]

On 28. 02. 23, 7:29, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> Parameter negotiation has been introduced with
> commit 92f1f0c3290d ("tty: n_gsm: add parameter negotiation support")
> 
> However, means to set individual parameters per DLCI are not yet
> implemented. Furthermore, it is currently not possible to keep a DLCI half
> open until the user application sets the right parameters for it. This is
> required to allow a user application to set its specific parameters before
> the underlying link is established. Otherwise, the link is opened and
> re-established right afterwards if the user application sets incompatible
> parameters. This may be an unexpected behavior for the peer.
> 
> Add parameter 'wait_config' to 'gsm_config' to support setups where the
> DLCI specific user application sets its specific parameters after open()
> and before the link gets fully established. Setting this to zero disables
> the user application specific DLCI configuration option.
> 
> Add the ioctls 'GSMIOC_GETCONF_DLCI' and 'GSMIOC_SETCONF_DLCI' for the
> ldisc and virtual ttys. This gets/sets the DLCI specific parameters and may
> trigger a reconnect of the DLCI if incompatible values have been set. Only
> the parameters for the DLCI associated with the virtual tty can be set or
> retrieved if called on these.
> 
> Add remark within the documentation to introduce the new ioctls.
> 
> Signed-off-by: Daniel Starke <daniel.starke@siemens.com>
...
> @@ -2453,6 +2476,128 @@ static void gsm_kick_timer(struct timer_list *t)
>   		pr_info("%s TX queue stalled\n", __func__);
>   }
>   
> +/**
> + * gsm_dlci_copy_config_values	-	copy DLCI configuration
> + * @dlci: source DLCI
> + * @dc: configuration structure to fill
> + */
> +static void gsm_dlci_copy_config_values(struct gsm_dlci *dlci, struct gsm_dlci_config *dc)
> +{
> +	memset(dc, 0, sizeof(*dc));
> +	dc->channel = (u32)dlci->addr;
> +	dc->adaption = (u32)dlci->adaption;
> +	dc->mtu = (u32)dlci->mtu;
> +	dc->priority = (u32)dlci->prio;
> +	if (dlci->ftype == UIH)
> +		dc->i = 1;
> +	else
> +		dc->i = 2;
> +	dc->k = (u32)dlci->k;

Why all those casts?

> +}
> +
> +/**
> + * gsm_dlci_config	-	configure DLCI from configuration
> + * @dlci: DLCI to configure
> + * @dc: DLCI configuration
> + * @open: open DLCI after configuration?
> + */
> +static int gsm_dlci_config(struct gsm_dlci *dlci, struct gsm_dlci_config *dc, int open)
> +{
> +	struct gsm_mux *gsm;
> +	bool need_restart = false;
> +	bool need_open = false;
> +	unsigned int i;
> +
> +	/*
> +	 * Check that userspace doesn't put stuff in here to prevent breakages
> +	 * in the future.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(dc->reserved); i++)
> +		if (dc->reserved[i])
> +			return -EINVAL;
> +
> +	if (!dlci)
> +		return -EINVAL;
> +	gsm = dlci->gsm;
> +
> +	/* Stuff we don't support yet - I frame transport */
> +	if (dc->adaption != 1 && dc->adaption != 2)
> +		return -EOPNOTSUPP;
> +	if (dc->mtu > MAX_MTU || dc->mtu < MIN_MTU || (unsigned int)dc->mtu > gsm->mru)
> +		return -EINVAL;
> +	if (dc->priority >= 64)
> +		return -EINVAL;
> +	if (dc->i == 0 || dc->i > 2)  /* UIH and UI only */
> +		return -EINVAL;
> +	if (dc->k > 7)
> +		return -EINVAL;
> +
> +	/*
> +	 * See what is needed for reconfiguration
> +	 */
> +	/* Framing fields */
> +	if ((int)dc->adaption != dlci->adaption)
> +		need_restart = true;
> +	if ((unsigned int)dc->mtu != dlci->mtu)
> +		need_restart = true;
> +	if ((u8)dc->i != dlci->ftype)
> +		need_restart = true;
> +	/* Requires care */
> +	if ((u8)dc->priority != (u32)dlci->prio)
> +		need_restart = true;

And here.

> +
> +	if ((open && gsm->wait_config) || need_restart)
> +		need_open = true;
> +	if (dlci->state == DLCI_WAITING_CONFIG) {
> +		need_restart = false;
> +		need_open = true;
> +	}
> +
> +	/*
> +	 * Close down what is needed, restart and initiate the new
> +	 * configuration.
> +	 */
> +	if (need_restart) {
> +		gsm_dlci_begin_close(dlci);
> +		wait_event_interruptible(gsm->event, dlci->state == DLCI_CLOSED);
> +		if (signal_pending(current))
> +			return -EINTR;
> +	}
> +	/*
> +	 * Setup the new configuration values
> +	 */
> +	dlci->adaption = (int)dc->adaption;
> +
> +	if (dlci->mtu)

dc->mtu?

> +		dlci->mtu = (unsigned int)dc->mtu;
> +	else
> +		dlci->mtu = gsm->mtu;
> +
> +	if (dc->priority)
> +		dlci->prio = (u8)dc->priority;
> +	else
> +		dlci->prio = roundup(dlci->addr + 1, 8) - 1;
> +
> +	if (dc->i == 1)
> +		dlci->ftype = UIH;
> +	else if (dc->i == 2)
> +		dlci->ftype = UI;
> +
> +	if (dc->k)
> +		dlci->k = (u8)dc->k;
> +	else
> +		dlci->k = gsm->k;
> +
> +	if (need_open) {
> +		if (gsm->initiator)
> +			gsm_dlci_begin_open(dlci);
> +		else
> +			gsm_dlci_set_opening(dlci);
> +	}
> +
> +	return 0;
> +}
> +
>   /*
>    *	Allocate/Free DLCI channels
>    */

-- 
js
suse labs

RE: [PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration

[Starke, Daniel]

> > +static void gsm_dlci_copy_config_values(struct gsm_dlci *dlci, struct gsm_dlci_config *dc)
> > +{
> > +	memset(dc, 0, sizeof(*dc));
> > +	dc->channel = (u32)dlci->addr;
> > +	dc->adaption = (u32)dlci->adaption;
> > +	dc->mtu = (u32)dlci->mtu;
> > +	dc->priority = (u32)dlci->prio;
> > +	if (dlci->ftype == UIH)
> > +		dc->i = 1;
> > +	else
> > +		dc->i = 2;
> > +	dc->k = (u32)dlci->k;
> 
> Why all those casts?

These fields in 'dlci' differ either in size and/or signedness from the
values of the ioctl structure for historic reasons. That is why I perform
explicit casts here.

> > +	if ((int)dc->adaption != dlci->adaption)
> > +		need_restart = true;
> > +	if ((unsigned int)dc->mtu != dlci->mtu)
> > +		need_restart = true;
> > +	if ((u8)dc->i != dlci->ftype)
> > +		need_restart = true;
> > +	/* Requires care */
> > +	if ((u8)dc->priority != (u32)dlci->prio)
> > +		need_restart = true;
> 
> And here.

I will remove these.

> > +	if (dlci->mtu)
> 
> dc->mtu?

Right, my mistake. Will be corrected in the next version of this patch.


Best regards,
Daniel Starke

Re: [PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration

[kernel test robot]

Hi Starke,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus]
[cannot apply to v6.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/D-Starke/tty-n_gsm-allow-window-size-configuration/20230228-143349
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20230228062957.3150-1-daniel.starke%40siemens.com
patch subject: [PATCH 1/3] tty: n_gsm: add ioctl for DLC specific parameter configuration
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230228/202302281856.S9Lz4gHB-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/06d7556b46ca2395b18cb700f19ee5de37d8383b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review D-Starke/tty-n_gsm-allow-window-size-configuration/20230228-143349
        git checkout 06d7556b46ca2395b18cb700f19ee5de37d8383b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302281856.S9Lz4gHB-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/tty/n_gsm.c: In function 'gsmld_ioctl':
>> drivers/tty/n_gsm.c:3720:26: warning: unused variable 'dlci' [-Wunused-variable]
    3720 |         struct gsm_dlci *dlci;
         |                          ^~~~


vim +/dlci +3720 drivers/tty/n_gsm.c

  3713	
  3714	static int gsmld_ioctl(struct tty_struct *tty, unsigned int cmd,
  3715			       unsigned long arg)
  3716	{
  3717		struct gsm_config c;
  3718		struct gsm_config_ext ce;
  3719		struct gsm_mux *gsm = tty->disc_data;
> 3720		struct gsm_dlci *dlci;
  3721		unsigned int base;
  3722	
  3723		switch (cmd) {
  3724		case GSMIOC_GETCONF:
  3725			gsm_copy_config_values(gsm, &c);
  3726			if (copy_to_user((void __user *)arg, &c, sizeof(c)))
  3727				return -EFAULT;
  3728			return 0;
  3729		case GSMIOC_SETCONF:
  3730			if (copy_from_user(&c, (void __user *)arg, sizeof(c)))
  3731				return -EFAULT;
  3732			return gsm_config(gsm, &c);
  3733		case GSMIOC_GETFIRST:
  3734			base = mux_num_to_base(gsm);
  3735			return put_user(base + 1, (__u32 __user *)arg);
  3736		case GSMIOC_GETCONF_EXT:
  3737			gsm_copy_config_ext_values(gsm, &ce);
  3738			if (copy_to_user((void __user *)arg, &ce, sizeof(ce)))
  3739				return -EFAULT;
  3740			return 0;
  3741		case GSMIOC_SETCONF_EXT:
  3742			if (copy_from_user(&ce, (void __user *)arg, sizeof(ce)))
  3743				return -EFAULT;
  3744			return gsm_config_ext(gsm, &ce);
  3745		default:
  3746			return n_tty_ioctl_helper(tty, cmd, arg);
  3747		}
  3748	}
  3749	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests