[patch v2 0/3] staging: speakup: support more than ttyS*

[patch v2 0/3] staging: speakup: support more than ttyS*

[Okash Khawaja]

Hi,

The patchset now contains a separate patch for dev name-to-number
conversion functionality inside tty_io.c.

These patches extend speakup support to ttyUSB* and lp*. They introduce
a new module param dev whose function is similar to ser but instead of
taking serial port number as argument, it takes strings like ttyS0 or
ttyUSB0. 

Patch 1 adds functionality to convert such strings into dev_t. 
Patch 2 uses the function in patch 1 and performs some checks.
Patch 3 adds new module param and actually extends support to more than
ttyS*.

Samuel, I have kept the Reviewed-by for patch 3 which has slightly 
changed from before - 'dev' renamed to 'dev_name' - but not for patch 2
which is a bit different now that dev name to number conversion has
moved to tty_io.c. Please let me know if you think otherwise.

Thanks,
Okash

[patch v2 1/3] tty: add function to convert device name to number

[Okash Khawaja]

[-- Attachment #1: 11_add_tty_dev_name_to_number --]
[-- Type: text/plain, Size: 2636 bytes --]

The function converts strings like ttyS0 and ttyUSB0 to dev_t like
(4, 64) and (188, 0). It does this by scanning tty_drivers list for
corresponding device name and index. If the driver is not registered,
this function returns -ENODEV. It also acquires tty_mutex.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

---
 drivers/tty/tty_io.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/tty.h  |    4 ++++
 2 files changed, 49 insertions(+)

--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -325,6 +325,51 @@ static struct tty_driver *get_tty_driver
 	return NULL;
 }
 
