staging: speakup: support more than ttyS*

The function converts strings like ttyS0 and ttyUSB0 to dev_t like (4, 64) and (188, 0). Subsequent patch in this set will call it to convert user-supplied device into device number. The function does some basic sanity checks on the string passed in. It currently supports ttyS*, ttyUSB* and, for selected synths, lp*.

In order to do this, the patch also introduces a string member variable named 'dev' to struct spk_synth. 'dev' represents the device name - ttyUSB0 etc - which needs conversion to dev_t.

Signed-off-by Okash Khawaja <[email protected]>

Reviewed-by Samuel Thibault <[email protected]>

---
 drivers/staging/speakup/spk_priv.h  |    2 
 drivers/staging/speakup/spk_ttyio.c |  105 ++++++++++++++++++++++++++++++++++++
 drivers/staging/speakup/spk_types.h |    1 
 3 files changed, 108 insertions(+)

drivers/staging/speakup/spk_priv.h +2 -0

@@ -40,6 +40,8 @@
 
 #define KT_SPKUP 15
 #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
 
 const struct old_serial_port *spk_serial_init(int index);
 void spk_stop_serial_interrupt(void);

drivers/staging/speakup/spk_ttyio.c +7 -0

@@ -7,6 +7,13 @@
 #include "spk_types.h"
 #include "spk_priv.h"
 
+/* Supported device types */
+#define DEV_PREFIX_TTYS "ttyS"
+#define DEV_PREFIX_TTYUSB "ttyUSB"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
 struct spk_ldisc_data {
 	char buf;
 	struct semaphore sem;

@@ -16,6 +23,104 @@ struct spk_ldisc_data {
 static struct spk_synth *spk_ttyio_synth;

drivers/staging/speakup/spk_ttyio.c +8 -0

+                        return -EINVAL;
+                }
+        }
+
+        if (maj == -1 || min == -1)
+                return -EINVAL;
+
+        /* if here, maj and min must be valid */

drivers/staging/speakup/spk_ttyio.c +37 -0

