diff mbox series

[1/1] tty: n_gsm: make n_gsm line number configurable

Message ID 20221012061715.4823-1-daniel.starke@siemens.com
State New
Headers show
Series [1/1] tty: n_gsm: make n_gsm line number configurable | expand

Commit Message

D. Starke Oct. 12, 2022, 6:17 a.m. UTC
From: Daniel Starke <daniel.starke@siemens.com>

Currently, the n_gsm line number and its derived virtual ttys are assigned
in the order of allocations with no means to change this.

Introduce additional ioctl parameters numValid and num to configure the
line number to allow predictable virtual tty allocation and numbering.
Especially when using multiple n_gsm instances at the same time.

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

Comments

Greg Kroah-Hartman Oct. 12, 2022, 6:50 a.m. UTC | #1
On Wed, Oct 12, 2022 at 08:17:15AM +0200, D. Starke wrote:
> From: Daniel Starke <daniel.starke@siemens.com>
> 
> Currently, the n_gsm line number and its derived virtual ttys are assigned
> in the order of allocations with no means to change this.

Which is fine, why do you need this to be changed?  What relies on
specific line numbers in userspace that can not handle things correctly
with the normal userspace tools we have for this type of thing?

> Introduce additional ioctl parameters numValid and num to configure the
> line number to allow predictable virtual tty allocation and numbering.
> Especially when using multiple n_gsm instances at the same time.

Ick, please no.  That should never be needed, the kernel number and name
can be anything random (and people have suggested that we do make them
random at times).  Don't hard-code numbers in your userspace tools, that
is just wrong.

> --- a/include/uapi/linux/gsmmux.h
> +++ b/include/uapi/linux/gsmmux.h
> @@ -19,7 +19,9 @@ struct gsm_config
>  	unsigned int mtu;
>  	unsigned int k;
>  	unsigned int i;
> -	unsigned int unused[8];		/* Padding for expansion without
> +	unsigned short numValid;
> +	unsigned short num;

This would never work anyway (hint "short" is not a valid uapi data
type...)

thanks,

greg k-h
D. Starke Oct. 13, 2022, 11:09 a.m. UTC | #2
> > Currently, the n_gsm line number and its derived virtual ttys are 
> > assigned in the order of allocations with no means to change this.
> 
> Which is fine, why do you need this to be changed?  What relies on specific
> line numbers in userspace that can not handle things correctly with the
> normal userspace tools we have for this type of thing?
> 
> > Introduce additional ioctl parameters numValid and num to configure 
> > the line number to allow predictable virtual tty allocation and numbering.
> > Especially when using multiple n_gsm instances at the same time.
> 
> Ick, please no.  That should never be needed, the kernel number and name
> can be anything random (and people have suggested that we do make them
> random at times).  Don't hard-code numbers in your userspace tools, that
> is just wrong.

Thank you for your feedback and review. I understand your points.
Please discard this patch.

> > --- a/include/uapi/linux/gsmmux.h
> > +++ b/include/uapi/linux/gsmmux.h
> > @@ -19,7 +19,9 @@ struct gsm_config
> >  	unsigned int mtu;
> >  	unsigned int k;
> >  	unsigned int i;
> > -	unsigned int unused[8];		/* Padding for expansion without
> > +	unsigned short numValid;
> > +	unsigned short num;
> 
> This would never work anyway (hint "short" is not a valid uapi data
> type...)

I am surprised about this as gsm_netconfig already uses unsigned short.

Best regards,
Daniel Starke
Greg Kroah-Hartman Oct. 13, 2022, 12:57 p.m. UTC | #3
On Thu, Oct 13, 2022 at 11:09:02AM +0000, Starke, Daniel wrote:
> > > --- a/include/uapi/linux/gsmmux.h
> > > +++ b/include/uapi/linux/gsmmux.h
> > > @@ -19,7 +19,9 @@ struct gsm_config
> > >  	unsigned int mtu;
> > >  	unsigned int k;
> > >  	unsigned int i;
> > > -	unsigned int unused[8];		/* Padding for expansion without
> > > +	unsigned short numValid;
> > > +	unsigned short num;
> > 
> > This would never work anyway (hint "short" is not a valid uapi data
> > type...)
> 
> I am surprised about this as gsm_netconfig already uses unsigned short.

How does netconfig pass data to/from userspace?  Through ioctls or
netlink?

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5e516f5cac5a..fafef4edea4f 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -228,7 +228,7 @@  struct gsm_mux {
 	struct tty_struct *tty;		/* The tty our ldisc is bound to */
 	spinlock_t lock;
 	struct mutex mutex;
-	unsigned int num;
+	unsigned short num;
 	struct kref ref;
 
 	/* Events on the GSM channel */