+/**
+ *	tty_dev_name_to_number	-	return dev_t for device name
+ *	@device_name: user space name of device under /dev
+ *	@dev_no: pointer to dev_t that this function will populate
+ *
+ *	This function converts device names like ttyS0 or ttyUSB1 into dev_t
+ *	like (4, 64) or (188, 1). If no corresponding driver is registered then
+ *	the function returns -ENODEV.
+ *
+ *	Locking: this acquires tty_mutex
+ */
+int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{
+	struct tty_driver *p;
+	int rv, index, prefix_length = 0;
+
+	while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
+			strlen(dev_name) )
+		prefix_length++;
+
+	if (prefix_length == strlen(dev_name))
+		return -EINVAL;
+
+	mutex_lock(&tty_mutex);
+
+	list_for_each_entry(p, &tty_drivers, tty_drivers)
+		if (prefix_length == strlen(p->name) && strncmp(dev_name,
+					p->name, prefix_length) == 0) {
+			rv = kstrtoint(dev_name + prefix_length, 10, &index);
+			if (rv) {
+				mutex_unlock(&tty_mutex);
+				return rv;
+			}
+			if (index < p->num) {
+				*dev_no = MKDEV(p->major, p->minor_start + index);
+				mutex_unlock(&tty_mutex);
+				return 0;
+			}
+		}
+
+	mutex_unlock(&tty_mutex);
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
+
 #ifdef CONFIG_CONSOLE_POLL
 
 /**
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -401,6 +401,7 @@ extern int __init tty_init(void);
 extern const char *tty_name(const struct tty_struct *tty);
 extern struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
 		struct file *filp);
+extern int tty_dev_name_to_number(char *dev_name, dev_t *dev_no);
 #else
 static inline void tty_kref_put(struct tty_struct *tty)
 { }
@@ -424,6 +425,9 @@ static inline const char *tty_name(const
 static inline struct tty_struct *tty_open_by_driver(dev_t device,
 		struct inode *inode, struct file *filp)
 { return NULL; }
+static inline int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
+{ return -ENOTSUPP; }
+
 #endif
 
 extern struct ktermios tty_std_termios;

[patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Okash Khawaja]

[-- Attachment #1: 12_check_and_convert_dev_or_ser_to_number --]
[-- Type: text/plain, Size: 3051 bytes --]

This patch adds functionality to validate and convert either a device
name or 'ser' member of synth into dev_t. Subsequent patch in this set
will call it to convert user-specified device into device number. For
device name, this patch does some basic sanity checks on the string
passed in. It currently supports ttyS*, ttyUSB* and, for selected
synths, lp*.

The patch also introduces a string member variable named 'dev_name' to
struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
which needs conversion to dev_t.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>

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

--- a/drivers/staging/speakup/spk_priv.h
+++ b/drivers/staging/speakup/spk_priv.h
@@ -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);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -7,6 +7,10 @@
 #include "spk_types.h"
 #include "spk_priv.h"
 
+#define DEV_PREFIX_LP "lp"
+
+const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
+
 struct spk_ldisc_data {
 	char buf;
 	struct semaphore sem;
@@ -16,6 +20,48 @@ struct spk_ldisc_data {
 static struct spk_synth *spk_ttyio_synth;
 static struct tty_struct *speakup_tty;
 
+int ser_to_dev(int ser, dev_t *dev_no)
+{
+	if (ser < 0 || ser > (255 - 64)) {
+                pr_err("speakup: Invalid ser param. \
+				Must be between 0 and 191 inclusive.\n");
+
+		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_name, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) {
+		/* for /dev/lp* check if synth is supported */
+		if (strncmp(synth->dev_name, 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 tty_dev_name_to_number(synth->dev_name, 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;
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -169,6 +169,7 @@ struct spk_synth {
 	int jiffies;
 	int full;
 	int ser;
+	char *dev_name;
 	short flags;
 	short startup;
 	const int checkval; /* for validating a proper synth module */

[patch v2 3/3] staging: speakup: make ttyio synths use device name

[Okash Khawaja]

[-- Attachment #1: 13_make_ttyio_use_device_name --]
[-- Type: text/plain, Size: 10736 bytes --]

This patch introduces new module parameter, dev, which takes a string
representing the device that the external synth is connected to, e.g.
ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
That way, speakup can support more than ttyS*. As of this patch, it
only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
is only available for tty-migrated synths.

Users will either use dev or ser as both serve same purpose. This patch
maintains backward compatility by allowing ser to be specified. When
both are specified, whichever is non-default, i.e. not ttyS0, is used.
If both are non-default then dev is used.

Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

---
 drivers/staging/speakup/speakup_acntsa.c |    3 +++
 drivers/staging/speakup/speakup_apollo.c |    3 +++
 drivers/staging/speakup/speakup_audptr.c |    3 +++
 drivers/staging/speakup/speakup_bns.c    |    3 +++
 drivers/staging/speakup/speakup_decext.c |    3 +++
 drivers/staging/speakup/speakup_dectlk.c |    3 +++
 drivers/staging/speakup/speakup_dummy.c  |    3 +++
 drivers/staging/speakup/speakup_ltlk.c   |    3 +++
 drivers/staging/speakup/speakup_spkout.c |    3 +++
 drivers/staging/speakup/speakup_txprt.c  |    3 +++
 drivers/staging/speakup/spk_ttyio.c      |   15 +++++++--------
 11 files changed, 37 insertions(+), 8 deletions(-)

--- a/drivers/staging/speakup/speakup_acntsa.c
+++ b/drivers/staging/speakup/speakup_acntsa.c
@@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
 	.trigger = 50,
 	.jiffies = 30,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
 }
 
 module_param_named(ser, synth_acntsa.ser, int, 0444);
+module_param_named(dev, synth_acntsa.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_acntsa.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_acntsa);
--- a/drivers/staging/speakup/speakup_apollo.c
+++ b/drivers/staging/speakup/speakup_apollo.c
@@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
 }
 
 module_param_named(ser, synth_apollo.ser, int, 0444);
+module_param_named(dev, synth_apollo.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_apollo.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_apollo);
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
 	.trigger = 50,
 	.jiffies = 30,
 	.full = 18000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
 }
 
 module_param_named(ser, synth_audptr.ser, int, 0444);
+module_param_named(dev, synth_audptr.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_audptr.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_audptr);
--- a/drivers/staging/speakup/speakup_bns.c
+++ b/drivers/staging/speakup/speakup_bns.c
@@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
 };
 
 module_param_named(ser, synth_bns.ser, int, 0444);
+module_param_named(dev, synth_bns.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_bns.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_bns);
--- a/drivers/staging/speakup/speakup_decext.c
+++ b/drivers/staging/speakup/speakup_decext.c
@@ -120,6 +120,7 @@ static struct spk_synth synth_decext = {
 	.jiffies = 50,
 	.full = 40000,
 	.flags = SF_DEC,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -226,9 +227,11 @@ static void synth_flush(struct spk_synth
 }
 
 module_param_named(ser, synth_decext.ser, int, 0444);
+module_param_named(dev, synth_decext.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_decext.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_decext);
--- a/drivers/staging/speakup/speakup_dectlk.c
+++ b/drivers/staging/speakup/speakup_dectlk.c
@@ -124,6 +124,7 @@ static struct spk_synth synth_dectlk = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -298,9 +299,11 @@ static void synth_flush(struct spk_synth
 }
 
 module_param_named(ser, synth_dectlk.ser, int, 0444);
+module_param_named(dev, synth_dectlk.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_dectlk.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_dectlk);
--- a/drivers/staging/speakup/speakup_dummy.c
+++ b/drivers/staging/speakup/speakup_dummy.c
@@ -95,6 +95,7 @@ static struct spk_synth synth_dummy = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -121,9 +122,11 @@ static struct spk_synth synth_dummy = {
 };
 
 module_param_named(ser, synth_dummy.ser, int, 0444);
+module_param_named(dev, synth_dummy.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_dummy.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_dummy);
--- a/drivers/staging/speakup/speakup_ltlk.c
+++ b/drivers/staging/speakup/speakup_ltlk.c
@@ -107,6 +107,7 @@ static struct spk_synth synth_ltlk = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -166,9 +167,11 @@ static int synth_probe(struct spk_synth
 }
 
 module_param_named(ser, synth_ltlk.ser, int, 0444);
+module_param_named(dev, synth_ltlk.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_ltlk.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_ltlk);
--- a/drivers/staging/speakup/speakup_spkout.c
+++ b/drivers/staging/speakup/speakup_spkout.c
@@ -98,6 +98,7 @@ static struct spk_synth synth_spkout = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -130,9 +131,11 @@ static void synth_flush(struct spk_synth
 }
 
 module_param_named(ser, synth_spkout.ser, int, 0444);
+module_param_named(dev, synth_spkout.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_spkout.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_spkout);
--- a/drivers/staging/speakup/speakup_txprt.c
+++ b/drivers/staging/speakup/speakup_txprt.c
@@ -92,6 +92,7 @@ static struct spk_synth synth_txprt = {
 	.trigger = 50,
 	.jiffies = 50,
 	.full = 40000,
+	.dev_name = SYNTH_DEFAULT_DEV,
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
@@ -118,9 +119,11 @@ static struct spk_synth synth_txprt = {
 };
 
 module_param_named(ser, synth_txprt.ser, int, 0444);
+module_param_named(dev, synth_txprt.dev_name, charp, S_IRUGO);
 module_param_named(start, synth_txprt.startup, short, 0444);
 
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
+MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
 module_spk_synth(synth_txprt);
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -151,11 +151,12 @@ static inline void get_termios(struct tt
 	up_read(&tty->termios_rwsem);
 }
 
-static int spk_ttyio_initialise_ldisc(int ser)
+static int spk_ttyio_initialise_ldisc(struct spk_synth *synth)
 {
 	int ret = 0;
 	struct tty_struct *tty;
 	struct ktermios tmp_termios;
+	dev_t dev;
 
 	ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
 	if (ret) {
@@ -163,13 +164,11 @@ static int spk_ttyio_initialise_ldisc(in
 		return ret;
 	}
 
-	if (ser < 0 || ser > (255 - 64)) {
-		pr_err("speakup: Invalid ser param. Must be between 0 and 191 inclusive.\n");
-		return -EINVAL;
-	}
+	ret = get_dev_to_use(synth, &dev);
+	if (ret)
+		return ret;
 
-	/* TODO: support more than ttyS* */
-	tty = tty_open_by_driver(MKDEV(4, (ser +  64)), NULL, NULL);
+	tty = tty_open_by_driver(dev, NULL, NULL);
 	if (IS_ERR(tty))
 		return PTR_ERR(tty);
 
@@ -281,7 +280,7 @@ static void spk_ttyio_flush_buffer(void)
 
 int spk_ttyio_synth_probe(struct spk_synth *synth)
 {
-	int rv = spk_ttyio_initialise_ldisc(synth->ser);
+	int rv = spk_ttyio_initialise_ldisc(synth);
 
 	if (rv)
 		return rv;

Re: [patch v2 1/3] tty: add function to convert device name to number

[Andy Shevchenko]

On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> The function converts strings like ttyS0 and ttyUSB0 to dev_t like
> (4, 64) and (188, 0). It does this by scanning tty_drivers list for
> corresponding device name and index. If the driver is not registered,
> this function returns -ENODEV. It also acquires tty_mutex.

Nice!
Few comments below.

> +/**
> + *     tty_dev_name_to_number  -       return dev_t for device name

> + *     @device_name: user space name of device under /dev

This doesn't have actual parameter name.
Btw, I would drop dev_ suffix completely from parameter (you have it
in function name).

> + *     @dev_no: pointer to dev_t that this function will populate

Here I'm not sure if dev_ prefix is good to have or not.

> + *
> + *     This function converts device names like ttyS0 or ttyUSB1 into dev_t
> + *     like (4, 64) or (188, 1). If no corresponding driver is registered then
> + *     the function returns -ENODEV.
> + *

> + *     Locking: this acquires tty_mutex

...and releases.

Perhaps it makes sense to describe what it protects (like it's done in
some functions around).

> + */
> +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)

const char *name, right?

> +{
> +       struct tty_driver *p;

> +       int rv, index, prefix_length = 0;

I would keep returned variable on a separate line and name it like
other functions do in this file, i.e. ret.

int ret;

> +       while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
> +                       strlen(dev_name) )
> +               prefix_length++;
> +
> +       if (prefix_length == strlen(dev_name))
> +               return -EINVAL;

Basically, what you need is to get tailing digits, right?

Moreover, there is quite similar piece of code in
tty_find_polling_driver() you may share.

> +       mutex_lock(&tty_mutex);
> +
> +       list_for_each_entry(p, &tty_drivers, tty_drivers)
> +               if (prefix_length == strlen(p->name) && strncmp(dev_name,
> +                                       p->name, prefix_length) == 0) {

> +                       rv = kstrtoint(dev_name + prefix_length, 10, &index);
> +                       if (rv) {

> +                               mutex_unlock(&tty_mutex);
> +                               return rv;

I would go with goto style in this function (since it has locking involved).

> +                       }

All together kstrtoint() is invariant here as far as I can see and can
be done out of locking. Also see above comment how to get line index.

> +                       if (index < p->num) {
> +                               *dev_no = MKDEV(p->major, p->minor_start + index);
> +                               mutex_unlock(&tty_mutex);
> +                               return 0;
> +                       }
> +               }
> +
> +       mutex_unlock(&tty_mutex);
> +       return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(tty_dev_name_to_number);

-- 
With Best Regards,
Andy Shevchenko

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Andy Shevchenko]

On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> This patch adds functionality to validate and convert either a device
> name or 'ser' member of synth into dev_t. Subsequent patch in this set
> will call it to convert user-specified device into device number. For
> device name, this patch does some basic sanity checks on the string
> passed in. It currently supports ttyS*, ttyUSB* and, for selected
> synths, lp*.
>
> The patch also introduces a string member variable named 'dev_name' to
> struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
> which needs conversion to dev_t.

Looks fine. Few minor comments below, and take my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
>
> ---
>  drivers/staging/speakup/spk_priv.h  |    2 +
>  drivers/staging/speakup/spk_ttyio.c |   46 ++++++++++++++++++++++++++++++++++++
>  drivers/staging/speakup/spk_types.h |    1
>  3 files changed, 49 insertions(+)
>
> --- a/drivers/staging/speakup/spk_priv.h
> +++ b/drivers/staging/speakup/spk_priv.h
> @@ -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);
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -7,6 +7,10 @@
>  #include "spk_types.h"
>  #include "spk_priv.h"
>
> +#define DEV_PREFIX_LP "lp"
> +

> +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };

static ?

> +
>  struct spk_ldisc_data {
>         char buf;
>         struct semaphore sem;
> @@ -16,6 +20,48 @@ struct spk_ldisc_data {
>  static struct spk_synth *spk_ttyio_synth;
>  static struct tty_struct *speakup_tty;
>
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> +       if (ser < 0 || ser > (255 - 64)) {

> +                pr_err("speakup: Invalid ser param. \
> +                               Must be between 0 and 191 inclusive.\n");

Just make it one line.

> +
> +               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_name, SYNTH_DEFAULT_DEV) || synth->ser == SYNTH_DEFAULT_SER) {
> +               /* for /dev/lp* check if synth is supported */
> +               if (strncmp(synth->dev_name, 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)) {

match_string()

> +                               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");

pr_cont() is not the best idea, though I think it will be rare cases
when it might be broken in pieces.

> +
> +                               return -ENOTSUPP;
> +                       }
> +               }
> +
> +               return tty_dev_name_to_number(synth->dev_name, 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;
> --- a/drivers/staging/speakup/spk_types.h
> +++ b/drivers/staging/speakup/spk_types.h
> @@ -169,6 +169,7 @@ struct spk_synth {
>         int jiffies;
>         int full;
>         int ser;

> +       char *dev_name;

const ?

>         short flags;
>         short startup;
>         const int checkval; /* for validating a proper synth module */
>



-- 
With Best Regards,
Andy Shevchenko

Re: [patch v2 3/3] staging: speakup: make ttyio synths use device name

[Andy Shevchenko]

On Sun, Jun 18, 2017 at 11:58 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> This patch introduces new module parameter, dev, which takes a string
> representing the device that the external synth is connected to, e.g.
> ttyS0, ttyUSB0 etc. This is then used to communicate with the synth.
> That way, speakup can support more than ttyS*. As of this patch, it
> only supports ttyS*, ttyUSB* and selected synths for lp*. dev parameter
> is only available for tty-migrated synths.
>
> Users will either use dev or ser as both serve same purpose. This patch
> maintains backward compatility by allowing ser to be specified. When
> both are specified, whichever is non-default, i.e. not ttyS0, is used.
> If both are non-default then dev is used.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

...with an assumption Greg is okay with new module parameter here.

> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> ---
>  drivers/staging/speakup/speakup_acntsa.c |    3 +++
>  drivers/staging/speakup/speakup_apollo.c |    3 +++
>  drivers/staging/speakup/speakup_audptr.c |    3 +++
>  drivers/staging/speakup/speakup_bns.c    |    3 +++
>  drivers/staging/speakup/speakup_decext.c |    3 +++
>  drivers/staging/speakup/speakup_dectlk.c |    3 +++
>  drivers/staging/speakup/speakup_dummy.c  |    3 +++
>  drivers/staging/speakup/speakup_ltlk.c   |    3 +++
>  drivers/staging/speakup/speakup_spkout.c |    3 +++
>  drivers/staging/speakup/speakup_txprt.c  |    3 +++
>  drivers/staging/speakup/spk_ttyio.c      |   15 +++++++--------
>  11 files changed, 37 insertions(+), 8 deletions(-)
>
> --- a/drivers/staging/speakup/speakup_acntsa.c
> +++ b/drivers/staging/speakup/speakup_acntsa.c
> @@ -96,6 +96,7 @@ static struct spk_synth synth_acntsa = {
>         .trigger = 50,
>         .jiffies = 30,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -135,9 +136,11 @@ static int synth_probe(struct spk_synth
>  }
>
>  module_param_named(ser, synth_acntsa.ser, int, 0444);
> +module_param_named(dev, synth_acntsa.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_acntsa.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_acntsa);
> --- a/drivers/staging/speakup/speakup_apollo.c
> +++ b/drivers/staging/speakup/speakup_apollo.c
> @@ -105,6 +105,7 @@ static struct spk_synth synth_apollo = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -199,9 +200,11 @@ static void do_catch_up(struct spk_synth
>  }
>
>  module_param_named(ser, synth_apollo.ser, int, 0444);
> +module_param_named(dev, synth_apollo.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_apollo.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_apollo);
> --- a/drivers/staging/speakup/speakup_audptr.c
> +++ b/drivers/staging/speakup/speakup_audptr.c
> @@ -100,6 +100,7 @@ static struct spk_synth synth_audptr = {
>         .trigger = 50,
>         .jiffies = 30,
>         .full = 18000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -162,9 +163,11 @@ static int synth_probe(struct spk_synth
>  }
>
>  module_param_named(ser, synth_audptr.ser, int, 0444);
> +module_param_named(dev, synth_audptr.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_audptr.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_audptr);
> --- a/drivers/staging/speakup/speakup_bns.c
> +++ b/drivers/staging/speakup/speakup_bns.c
> @@ -93,6 +93,7 @@ static struct spk_synth synth_bns = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -119,9 +120,11 @@ static struct spk_synth synth_bns = {
>  };
>
>  module_param_named(ser, synth_bns.ser, int, 0444);
> +module_param_named(dev, synth_bns.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_bns.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_bns);
> --- a/drivers/staging/speakup/speakup_decext.c
> +++ b/drivers/staging/speakup/speakup_decext.c
> @@ -120,6 +120,7 @@ static struct spk_synth synth_decext = {
>         .jiffies = 50,
>         .full = 40000,
>         .flags = SF_DEC,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -226,9 +227,11 @@ static void synth_flush(struct spk_synth
>  }
>
>  module_param_named(ser, synth_decext.ser, int, 0444);
> +module_param_named(dev, synth_decext.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_decext.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_decext);
> --- a/drivers/staging/speakup/speakup_dectlk.c
> +++ b/drivers/staging/speakup/speakup_dectlk.c
> @@ -124,6 +124,7 @@ static struct spk_synth synth_dectlk = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -298,9 +299,11 @@ static void synth_flush(struct spk_synth
>  }
>
>  module_param_named(ser, synth_dectlk.ser, int, 0444);
> +module_param_named(dev, synth_dectlk.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_dectlk.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_dectlk);
> --- a/drivers/staging/speakup/speakup_dummy.c
> +++ b/drivers/staging/speakup/speakup_dummy.c
> @@ -95,6 +95,7 @@ static struct spk_synth synth_dummy = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -121,9 +122,11 @@ static struct spk_synth synth_dummy = {
>  };
>
>  module_param_named(ser, synth_dummy.ser, int, 0444);
> +module_param_named(dev, synth_dummy.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_dummy.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_dummy);
> --- a/drivers/staging/speakup/speakup_ltlk.c
> +++ b/drivers/staging/speakup/speakup_ltlk.c
> @@ -107,6 +107,7 @@ static struct spk_synth synth_ltlk = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -166,9 +167,11 @@ static int synth_probe(struct spk_synth
>  }
>
>  module_param_named(ser, synth_ltlk.ser, int, 0444);
> +module_param_named(dev, synth_ltlk.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_ltlk.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_ltlk);
> --- a/drivers/staging/speakup/speakup_spkout.c
> +++ b/drivers/staging/speakup/speakup_spkout.c
> @@ -98,6 +98,7 @@ static struct spk_synth synth_spkout = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -130,9 +131,11 @@ static void synth_flush(struct spk_synth
>  }
>
>  module_param_named(ser, synth_spkout.ser, int, 0444);
> +module_param_named(dev, synth_spkout.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_spkout.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_spkout);
> --- a/drivers/staging/speakup/speakup_txprt.c
> +++ b/drivers/staging/speakup/speakup_txprt.c
> @@ -92,6 +92,7 @@ static struct spk_synth synth_txprt = {
>         .trigger = 50,
>         .jiffies = 50,
>         .full = 40000,
> +       .dev_name = SYNTH_DEFAULT_DEV,
>         .startup = SYNTH_START,
>         .checkval = SYNTH_CHECK,
>         .vars = vars,
> @@ -118,9 +119,11 @@ static struct spk_synth synth_txprt = {
>  };
>
>  module_param_named(ser, synth_txprt.ser, int, 0444);
> +module_param_named(dev, synth_txprt.dev_name, charp, S_IRUGO);
>  module_param_named(start, synth_txprt.startup, short, 0444);
>
>  MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
> +MODULE_PARM_DESC(dev, "Set the device e.g. ttyUSB0, for the synthesizer.");
>  MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
>
>  module_spk_synth(synth_txprt);
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -151,11 +151,12 @@ static inline void get_termios(struct tt
>         up_read(&tty->termios_rwsem);
>  }
>
> -static int spk_ttyio_initialise_ldisc(int ser)
> +static int spk_ttyio_initialise_ldisc(struct spk_synth *synth)
>  {
>         int ret = 0;
>         struct tty_struct *tty;
>         struct ktermios tmp_termios;
> +       dev_t dev;
>
>         ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
>         if (ret) {
> @@ -163,13 +164,11 @@ static int spk_ttyio_initialise_ldisc(in
>                 return ret;
>         }
>
> -       if (ser < 0 || ser > (255 - 64)) {
> -               pr_err("speakup: Invalid ser param. Must be between 0 and 191 inclusive.\n");
> -               return -EINVAL;
> -       }
> +       ret = get_dev_to_use(synth, &dev);
> +       if (ret)
> +               return ret;
>
> -       /* TODO: support more than ttyS* */
> -       tty = tty_open_by_driver(MKDEV(4, (ser +  64)), NULL, NULL);
> +       tty = tty_open_by_driver(dev, NULL, NULL);
>         if (IS_ERR(tty))
>                 return PTR_ERR(tty);
>
> @@ -281,7 +280,7 @@ static void spk_ttyio_flush_buffer(void)
>
>  int spk_ttyio_synth_probe(struct spk_synth *synth)
>  {
> -       int rv = spk_ttyio_initialise_ldisc(synth->ser);
> +       int rv = spk_ttyio_initialise_ldisc(synth);
>
>         if (rv)
>                 return rv;
>



-- 
With Best Regards,
Andy Shevchenko

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Okash Khawaja]

Hi,

Thanks for the reviews. Couple of things inlined below.

On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
> 
> > +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
> 
> static ?
Sure!

> > +       if (ser < 0 || ser > (255 - 64)) {
> 
> > +                pr_err("speakup: Invalid ser param. \
> > +                               Must be between 0 and 191 inclusive.\n");
> 
> Just make it one line.
Is it okay if it becomes larger than 80 chars?

> > +
> > +                       for (i = 0; i < ARRAY_SIZE(lp_supported); i++) {
> > +                               if (strcmp(synth->name, lp_supported[i]) == 0)
> > +                                       break;
> > +                       }
> > +
> > +                       if (i >= ARRAY_SIZE(lp_supported)) {
> 
> match_string()
Cool, didn't know about it

> 
> > +                               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");
> 
> pr_cont() is not the best idea, though I think it will be rare cases
> when it might be broken in pieces.
Hmm... I would like to keep it if it doesn't incur an overhead. It also
indicates to the reader that this all part of same output line. Let me
know what you think.

> 
> > +
> > +                               return -ENOTSUPP;
> > +                       }
> > +               }
> > +
> > +               return tty_dev_name_to_number(synth->dev_name, 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;
> > --- a/drivers/staging/speakup/spk_types.h
> > +++ b/drivers/staging/speakup/spk_types.h
> > @@ -169,6 +169,7 @@ struct spk_synth {
> >         int jiffies;
> >         int full;
> >         int ser;
> 
> > +       char *dev_name;
> 
> const ?
This becomes the target of module_param in following patch. It complains
when set to const.

Thanks!
Okash

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Andy Shevchenko]

On Sun, Jun 18, 2017 at 8:22 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:

>> > +       if (ser < 0 || ser > (255 - 64)) {
>>
>> > +                pr_err("speakup: Invalid ser param. \
>> > +                               Must be between 0 and 191 inclusive.\n");
>>
>> Just make it one line.
> Is it okay if it becomes larger than 80 chars?

Yes. Even checkpatch will not complain in this case.

>> > +                               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");
>>
>> pr_cont() is not the best idea, though I think it will be rare cases
>> when it might be broken in pieces.

> Hmm... I would like to keep it if it doesn't incur an overhead. It also
> indicates to the reader that this all part of same output line. Let me
> know what you think.

For better user experience you need something like a temporary buffer
and one pr_err(); line at the end.
But as I said here is quite a chance that no one will see this message
broken apart.

>> > --- a/drivers/staging/speakup/spk_types.h
>> > +++ b/drivers/staging/speakup/spk_types.h
>> > @@ -169,6 +169,7 @@ struct spk_synth {
>> >         int jiffies;
>> >         int full;
>> >         int ser;
>>
>> > +       char *dev_name;
>>
>> const ?
> This becomes the target of module_param in following patch. It complains
> when set to const.

Fair enough.

-- 
With Best Regards,
Andy Shevchenko

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Joe Perches]

On Sun, 2017-06-18 at 22:54 +0300, Andy Shevchenko wrote:
> On Sun, Jun 18, 2017 at 8:22 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> > On Sun, Jun 18, 2017 at 04:35:21PM +0300, Andy Shevchenko wrote:
> > > > +       if (ser < 0 || ser > (255 - 64)) {
> > > > +                pr_err("speakup: Invalid ser param. \
> > > > +                               Must be between 0 and 191 inclusive.\n");
> > > 
> > > Just make it one line.
> > 
> > Is it okay if it becomes larger than 80 chars?
> 
> Yes. Even checkpatch will not complain in this case.

And even if it didn't, as written it's a defect
because line continuations don't act like
string concatenations.  You've added a space
before the line continuation \ and a bunch of
whitespace before the word Must

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Greg Kroah-Hartman]

On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> This patch adds functionality to validate and convert either a device
> name or 'ser' member of synth into dev_t. Subsequent patch in this set
> will call it to convert user-specified device into device number. For
> device name, this patch does some basic sanity checks on the string
> passed in. It currently supports ttyS*, ttyUSB* and, for selected
> synths, lp*.
> 
> The patch also introduces a string member variable named 'dev_name' to
> struct spk_synth. 'dev_name' represents the device name - ttyUSB0 etc -
> which needs conversion to dev_t.
> 
> Signed-off-by: Okash Khawaja <okash.khawaja@gmail.com>
> 
> ---
>  drivers/staging/speakup/spk_priv.h  |    2 +
>  drivers/staging/speakup/spk_ttyio.c |   46 ++++++++++++++++++++++++++++++++++++
>  drivers/staging/speakup/spk_types.h |    1 
>  3 files changed, 49 insertions(+)
> 
> --- a/drivers/staging/speakup/spk_priv.h
> +++ b/drivers/staging/speakup/spk_priv.h
> @@ -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);
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -7,6 +7,10 @@
>  #include "spk_types.h"
>  #include "spk_priv.h"
>  
> +#define DEV_PREFIX_LP "lp"
> +
> +const char *lp_supported[] = { "acntsa", "bns", "dummy", "txprt" };
> +
>  struct spk_ldisc_data {
>  	char buf;
>  	struct semaphore sem;
> @@ -16,6 +20,48 @@ struct spk_ldisc_data {
>  static struct spk_synth *spk_ttyio_synth;
>  static struct tty_struct *speakup_tty;
>  
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> +	if (ser < 0 || ser > (255 - 64)) {
> +                pr_err("speakup: Invalid ser param. \
> +				Must be between 0 and 191 inclusive.\n");

As Andy pointed out, never do this for a C string, it's not doing what
you think it is :)

Worse case, do this like the following:
		pr_err("speakup: Invalid ser param."
			"Must be between 0 and 191 inclusive.\n");

Also note, you are using spaces here in the patch, always run
checkpatch.pl on your patches, so you don't get a grumpy maintainer
telling you to run checkpatch.pl on your patches :)

Please fix up and resend the series.

thanks,

greg k-h

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Joe Perches]

On Mon, 2017-06-19 at 09:15 +0800, Greg Kroah-Hartman wrote:
> On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> > This patch adds functionality to validate and convert either a device
> > name or 'ser' member of synth into dev_t.
[]
> > --- a/drivers/staging/speakup/spk_ttyio.c
[]
> > +int ser_to_dev(int ser, dev_t *dev_no)
> > +{
> > +	if (ser < 0 || ser > (255 - 64)) {
> > +                pr_err("speakup: Invalid ser param. \
> > +				Must be between 0 and 191 inclusive.\n");
> 
> As Andy pointed out, never do this for a C string, it's not doing what
> you think it is :)

Well, some guy.

> Worse case, do this like the following:
> 		pr_err("speakup: Invalid ser param."
> 			"Must be between 0 and 191 inclusive.\n");

Nope, now there's no space between param and Must.

Using string concatenation on multiple lines is error prone.

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Greg Kroah-Hartman]

On Sun, Jun 18, 2017 at 10:33:35PM -0700, Joe Perches wrote:
> On Mon, 2017-06-19 at 09:15 +0800, Greg Kroah-Hartman wrote:
> > On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> > > This patch adds functionality to validate and convert either a device
> > > name or 'ser' member of synth into dev_t.
> []
> > > --- a/drivers/staging/speakup/spk_ttyio.c
> []
> > > +int ser_to_dev(int ser, dev_t *dev_no)
> > > +{
> > > +	if (ser < 0 || ser > (255 - 64)) {
> > > +                pr_err("speakup: Invalid ser param. \
> > > +				Must be between 0 and 191 inclusive.\n");
> > 
> > As Andy pointed out, never do this for a C string, it's not doing what
> > you think it is :)
> 
> Well, some guy.
> 
> > Worse case, do this like the following:
> > 		pr_err("speakup: Invalid ser param."
> > 			"Must be between 0 and 191 inclusive.\n");
> 
> Nope, now there's no space between param and Must.
> 
> Using string concatenation on multiple lines is error prone.

Ah, yes it is, see, I messed it up!  :)

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Okash Khawaja]

On Mon, Jun 19, 2017 at 09:15:33AM +0800, Greg Kroah-Hartman wrote:
> > +int ser_to_dev(int ser, dev_t *dev_no)
> > +{
> > +	if (ser < 0 || ser > (255 - 64)) {
> > +                pr_err("speakup: Invalid ser param. \
> > +				Must be between 0 and 191 inclusive.\n");
> 
> As Andy pointed out, never do this for a C string, it's not doing what
> you think it is :)
Of course! I am sorry I should address such issues before submitting.
Will watch out more carefully next time.

> 
> Worse case, do this like the following:
> 		pr_err("speakup: Invalid ser param."
> 			"Must be between 0 and 191 inclusive.\n");
> 
> Also note, you are using spaces here in the patch, always run
> checkpatch.pl on your patches, so you don't get a grumpy maintainer
> telling you to run checkpatch.pl on your patches :)
Sure.

Thanks for the patience.

Okash

Re: [patch v2 1/3] tty: add function to convert device name to number

[Okash Khawaja]

On Sun, Jun 18, 2017 at 04:27:42PM +0300, Andy Shevchenko wrote: 
> This doesn't have actual parameter name.
> Btw, I would drop dev_ suffix completely from parameter (you have it
> in function name).
Good call, thanks.

> > + *     Locking: this acquires tty_mutex
> 
> ...and releases.
> 
> Perhaps it makes sense to describe what it protects (like it's done in
> some functions around).
Yes, will add the description

> > + */
> > +int tty_dev_name_to_number(char *dev_name, dev_t *dev_no)
> 
> const char *name, right?
Yes :)

> > +       int rv, index, prefix_length = 0;
> 
> I would keep returned variable on a separate line and name it like
> other functions do in this file, i.e. ret.
> 
> int ret;
Sure

> > +       while (!isdigit(*(dev_name + prefix_length)) && prefix_length <
> > +                       strlen(dev_name) )
> > +               prefix_length++;
> > +
> > +       if (prefix_length == strlen(dev_name))
> > +               return -EINVAL;
> 
> Basically, what you need is to get tailing digits, right?
> 
> Moreover, there is quite similar piece of code in
> tty_find_polling_driver() you may share.
tty_find_polling_driver does something slightly different. It looks for
digits embedded in a string, so something like kgdboc=ttyS0,115200 where
the digit being sought is 0 after ttyS. So the sanity checks aren't the
same. It also looks for a comma in addition to looking for a number,
which doesn't apply here. Little bit of functionality may be factored
out but that will be too trivial.

While skimming through tty_find_polling_driver, I also noticed that, in
a strict sense, the following check may not be sufficient when name is
prefix of p->name.

if (strncmp(name, p->name, len) != 0)

> > +                       if (rv) {
> 
> > +                               mutex_unlock(&tty_mutex);
> > +                               return rv;
> 
> I would go with goto style in this function (since it has locking involved).
Sure
> 
> > +                       }
> 
> All together kstrtoint() is invariant here as far as I can see and can
> be done out of locking. Also see above comment how to get line index.
Good call, thanks. Think I changed the code afterwards but didn't
notice that kstrtoint no longer needs to be inside the loop and lock.

Best regards,
Okash

Re: [patch v2 2/3] staging: speakup: check and convert dev name or ser to dev_t

[Dan Carpenter]

On Sun, Jun 18, 2017 at 09:58:27AM +0100, Okash Khawaja wrote:
> +int ser_to_dev(int ser, dev_t *dev_no)
> +{
> +	if (ser < 0 || ser > (255 - 64)) {
> +                pr_err("speakup: Invalid ser param. \
> +				Must be between 0 and 191 inclusive.\n");


I pointed out that all these strings are wrong in the first version of
the patch.  You're going to end up with a whole bunch of tabs in your
dmesg output.

regards,
dan carpenter