+
+		return -EINVAL;
+        }
+
+	*dev_no = MKDEV(4, (64 + ser));
+	return 0;
+}
+
+static int get_dev_to_use(struct spk_synth *synth, dev_t *dev_no)
+{
+	/* use ser only when dev is not specified */
+	if (strcmp(synth->dev, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) {
+		/* for /dev/lp* check if synth is supported */
+		if (strncmp(synth->dev, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
+			int i;
+
+			for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
+				if (strcmp(synth->name, lp_supported[i]) == 0)
+					break;
+			}
+
+			if (i >= ARRAY_SIZE(lp_supported)) {
+				pr_err("speakup: lp* is only supported on:");
+				for (i = 0; i < ARRAY_SIZE(lp_supported); i++)
+					pr_cont(" %s", lp_supported[i]);
+				pr_cont("\n");
+
+				return -ENOTSUPP;
+			}
+		}
+
+		return name_to_dev(synth->dev, dev_no);
+	}
+
+	return ser_to_dev(synth->ser, dev_no);
+}
+
 static int spk_ttyio_ldisc_open(struct tty_struct *tty)
 {
 	struct spk_ldisc_data *ldisc_data;

The function converts strings like ttyS0 and ttyUSB0 to dev_t like (4, 64) and (188, 0). Subsequent patch in this set will call it to convert user-supplied device into device number. The function does some basic sanity checks on the string passed in. It currently supports ttyS*, ttyUSB* and, for selected synths, lp*.

In order to do this, the patch also introduces a string member variable named 'dev' to struct spk_synth. 'dev' represents the device name - ttyUSB0 etc - which needs conversion to dev_t.

Signed-off-by Okash Khawaja <[email protected]>

Reviewed-by Samuel Thibault <[email protected]>

---
 drivers/staging/speakup/spk_priv.h  |    2 
 drivers/staging/speakup/spk_ttyio.c |  105 ++++++++++++++++++++++++++++++++++++
 drivers/staging/speakup/spk_types.h |    1 
 3 files changed, 108 insertions(+)

drivers/staging/speakup/spk_priv.h +2 -0

@@ -40,6 +40,8 @@
 
 #define KT_SPKUP 15
 #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
 
 const struct old_serial_port *spk_serial_init(int index);
 void spk_stop_serial_interrupt(void);

drivers/staging/speakup/spk_ttyio.c +52 -0

@@ -7,6 +7,13 @@
 #include "spk_types.h"
 #include "spk_priv.h"
 
+/* Supported device types */
+#define DEV_PREFIX_TTYS "ttyS"
+#define DEV_PREFIX_TTYUSB "ttyUSB"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
 struct spk_ldisc_data {
 	char buf;
 	struct semaphore sem;

@@ -16,6 +23,104 @@ struct spk_ldisc_data {
 static struct spk_synth *spk_ttyio_synth;
 static struct tty_struct *speakup_tty;
 
+static int name_to_dev(const char *name, dev_t *dev_no)
+{
+        int maj = -1, min = -1;
+
+        if (strncmp(name, DEV_PREFIX_TTYS, strlen(DEV_PREFIX_TTYS)) == 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_TTYS), 10, &min)) {
+			pr_err("speakup: Invalid ser param. Must be \
+					between 0 and 191 inclusive.\n");
+                        return -EINVAL;
+                }
+                maj = 4;
+
+                if (min < 0 || min > 191) {
+			pr_err("speakup: Invalid ser param. Must be \
+					between 0 and 191 inclusive.\n");
+                        return -EINVAL;
+                }
+                min = min + 64;
+        } else if (strncmp(name, DEV_PREFIX_TTYUSB, strlen(DEV_PREFIX_TTYUSB))
+			== 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_TTYUSB), 10, &min)) {
+                        pr_err("speakup: Invalid ttyUSB number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+                maj = 188;
+
+                if (min < 0) {
+                        pr_err("speakup: Invalid ttyUSB number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+        } else if (strncmp(name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_LP), 10, &min)) {
+                        pr_warn("speakup: Invalid lp number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+                maj = 6;
+
+                if (min < 0) {
+                        pr_warn("speakup: Invalid lp number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }

Quoted 95 lines · On Tue, Jun 13, 2017 at 11:37:03PM +0100, [email protected] wrote:

The function converts strings like ttyS0 and ttyUSB0 to dev_t like (4, 64) and (188, 0). Subsequent patch in this set will call it to convert user-supplied device into device number. The function does some basic sanity checks on the string passed in. It currently supports ttyS*, ttyUSB* and, for selected synths, lp*.

In order to do this, the patch also introduces a string member variable named 'dev' to struct spk_synth. 'dev' represents the device name - ttyUSB0 etc - which needs conversion to dev_t.

Signed-off-by Okash Khawaja <[email protected]>

Reviewed-by Samuel Thibault <[email protected]>

---
 drivers/staging/speakup/spk_priv.h  |    2 
 drivers/staging/speakup/spk_ttyio.c |  105 ++++++++++++++++++++++++++++++++++++
 drivers/staging/speakup/spk_types.h |    1 
 3 files changed, 108 insertions(+)

drivers/staging/speakup/spk_priv.h +2 -0

@@ -40,6 +40,8 @@
 
 #define KT_SPKUP 15
 #define SPK_SYNTH_TIMEOUT 100000 /* in micro-seconds */
+#define SYNTH_DEFAULT_DEV "ttyS0"
+#define SYNTH_DEFAULT_SER 0
 
 const struct old_serial_port *spk_serial_init(int index);
 void spk_stop_serial_interrupt(void);

drivers/staging/speakup/spk_ttyio.c +52 -0

@@ -7,6 +7,13 @@
 #include "spk_types.h"
 #include "spk_priv.h"
 
+/* Supported device types */
+#define DEV_PREFIX_TTYS "ttyS"
+#define DEV_PREFIX_TTYUSB "ttyUSB"
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
 struct spk_ldisc_data {
 	char buf;
 	struct semaphore sem;

@@ -16,6 +23,104 @@ struct spk_ldisc_data {
 static struct spk_synth *spk_ttyio_synth;
 static struct tty_struct *speakup_tty;
 
+static int name_to_dev(const char *name, dev_t *dev_no)
+{
+        int maj = -1, min = -1;
+
+        if (strncmp(name, DEV_PREFIX_TTYS, strlen(DEV_PREFIX_TTYS)) == 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_TTYS), 10, &min)) {
+			pr_err("speakup: Invalid ser param. Must be \
+					between 0 and 191 inclusive.\n");
+                        return -EINVAL;
+                }
+                maj = 4;
+
+                if (min < 0 || min > 191) {
+			pr_err("speakup: Invalid ser param. Must be \
+					between 0 and 191 inclusive.\n");
+                        return -EINVAL;
+                }
+                min = min + 64;
+        } else if (strncmp(name, DEV_PREFIX_TTYUSB, strlen(DEV_PREFIX_TTYUSB))
+			== 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_TTYUSB), 10, &min)) {
+                        pr_err("speakup: Invalid ttyUSB number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+                maj = 188;
+
+                if (min < 0) {
+                        pr_err("speakup: Invalid ttyUSB number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+        } else if (strncmp(name, DEV_PREFIX_LP, strlen(DEV_PREFIX_LP)) == 0) {
+                if (kstrtoint(name + strlen(DEV_PREFIX_LP), 10, &min)) {
+                        pr_warn("speakup: Invalid lp number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }
+                maj = 6;
+
+                if (min < 0) {
+                        pr_warn("speakup: Invalid lp number. \
+					Must be a number from 0 onwards\n");
+                        return -EINVAL;
+                }

drivers/staging/speakup/spk_ttyio.c struct spk_ldisc_data { · line 80

Quoted 10 lines

drivers/staging/speakup/spk_ttyio.c +10 -0

+        }
+
+        if (maj == -1 || min == -1)
+                return -EINVAL;
+
+        /* if here, maj and min must be valid */
+        *dev_no = MKDEV(maj, min);
+
+        return 0;
+}

Eeek, no, let's never try to parse strings like this and "figure out" what major/minor number it is. That's madness and will break if we ever make all char majors dynamic (there's a thread on lkml about that.)

Why would the kernel need to know major/minor? Is it going to open a device node? If so, again, crazy stuff, that's not good...

Ah, no, nevermind, you just need the major/minor. So why not just take that as the input here, and not a string?

Quoted 2 lines · Greg KH, on mer. 14 juin 2017 12:15:41 +0200, wrote:

Ah, no, nevermind, you just need the major/minor. So why not just take that as the input here, and not a string?

Because real users don't know about major/minor numbers.

Quoted 5 lines · Samuel Thibault

Quoted 2 lines · Greg KH, on mer. 14 juin 2017 12:15:41 +0200, wrote:

Ah, no, nevermind, you just need the major/minor. So why not just take that as the input here, and not a string?

Because real users don't know about major/minor numbers.

I'm not disagreeing, it's just that the kernel doesn't know about "device names" either :)

And trying to have the kernel do the mapping based on strings like this is not ok, sorry.

Quoted 5 lines · Greg KH, on mer. 14 juin 2017 13:48:02 +0200, wrote:

Quoted 1 line · Samuel Thibault

Greg KH, on mer. 14 juin 2017 12:15:41 +0200, wrote:

And trying to have the kernel do the mapping based on strings like this is not ok, sorry.

So what is the solution? Users would find it completely crazy to have to provide major/minor numbers.

Quoted 3 lines · Samuel Thibault

Quoted 2 lines · Greg KH, on mer. 14 juin 2017 13:48:02 +0200, wrote:

Quoted 1 line · Samuel Thibault

Greg KH, on mer. 14 juin 2017 12:15:41 +0200, wrote:

Quoted 5 lines

Quoted 2 lines

And trying to have the kernel do the mapping based on strings like this is not ok, sorry.

So what is the solution? Users would find it completely crazy to have to provide major/minor numbers.

Shouldn't udev take care of major/minor and alike stuff in user space?

Quoted 3 lines · Samuel Thibault

Quoted 2 lines · Greg KH, on mer. 14 juin 2017 13:48:02 +0200, wrote:

Quoted 1 line · Samuel Thibault

Greg KH, on mer. 14 juin 2017 12:15:41 +0200, wrote:

Quoted 5 lines

Quoted 2 lines

And trying to have the kernel do the mapping based on strings like this is not ok, sorry.

So what is the solution? Users would find it completely crazy to have to provide major/minor numbers.

Shouldn't udev take care of major/minor and alike stuff in user space?

The console stuff is odd though, but that is each driver defining the name for itself, major/minor does not apply. Also, a separate driver is not having to figure this all out. I wonder if we could just add a tty core function for this as it does know the name of everything that has been registered with it, right?

Could you call it "dev_name" instead? I normally expect "dev" to be a device struct.

Quoted 5 lines · Greg Kroah-Hartman

The console stuff is odd though, but that is each driver defining the name for itself, major/minor does not apply. Also, a separate driver is not having to figure this all out. I wonder if we could just add a tty core function for this as it does know the name of everything that has been registered with it, right?

I can start working on a patch for this. I'm still new to the tty code so will follow up with any questions I might have.

Hi,

Quoted 7 lines · Okash Khawaja

Quoted 5 lines · Greg Kroah-Hartman

The console stuff is odd though, but that is each driver defining the name for itself, major/minor does not apply. Also, a separate driver is not having to figure this all out. I wonder if we could just add a tty core function for this as it does know the name of everything that has been registered with it, right?

Quoted 2 lines

I can start working on a patch for this. I'm still new to the tty code so will follow up with any questions I might have.

I've put together this function for converting device name to number. It traverses tty_drivers list looking for corresponding dev name and index. Is this the right approach?

Quoted 10 lines · Okash Khawaja

Hi,

Quoted 7 lines · Okash Khawaja

Quoted 5 lines · Greg Kroah-Hartman

The console stuff is odd though, but that is each driver defining the name for itself, major/minor does not apply. Also, a separate driver is not having to figure this all out. I wonder if we could just add a tty core function for this as it does know the name of everything that has been registered with it, right?

Quoted 6 lines

Quoted 2 lines

I can start working on a patch for this. I'm still new to the tty code so will follow up with any questions I might have.

I've put together this function for converting device name to number. It traverses tty_drivers list looking for corresponding dev name and index. Is this the right approach?

Yes, this looks great, nice job! It is a lot simpler than your previous function, and now you are not limited to just a subset off the different serial ports in the system.

Keep up the good work, it's really appreciated.