@@ -519,6 +519,64 @@  static void gsm_hex_dump_bytes(const char *fname, const u8 *data,
 	kfree(prefix);
 }
 
+/**
+ * gsm_get_mux	-	find a mux instance by line number
+ *
+ * @num:    the mux line number to search for may differ from the position
+ *          in the mux array.
+ *
+ * @return: Pointer of the mux instance with index <index> or
+ *          NULL if no instance with that index exists
+ */
+static struct gsm_mux *gsm_get_mux(unsigned int num)
+{
+	unsigned int i;
+	struct gsm_mux *gsm;
+
+	/* find the object with that line number */
+	spin_lock(&gsm_mux_lock);
+	for (i = 0, gsm = NULL; i < MAX_MUX; i++) {
+		if (gsm_mux[i]) {
+			if (num == gsm_mux[i]->num) {
+				gsm = gsm_mux[i];
+				break;
+			}
+		}
+	}
+	spin_unlock(&gsm_mux_lock);
+	return gsm;
+}
+
+/**
+ * gsm_assign_num	-	assigns a new mux line number
+ *
+ * @gsm: GSM mux
+ * @num: the desired new mux line number
+ *
+ * The global GSM mux line table is modified to reflect this change.
+ */
+static int gsm_assign_num(struct gsm_mux *gsm, unsigned int num)
+{
+	int ret = 0;
+
+	if (gsm->num >= MAX_MUX || num >= MAX_MUX)
+		return -EINVAL;
+
+	spin_lock(&gsm_mux_lock);
+	if (!gsm_mux[num]) {
+		if (gsm_mux[gsm->num])
+			gsm_mux[gsm->num] = NULL;
+		gsm_mux[num] = gsm;
+		gsm->num = num;
+	} else if (gsm->num != num) {
+		/* Setting an occupied line number is not allowed. */
+		ret = -EBUSY;
+	}
+	spin_unlock(&gsm_mux_lock);
+
+	return ret;
+}
+
 /**
  *	gsm_register_devices	-	register all tty devices for a given mux index
  *
@@ -2700,6 +2758,8 @@  static void gsm_copy_config_values(struct gsm_mux *gsm,
 	c->mru = gsm->mru;
 	c->mtu = gsm->mtu;
 	c->k = 0;
+	c->numValid = 1;
+	c->num = gsm->num;
 }
 
 static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
@@ -2707,6 +2767,7 @@  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	int ret = 0;
 	int need_close = 0;
 	int need_restart = 0;
+	struct gsm_mux *found;
 
 	/* Stuff we don't support yet - UI or I frame transport, windowing */
 	if ((c->adaption != 1 && c->adaption != 2) || c->k)
@@ -2722,10 +2783,22 @@  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 		return -EINVAL;
 	if (c->i == 0 || c->i > 2)	/* UIH and UI only */
 		return -EINVAL;
+	if (c->numValid) {
+		if (c->num >= MAX_MUX)
+			return -EINVAL;
+		found = gsm_get_mux(c->num);
+		if (found) {
+			/* If there is a mux with that number then it must be the same. */
+			if (found != gsm)
+				return -EBUSY;
+		}
+	}
 	/*
 	 * See what is needed for reconfiguration
 	 */
-
+	/* Line number */
+	if (c->numValid && gsm->num != c->num)
+		need_restart = true;
 	/* Timing fields */
 	if (c->t1 != 0 && c->t1 != gsm->t1)
 		need_restart = 1;
@@ -2748,8 +2821,14 @@  static int gsm_config(struct gsm_mux *gsm, struct gsm_config *c)
 	 * configuration. On the first time there is no DLCI[0]
 	 * and closing or cleaning up is not necessary.
 	 */
-	if (need_close || need_restart)
+	if (need_close || need_restart) {
 		gsm_cleanup_mux(gsm, true);
+		if (c->numValid) {
+			ret = gsm_assign_num(gsm, c->num);
+			if (ret)
+				return ret;
+		}
+	}
 
 	gsm->initiator = c->initiator;
 	gsm->mru = c->mru;
diff --git a/include/uapi/linux/gsmmux.h b/include/uapi/linux/gsmmux.h
index cb8693b39cb7..03b28a0076d8 100644
--- a/include/uapi/linux/gsmmux.h
+++ b/include/uapi/linux/gsmmux.h
@@ -19,7 +19,9 @@  struct gsm_config
 	unsigned int mtu;
 	unsigned int k;
 	unsigned int i;
-	unsigned int unused[8];		/* Padding for expansion without
+	unsigned short numValid;
+	unsigned short num;
+	unsigned int unused[7];		/* Padding for expansion without
 					   breaking stuff */
 };