[patch 0/6] staging: speakup: Introduce TTY-based comms

[patch 0/6] staging: speakup: Introduce TTY-based comms

[Okash Khawaja]

Hi,

This patchset introduces a TTY-based way for the synths to communicate
with devices as an alternate for direct serial comms used by the synths
at the moment. It then migrates some of the synths to the TTY-based
comms.

Synths migrated in this patchset are dummy, acntsa, bns, dectlk and
txprt. Please test them if possible.

Thanks,
Okash

[patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Okash Khawaja]

Allow access to TTY device from kernel. This is based on Alan Cox's patch
(http://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1215095.html),
with description quoted below.

"tty_port: allow a port to be opened with a tty that has no file handle

Let us create tty objects entirely in kernel space.

With this a kernel created non file backed tty object could be used to handle
data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
particular has to work back to the fs/tty layer.

The tty_port code is however otherwise clean of file handles as far as I can
tell as is the low level tty port write path used by the ldisc, the
configuration low level interfaces and most of the ldiscs.

Currently you don't have any exposure to see tty hangups because those are
built around the file layer. However a) it's a fixed port so you probably
don't care about that b) if you do we can add a callback and c) you almost
certainly don't want the userspace tear down/rebuild behaviour anyway.

This should however be sufficient if we wanted for example to enumerate all
the bluetooth bound fixed ports via ACPI and make them directly available.

It doesn't deal with the case of a user opening a port that's also kernel
opened and that would need some locking out (so it returned EBUSY if bound
to a kernel device of some kind). That needs resolving along with how you
"up" or "down" your new bluetooth device, or enumerate it while providing
the existing tty API to avoid regressions (and to debug)."

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

Index: linux-stable/drivers/tty/tty_io.c
===================================================================
--- linux-stable.orig/drivers/tty/tty_io.c
+++ linux-stable/drivers/tty/tty_io.c
@@ -855,7 +855,7 @@ static void tty_vhangup_session(struct t
 
 int tty_hung_up_p(struct file *filp)
 {
-	return (filp->f_op == &hung_up_tty_fops);
+	return (filp && filp->f_op == &hung_up_tty_fops);
 }
 
 EXPORT_SYMBOL(tty_hung_up_p);
@@ -1368,7 +1368,10 @@ static struct tty_struct *tty_driver_loo
 	struct tty_struct *tty;
 
 	if (driver->ops->lookup)
-		tty = driver->ops->lookup(driver, file, idx);
+		if (!file)
+			tty = ERR_PTR(-EIO);
+		else
+			tty = driver->ops->lookup(driver, file, idx);
 	else
 		tty = driver->ttys[idx];
 
@@ -1986,7 +1989,7 @@ static struct tty_driver *tty_lookup_dri
 		struct tty_driver *console_driver = console_device(index);
 		if (console_driver) {
 			driver = tty_driver_kref_get(console_driver);
-			if (driver) {
+			if (driver && filp) {
 				/* Don't let /dev/console block */
 				filp->f_flags |= O_NONBLOCK;
 				break;
@@ -2019,7 +2022,7 @@ static struct tty_driver *tty_lookup_dri
  *	  - concurrent tty driver removal w/ lookup
  *	  - concurrent tty removal from driver table
  */
-static struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
+struct tty_struct *tty_open_by_driver(dev_t device, struct inode *inode,
 					     struct file *filp)
 {
 	struct tty_struct *tty;
@@ -2064,6 +2067,7 @@ out:
 	tty_driver_kref_put(driver);
 	return tty;
 }
+EXPORT_SYMBOL(tty_open_by_driver);
 
 /**
  *	tty_open		-	open a tty device
Index: linux-stable/drivers/tty/tty_port.c
===================================================================
--- linux-stable.orig/drivers/tty/tty_port.c
+++ linux-stable/drivers/tty/tty_port.c
@@ -335,7 +335,7 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
  *	tty_port_block_til_ready	-	Waiting logic for tty open
  *	@port: the tty port being opened
  *	@tty: the tty device being bound
- *	@filp: the file pointer of the opener
+ *	@filp: the file pointer of the opener or NULL
  *
  *	Implement the core POSIX/SuS tty behaviour when opening a tty device.
  *	Handles:
@@ -369,7 +369,7 @@ int tty_port_block_til_ready(struct tty_
 		tty_port_set_active(port, 1);
 		return 0;
 	}
-	if (filp->f_flags & O_NONBLOCK) {
+	if (filp ==  NULL || filp->f_flags & O_NONBLOCK) {
 		/* Indicate we are open */
 		if (C_BAUD(tty))
 			tty_port_raise_dtr_rts(port);
Index: linux-stable/include/linux/tty.h
===================================================================
--- linux-stable.orig/include/linux/tty.h
+++ linux-stable/include/linux/tty.h
@@ -394,6 +394,8 @@ extern struct tty_struct *get_current_tt
 /* tty_io.c */
 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);
 #else
 static inline void console_init(void)
 { }

[patch 3/6] staging: speakup: refactor spk_stop_serial_interrupt

[Okash Khawaja]

This moves call to spk_stop_serial_interrupt() function out of synth_release()
and into release() method of specific spk_synth instances. This is because
a TTY-based synth implementation wouldn't need spk_stop_serial_interrupt()
call. Moving it into each synth's release() method gives the decision of
calling  spk_stop_serial_interrupt() to that synth. TTY-based synths which
follow in this patchset simply woudn't call it.

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

Index: linux-stable/drivers/staging/speakup/serialio.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/serialio.c
+++ linux-stable/drivers/staging/speakup/serialio.c
@@ -219,6 +219,7 @@
 
 void spk_serial_release(void)
 {
+	spk_stop_serial_interrupt();
 	if (speakup_info.port_tts == 0)
 		return;
 	synth_release_region(speakup_info.port_tts, 8);
Index: linux-stable/drivers/staging/speakup/speakup_acntpc.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_acntpc.c
+++ linux-stable/drivers/staging/speakup/speakup_acntpc.c
@@ -303,6 +303,7 @@
 
 static void accent_release(void)
 {
+	spk_stop_serial_interrupt();
 	if (speakup_info.port_tts)
 		synth_release_region(speakup_info.port_tts-1, SYNTH_IO_EXTENT);
 	speakup_info.port_tts = 0;
Index: linux-stable/drivers/staging/speakup/speakup_decpc.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_decpc.c
+++ linux-stable/drivers/staging/speakup/speakup_decpc.c
@@ -482,6 +482,7 @@
 
 static void dtpc_release(void)
 {
+	spk_stop_serial_interrupt();
 	if (speakup_info.port_tts)
 		synth_release_region(speakup_info.port_tts, SYNTH_IO_EXTENT);
 	speakup_info.port_tts = 0;
Index: linux-stable/drivers/staging/speakup/speakup_dtlk.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_dtlk.c
+++ linux-stable/drivers/staging/speakup/speakup_dtlk.c
@@ -374,6 +374,7 @@
 
 static void dtlk_release(void)
 {
+	spk_stop_serial_interrupt();
 	if (speakup_info.port_tts)
 		synth_release_region(speakup_info.port_tts-1, SYNTH_IO_EXTENT);
 	speakup_info.port_tts = 0;
Index: linux-stable/drivers/staging/speakup/speakup_keypc.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_keypc.c
+++ linux-stable/drivers/staging/speakup/speakup_keypc.c
@@ -305,6 +305,7 @@
 
 static void keynote_release(void)
 {
+	spk_stop_serial_interrupt();
 	if (synth_port)
 		synth_release_region(synth_port, SYNTH_IO_EXTENT);
 	synth_port = 0;
Index: linux-stable/drivers/staging/speakup/synth.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/synth.c
+++ linux-stable/drivers/staging/speakup/synth.c
@@ -432,7 +432,6 @@
 		sysfs_remove_group(speakup_kobj, &synth->attributes);
 	for (var = synth->vars; var->var_id != MAXVARS; var++)
 		speakup_unregister_var(var->var_id);
-	spk_stop_serial_interrupt();
 	synth->release();
 	synth = NULL;
 }

[patch 4/6] staging: speakup: Add spk_ttyio

[Okash Khawaja]

This adds spk_ttyio.c file. It contains a set of functions which implement
those methods in spk_synth struct which relate to sending bytes out using
serial comms. Implementations in this file perform the same function but
using TTY subsystem instead. Currently synths access serial ports, directly
poking standard ISA ports by trying to steal them from serial driver. Some ISA
cards actually need this way of doing it, but most other synthesizers don't,
and can actually work by using the proper TTY subsystem through a new N_SPEAKUP
line discipline. So this adds the methods for drivers to switch to accessing
serial ports through the TTY subsystem, whenever appropriate.

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

Index: linux-stable/drivers/staging/speakup/Makefile
===================================================================
--- linux-stable.orig/drivers/staging/speakup/Makefile
+++ linux-stable/drivers/staging/speakup/Makefile
@@ -25,6 +25,7 @@
 	kobjects.o \
 	selection.o \
 	serialio.o \
+	spk_ttyio.o \
 	synth.o \
 	thread.o \
 	varhandlers.o
Index: linux-stable/drivers/staging/speakup/spk_priv.h
===================================================================
--- linux-stable.orig/drivers/staging/speakup/spk_priv.h
+++ linux-stable/drivers/staging/speakup/spk_priv.h
@@ -46,7 +46,9 @@
 unsigned char spk_serial_in(void);
 unsigned char spk_serial_in_nowait(void);
 int spk_serial_out(struct spk_synth *in_synth, const char ch);
+int spk_ttyio_out(struct spk_synth *in_synth, const char ch);
 void spk_serial_release(void);
+void spk_ttyio_release(void);
 
 char synth_buffer_getc(void);
 char synth_buffer_peek(void);
@@ -58,7 +60,9 @@
 		      const char *buf, size_t count);
 
 int spk_serial_synth_probe(struct spk_synth *synth);
+int spk_ttyio_synth_probe(struct spk_synth *synth);
 const char *spk_synth_immediate(struct spk_synth *synth, const char *buff);
+const char *spk_ttyio_immediate(struct spk_synth *synth, const char *buff);
 void spk_do_catch_up(struct spk_synth *synth);
 void spk_synth_flush(struct spk_synth *synth);
 int spk_synth_is_alive_nop(struct spk_synth *synth);
Index: linux-stable/drivers/staging/speakup/spk_ttyio.c
===================================================================
--- /dev/null
+++ linux-stable/drivers/staging/speakup/spk_ttyio.c
@@ -0,0 +1,148 @@
+#include <linux/types.h>
+#include <linux/tty.h>
+
+#include "speakup.h"
+#include "spk_types.h"
+
+struct tty_struct *speakup_tty;
+EXPORT_SYMBOL_GPL(speakup_tty);
+
+static int spk_ttyio_ldisc_open(struct tty_struct *tty)
+{
+        if (tty->ops->write == NULL)
+                return -EOPNOTSUPP;
+        speakup_tty = tty;
+
+        return 0;
+}
+
+static void spk_ttyio_ldisc_close(struct tty_struct *tty)
+{
+        speakup_tty = NULL;
+}
+
+static struct tty_ldisc_ops spk_ttyio_ldisc_ops = {
+        .owner          = THIS_MODULE,
+        .magic          = TTY_LDISC_MAGIC,
+        .name           = "speakup_ldisc",
+        .open           = spk_ttyio_ldisc_open,
+        .close          = spk_ttyio_ldisc_close,
+};
+
+static int spk_ttyio_initialise_ldisc(void)
+{
+        int ret = 0;
+        struct tty_struct *tty;
+
+        ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
+        if (ret) {
+                pr_err("speakup_test_init(): Error registering line discipline.\n");
+                return ret;
+        }
+
+        tty = tty_open_by_driver(MKDEV(4, 64), NULL, NULL);
+        if (IS_ERR(tty))
+                return PTR_ERR(tty);
+
+        if (tty->ops->open)
+                ret = tty->ops->open(tty, NULL);
+        else
+                ret = -ENODEV;
+
+        if (ret) {
+                tty_unlock(tty);
+                return ret;
+        }
+
+        clear_bit(TTY_HUPPED, &tty->flags);
+        tty_unlock(tty);
+
+        ret = tty_set_ldisc(tty, N_SPEAKUP);
+
+        return ret;
+}
+
+int spk_ttyio_synth_probe(struct spk_synth *synth)
+{
+	int rv = spk_ttyio_initialise_ldisc();
+
+	if (rv)
+		return rv;
+
+	synth->alive = 1;
+
+        return 0;
+}
+EXPORT_SYMBOL_GPL(spk_ttyio_synth_probe);
+
+void spk_ttyio_release(void)
+{
+        int idx;
+
+        if (!speakup_tty)
+                return;
+
+        tty_lock(speakup_tty);
+        idx = speakup_tty->index;
+
+#if 0
+        if (tty_release_checks(tty, idx)) {
+                tty_unlock(tty);
+                return 0;
+        }
+#endif
+
+        if (speakup_tty->ops->close)
+                speakup_tty->ops->close(speakup_tty, NULL);
+
+        tty_ldisc_flush(speakup_tty);
+        tty_unlock(speakup_tty);
+        tty_ldisc_release(speakup_tty);
+#if 0
+        tty_flush_works(tty);
+#endif
+#if 0
+        mutex_lock(&tty_mutex);
+        release_tty(tty, idx);
+        mutex_unlock(&tty_mutex);
+#endif
+}
+EXPORT_SYMBOL_GPL(spk_ttyio_release);
+
+int spk_ttyio_out(struct spk_synth *in_synth, const char ch)
+{
+	if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
+		int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
+		if (ret == 0)
+			/* No room */
+			return 0;
+		if (ret < 0) {
+			pr_warn("%s: I/O error, deactivating speakup\n", in_synth->long_name);
+			/* No synth any more, so nobody will restart TTYs, and we thus
+			 * need to do it ourselves.  Now that there is no synth we can
+			 * let application flood anyway
+			 */
+			in_synth->alive = 0;
+			speakup_start_ttys();
+			return 0;
+		}
+		return 1;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spk_ttyio_out);
+
+const char *spk_ttyio_immediate(struct spk_synth *synth, const char *buff)
+{
+	u_char ch;
+
+	while ((ch = *buff)) {
+		if (ch == '\n')
+			ch = synth->procspeech;
+		if (tty_write_room(speakup_tty) < 1 || !spk_ttyio_out(synth, ch))
+			return buff;
+		buff++;
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(spk_ttyio_immediate);
Index: linux-stable/include/uapi/linux/tty.h
===================================================================
--- linux-stable.orig/include/uapi/linux/tty.h
+++ linux-stable/include/uapi/linux/tty.h
@@ -35,5 +35,6 @@
 #define N_TRACESINK	23	/* Trace data routing for MIPI P1149.7 */
 #define N_TRACEROUTER	24	/* Trace data routing for MIPI P1149.7 */
 #define N_NCI		25	/* NFC NCI UART */
+#define N_SPEAKUP	26      /* Speakup communication with synths*/
 
 #endif /* _UAPI_LINUX_TTY_H */
Index: linux-stable/drivers/tty/tty_ldisc.c
===================================================================
--- linux-stable.orig/drivers/tty/tty_ldisc.c
+++ linux-stable/drivers/tty/tty_ldisc.c
@@ -603,6 +603,8 @@
 	return retval;
 }
 
+EXPORT_SYMBOL(tty_set_ldisc);
+
 /**
  *	tty_ldisc_kill	-	teardown ldisc
  *	@tty: tty being released
@@ -795,6 +797,8 @@
 	tty_ldisc_debug(tty, "released\n");
 }
 
+EXPORT_SYMBOL(tty_ldisc_release);
+
 /**
  *	tty_ldisc_init		-	ldisc setup for new tty
  *	@tty: tty being allocated

[patch 5/6] staging: speakup: Make speakup_dummy use ttyio

[Okash Khawaja]

This changes speakup_dummy to use spk_ttyio implementations.

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

Index: linux-stable/drivers/staging/speakup/speakup_dummy.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_dummy.c
+++ linux-stable/drivers/staging/speakup/speakup_dummy.c
@@ -98,10 +98,10 @@
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
-	.probe = spk_serial_synth_probe,
-	.release = spk_serial_release,
-	.serial_out = spk_serial_out,
-	.synth_immediate = spk_synth_immediate,
+	.probe = spk_ttyio_synth_probe,
+	.release = spk_ttyio_release,
+	.serial_out = spk_ttyio_out,
+	.synth_immediate = spk_ttyio_immediate,
 	.catch_up = spk_do_catch_up,
 	.flush = spk_synth_flush,
 	.is_alive = spk_synth_is_alive_restart,

[patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

This changes the above four synths to TTY-based comms. They were chosen as a
first pass because their serial comms are straightforward, i.e. they don't use
serial input and don't do internal port knocking. This also moves
spk_serial_synth_probe() into serialio.c in order to segregate those functions
which directly use serial comms into serialio.c.

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

Index: linux-stable/drivers/staging/speakup/speakup_acntsa.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_acntsa.c
+++ linux-stable/drivers/staging/speakup/speakup_acntsa.c
@@ -100,9 +100,9 @@
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
 	.probe = synth_probe,
-	.release = spk_serial_release,
-	.serial_out = spk_serial_out,
-	.synth_immediate = spk_synth_immediate,
+	.release = spk_ttyio_release,
+	.serial_out = spk_ttyio_out,
+	.synth_immediate = spk_ttyio_immediate,
 	.catch_up = spk_do_catch_up,
 	.flush = spk_synth_flush,
 	.is_alive = spk_synth_is_alive_restart,
@@ -125,9 +125,9 @@
 {
 	int failed;
 
-	failed = spk_serial_synth_probe(synth);
+	failed = spk_ttyio_synth_probe(synth);
 	if (failed == 0) {
-		spk_synth_immediate(synth, "\033=R\r");
+		synth->synth_immediate(synth, "\033=R\r");
 		mdelay(100);
 	}
 	synth->alive = !failed;
Index: linux-stable/drivers/staging/speakup/speakup_bns.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_bns.c
+++ linux-stable/drivers/staging/speakup/speakup_bns.c
@@ -96,10 +96,10 @@
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
-	.probe = spk_serial_synth_probe,
-	.release = spk_serial_release,
-	.serial_out = spk_serial_out,
-	.synth_immediate = spk_synth_immediate,
+	.probe = spk_ttyio_synth_probe,
+	.release = spk_ttyio_release,
+	.serial_out = spk_ttyio_out,
+	.synth_immediate = spk_ttyio_immediate,
 	.catch_up = spk_do_catch_up,
 	.flush = spk_synth_flush,
 	.is_alive = spk_synth_is_alive_restart,
Index: linux-stable/drivers/staging/speakup/speakup_txprt.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_txprt.c
+++ linux-stable/drivers/staging/speakup/speakup_txprt.c
@@ -95,10 +95,10 @@
 	.startup = SYNTH_START,
 	.checkval = SYNTH_CHECK,
 	.vars = vars,
-	.probe = spk_serial_synth_probe,
-	.release = spk_serial_release,
-	.serial_out = spk_serial_out,
-	.synth_immediate = spk_synth_immediate,
+	.probe = spk_ttyio_synth_probe,
+	.release = spk_ttyio_release,
+	.serial_out = spk_ttyio_out,
+	.synth_immediate = spk_ttyio_immediate,
 	.catch_up = spk_do_catch_up,
 	.flush = spk_synth_flush,
 	.is_alive = spk_synth_is_alive_restart,
Index: linux-stable/drivers/staging/speakup/speakup_dectlk.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/speakup_dectlk.c
+++ linux-stable/drivers/staging/speakup/speakup_dectlk.c
@@ -130,10 +130,10 @@
 	.vars = vars,
 	.default_pitch = ap_defaults,
 	.default_vol = g5_defaults,
-	.probe = spk_serial_synth_probe,
-	.serial_out = spk_serial_out,
-	.release = spk_serial_release,
-	.synth_immediate = spk_synth_immediate,
+	.probe = spk_ttyio_synth_probe,
+	.serial_out = spk_ttyio_out,
+	.release = spk_ttyio_release,
+	.synth_immediate = spk_ttyio_immediate,
 	.catch_up = do_catch_up,
 	.flush = synth_flush,
 	.is_alive = spk_synth_is_alive_restart,
Index: linux-stable/drivers/staging/speakup/serialio.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/serialio.c
+++ linux-stable/drivers/staging/speakup/serialio.c
@@ -181,6 +181,36 @@
 	return 1;
 }
 
+int spk_serial_synth_probe(struct spk_synth *synth)
+{
+	const struct old_serial_port *ser;
+	int failed = 0;
+
+	if ((synth->ser >= SPK_LO_TTY) && (synth->ser <= SPK_HI_TTY)) {
+		ser = spk_serial_init(synth->ser);
+		if (ser == NULL) {
+			failed = -1;
+		} else {
+			outb_p(0, ser->port);
+			mdelay(1);
+			outb_p('\r', ser->port);
+		}
+	} else {
+		failed = -1;
+		pr_warn("ttyS%i is an invalid port\n", synth->ser);
+	}
+	if (failed) {
+		pr_info("%s: not found\n", synth->long_name);
+		return -ENODEV;
+	}
+	pr_info("%s: ttyS%i, Driver Version %s\n",
+		synth->long_name, synth->ser, synth->version);
+	synth->alive = 1;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spk_serial_synth_probe);
+
+
 unsigned char spk_serial_in(void)
 {
 	int tmout = SPK_SERIAL_TIMEOUT;
Index: linux-stable/drivers/staging/speakup/synth.c
===================================================================
--- linux-stable.orig/drivers/staging/speakup/synth.c
+++ linux-stable/drivers/staging/speakup/synth.c
@@ -44,35 +44,6 @@
 
 static int do_synth_init(struct spk_synth *in_synth);
 
-int spk_serial_synth_probe(struct spk_synth *synth)
-{
-	const struct old_serial_port *ser;
-	int failed = 0;
-
-	if ((synth->ser >= SPK_LO_TTY) && (synth->ser <= SPK_HI_TTY)) {
-		ser = spk_serial_init(synth->ser);
-		if (ser == NULL) {
-			failed = -1;
-		} else {
-			outb_p(0, ser->port);
-			mdelay(1);
-			outb_p('\r', ser->port);
-		}
-	} else {
-		failed = -1;
-		pr_warn("ttyS%i is an invalid port\n", synth->ser);
-	}
-	if (failed) {
-		pr_info("%s: not found\n", synth->long_name);
-		return -ENODEV;
-	}
-	pr_info("%s: ttyS%i, Driver Version %s\n",
-		synth->long_name, synth->ser, synth->version);
-	synth->alive = 1;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(spk_serial_synth_probe);
-
 /*
  * Main loop of the progression thread: keep eating from the buffer
  * and push to the serial port, waiting as needed

Re: [patch 3/6] staging: speakup: refactor spk_stop_serial_interrupt

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:25:26 +0000, wrote:
> This moves call to spk_stop_serial_interrupt() function out of synth_release()
> and into release() method of specific spk_synth instances.

Did you try to compile this as a module?  It seems this is missing an
EXPORT_SYMBOL_GPL line for modules to be able to call it.

I'm now having a try, will report later.

Samuel

Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Trevor Astrope]

Hi Okash,

Thanks for all the work you've done!

What is the minimum kernel version these patches can be applied against?

Trevor

On Sat, 25 Feb 2017, Okash Khawaja wrote:

> Hi,
>
> This patchset introduces a TTY-based way for the synths to communicate
> with devices as an alternate for direct serial comms used by the synths
> at the moment. It then migrates some of the synths to the TTY-based
> comms.
>
> Synths migrated in this patchset are dummy, acntsa, bns, dectlk and
> txprt. Please test them if possible.
>
> Thanks,
> Okash
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
>

Re: [patch 4/6] staging: speakup: Add spk_ttyio

[Samuel Thibault]

One thing you can already do easily is this:

Okash Khawaja, on sam. 25 févr. 2017 19:26:57 +0000, wrote:
> +static int spk_ttyio_initialise_ldisc(void)

Add an "int ser" parameter here.

> +{
> +        int ret = 0;
> +        struct tty_struct *tty;
> +
> +        ret = tty_register_ldisc(N_SPEAKUP, &spk_ttyio_ldisc_ops);
> +        if (ret) {
> +                pr_err("speakup_test_init(): Error registering line discipline.\n");
> +                return ret;
> +        }
> +
> +        tty = tty_open_by_driver(MKDEV(4, 64), NULL, NULL);

Here, check that ser is between 0 and (255-64), and add ser to 64.

> +int spk_ttyio_synth_probe(struct spk_synth *synth)
> +{
> +	int rv = spk_ttyio_initialise_ldisc();

Here, pass synth->ser to spk_ttyio_initialise_ldisc()


That way, people want easily configure the serial port with the existing
ser parameter, e.g. speakup_dummy.ser=1

and AIUI we should be getting the same support as before, and thus get
this tested and pushed upstream.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:21:32 +0000, wrote:
> Allow access to TTY device from kernel.

When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
we get this:

ttyS ttyS0: tty_open: tty->count(3) != #fd's(2)
ttyS ttyS0: tty_release: tty->count(3) != #fd's(2)

This is because the number of files in tty_files doesn't match the
open count for tty. spk_ttyio_initialise_ldisc should thus mimic
tty_open a bit more: after calling tty_open_by_driver, it should call
tty_add_file(tty, NULL); to add an entry in the tty_files list (and why
not calling check_tty_count too).  And of course, the converse
(tty_del_file) should be called by spk_ttyio_release between the
tty_ldisc_flush call and tty_unlock.

The consequence of this is that users of the tty_files list need to be
careful: __tty_hangup must now continue when filp is NULL. That last
modification should be added to patch 1.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

Samuel Thibault, on dim. 26 févr. 2017 01:53:42 +0100, wrote:
> Okash Khawaja, on sam. 25 févr. 2017 19:21:32 +0000, wrote:
> > Allow access to TTY device from kernel.
> 
> When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
> we get this:
> 
> ttyS ttyS0: tty_open: tty->count(3) != #fd's(2)
> ttyS ttyS0: tty_release: tty->count(3) != #fd's(2)
> 
> This is because the number of files in tty_files doesn't match the
> open count for tty. spk_ttyio_initialise_ldisc should thus mimic
> tty_open a bit more: after calling tty_open_by_driver, it should call
> tty_add_file(tty, NULL); to add an entry in the tty_files list (and why
> not calling check_tty_count too).  And of course, the converse
> (tty_del_file) should be called by spk_ttyio_release between the
> tty_ldisc_flush call and tty_unlock.

Oops, of course you don't have a filp to give to tty_del_file, so that
can't work. Ok, let's ignore the issue for now, applications are not
supposed to open the tty used by speakup anyway (and would get an EIO
error anyway).

Samuel

Re: [patch 4/6] staging: speakup: Add spk_ttyio

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:26:57 +0000, wrote:
> +const char *spk_ttyio_immediate(struct spk_synth *synth, const char *buff)

Rather call it spk_ttyio_synth_immediate, for coherency.

Samuel

Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:18:01 +0000, wrote:
> Synths migrated in this patchset are dummy, acntsa, bns, dectlk and
> txprt. Please test them if possible.

I patched ltlk too (dropping the input part, which is not mandatory),
and it seems to be working fine via a serial port :D I'll now try via a
USB port.

Samuel

Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Samuel Thibault]

Samuel Thibault, on dim. 26 févr. 2017 02:24:25 +0100, wrote:
> Okash Khawaja, on sam. 25 févr. 2017 19:18:01 +0000, wrote:
> > Synths migrated in this patchset are dummy, acntsa, bns, dectlk and
> > txprt. Please test them if possible.
> 
> I patched ltlk too (dropping the input part, which is not mandatory),
> and it seems to be working fine via a serial port :D I'll now try via a
> USB port.

Yep, works, congrats :)

Samuel

Re: [patch 2/6] staging: speakup: Add serial_out method

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:23:58 +0000, wrote:
> -int spk_wait_for_xmitr(void)
> +int spk_wait_for_xmitr(struct spk_synth *in_synth)

Please take this parameter addition into a separate patch, it's
unrelated to adding the serial_out method.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:30:31 +0000, wrote:
> This also moves spk_serial_synth_probe() into serialio.c in order
> to segregate those functions which directly use serial comms into
> serialio.c.

Please take this into another patch too, it is unrelated.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Okash Khawaja, on sam. 25 févr. 2017 19:30:31 +0000, wrote:
> Index: linux-stable/drivers/staging/speakup/speakup_dectlk.c
> ===================================================================
> --- linux-stable.orig/drivers/staging/speakup/speakup_dectlk.c
> +++ linux-stable/drivers/staging/speakup/speakup_dectlk.c

Actually, this one does use input, even if not explicitly, and it's
quite important actually since it's the flow control (XON/XOFF), so it
should not be included in our first round of conversion.

This is done through the read_buff_add method. Please note in your todo
for the input part, that ttyio will have to call read_buff_add for each
received character, when that method is not NULL (that's actually the
only driver using it, so we will really need a tester for this exact
driver).

Samuel

Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Samuel Thibault]

You should also move spk_synth_immediate to serialio.c, renaming it
spk_serial_synth_immediate along the way. That way, only some drivers
and serialio.c will be using in[bwl] and out[bwl].

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

You can also migrate ltlk, you just need to comment the call to
synth_interrogate() from synth_probe(). That'll disable getting the rom
version, but that's all, so it should be good enough for testing.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Samuel Thibault, on dim. 26 févr. 2017 02:50:05 +0100, wrote:
> Please note in your todo for the input part, that ttyio will have to
> call read_buff_add for each received character, when that method is
> not NULL (that's actually the only driver using it, so we will really
> need a tester for this exact driver).

More precisely, it's the spk_ttyio_ldisc_ops->receive_buf2 method which
should call read_buff_add for each received character.


The step further will be implementing spk_ttyio_in and
spk_ttyio_in_nowait.  I believe the easiest way is the following:

- define an spk_ldisc_data structure containing just one character (buf),
and a semaphore.

- on ldisc_open, allocate a pointer to such structure, set
tty->disc_data to point to it, and initialized the semaphore to 0
tokens.

- in the receive_buf2 method,
  - if read_buff_add is defined, just call it for each character and be
  done
  - otherwise, store the first received character in
    ((struct spk_ldisc_data *)tty->disc_data)->buf
    , call up() on the semaphore, and return 1 (to tell that you ate the
    character).

- in spk_serial_in, call down_timeout(usecs_to_jiffies(SPK_SERIAL_TIMEOUT)),
  - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
  - on failure (timed out), return 0xff.

- in spk_serial_in_nowait, call down_trylock(),
  - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
  - on failure, return 0.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Concerning speakup_apollo.c, it seems its only use of outb is to control
the modem RTS line. To support that, yet another step will be:

- introduce spk_serial_tiocmset(unsigned int set, unsigned int clear),
  which does:

  int old = inb(speakup_info.port_tts + UART_MCR);
  outb((old & ~clear) | set, speakup_info.port_tts + UART_MCR);

- introduce spk_ttyio_tiocmset(unsigned int set, unsigned int clear),
  which does:

  speakup_tty->ops->tiocmset(speakup_tty, set, clear);

and make them a tiocmset method of synths.

and then in speakup_apollo.c, instead of 

	outb(UART_MCR_DTR, speakup_info.port_tts + UART_MCR);
	outb(UART_MCR_DTR | UART_MCR_RTS,
			speakup_info.port_tts + UART_MCR);

rather use

	synth->tiocmset(0, UART_MCR_RTS);
	synth->tiocmset(UART_MCR_RTS, 0);

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Concerning speakup_decext.c, I believe it should be turned into defining
a read_buff_add method, which simply sets last_char to the received
character. get_last_char() can then simply be dropped, and synth_full()
just directly read last_char.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Last but not least, there are the outb(CLEAR) in speakup_audptr.c and
speakup_spkout. I'd say the following:

- add spk_serial_send_xchar(char ch) which does

  outb(ch, speakup_info.port_tts);

- add spk_ttyio_send_xchar(char ch) which does

  speakup_tty->ops->send_xchar(speakup_tty, ch)

and make these a send_xchar method of synths.

Then, in speakup_audptr.c, replace the content of synth_flush() with
synth->send_xchar(SYNTH_CLEAR) + the last line unmodified (synth->serial_out(PROCSPEECH);)

and in speakup_spkout.c, replace the content of synth_flush() with just
synth->send_xchar(SYNTH_CLEAR);

and we should be done with replacing in[bwl]/out[bwl]!
(except the internal synth (acntpc, decpc, keypc, dtlk))

Samuel

Re: [patch 2/6] staging: speakup: Add serial_out method

[Samuel Thibault]

So I sent a lot of mails :)

A lot of them are asking to add yet more methods.  I'm starting thinking
that it's tedious to change that in each and every driver, which is kind
of dumb anyway since it's just the same everywhere.

So I'd say before doing all that stuff, rework what we thought about
adding methods: instead of adding spk_serial_out directly in struct
spk_synth

- in spk_types.h, just before struct spk_synth, define

struct spk_io_ops {
	int (*serial_out)(struct spk_synth *synth, const char ch);
}

- in struct spk_synth, add

	struct spk_io_ops *io_ops;

- in serialio.c, add

	struct spk_io_ops spk_serial_io_ops {
		.serial_out = spk_serial_out,
	}

- in ttyio.c, add

	struct spk_io_ops spk_ttyio_io_ops {
		.serial_out = spk_ttyio_serial_out,
	}

- and in drivers, instead of defining

	.serial_out = spk_serial_out

rather define

	.io_ops = &spk_serial_io_ops


And then you can add the serial_in, serial_in_nowait, tiocmset,
send_xchar methods to spk_io_ops instead of spk_synth, thus making
switching between serial and tty much less tedious.

Note: we need to keep probe, release, and synth_immediate as spk_synth
methods since they vary among synth drivers.

Samuel

Read this first :) Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Samuel Thibault]

So, just a last mail, which could actually be read first :)

First, I notice that it took you 12 minutes to send the patch series.
This shows that you are not using quilt to do this :) Really, take the
time to look up how "quilt mail" works, and notably its --send option,
it will save you an awful lot of time (notably since I'm asking to
split the series is rather small patches, which is preferred thing when
pushing to Linux)

I also gave a lot of information, most of which are mostly notes for
later.  I'd say that the priority is this:

- work on the comments I made on the actual code (patch splitting,
  migrating ltlk, not migrating dectlk, moving spk_synth_immediate,
  exporting spk_stop_serial_interrupt, taking the ser parameter into
  account, renaming spk_ttyio_immediate to spk_ttyio_synth_immediate,
  and putting serial_out in a separate spk_io_ops structure)

- maybe post a last round of patches here, so I make a last review
  before we push to linux-kernel

- then we push to linux-kernel and get flames :)

- in the meanwhile, you can work on adding a "char *dev" synth parameter
  which would replace the "ser" parameter, and writing a function which
  translates it into an MKDEV(major,minor) pair (first implement it for
  "ttyS*", then for "ttyUSB*", etc.)

- hopefully at that point we can get that series commited mainstream,
  even if we only migrate the trivial drivers.

- then we can work on the other drivers as my mail series suggests.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[John Covici]

I wonder if applications should be allowed to open the tty?  What if
you had a speech dispatcher serial driver or some such?

On Sat, 25 Feb 2017 20:05:43 -0500,
Samuel Thibault wrote:
> 
> Samuel Thibault, on dim. 26 févr. 2017 01:53:42 +0100, wrote:
> > Okash Khawaja, on sam. 25 févr. 2017 19:21:32 +0000, wrote:
> > > Allow access to TTY device from kernel.
> > 
> > When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
> > we get this:
> > 
> > ttyS ttyS0: tty_open: tty->count(3) != #fd's(2)
> > ttyS ttyS0: tty_release: tty->count(3) != #fd's(2)
> > 
> > This is because the number of files in tty_files doesn't match the
> > open count for tty. spk_ttyio_initialise_ldisc should thus mimic
> > tty_open a bit more: after calling tty_open_by_driver, it should call
> > tty_add_file(tty, NULL); to add an entry in the tty_files list (and why
> > not calling check_tty_count too).  And of course, the converse
> > (tty_del_file) should be called by spk_ttyio_release between the
> > tty_ldisc_flush call and tty_unlock.
> 
> Oops, of course you don't have a filp to give to tty_del_file, so that
> can't work. Ok, let's ignore the issue for now, applications are not
> supposed to open the tty used by speakup anyway (and would get an EIO
> error anyway).
> 
> Samuel
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
> 

-- 
Your life is like a penny.  You're going to lose it.  The question is:
How do
you spend it?

         John Covici
         covici@ccs.covici.com

Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Okash Khawaja]

Hi Trevor,

The version that I have tested these on is 4.10.0. However, apart from
the first patch, most of the changes are within speakup code. So
patches should apply cleanly and work with older kernels as long as
it's not too old.

Thanks,
Okash

> On 26 Feb 2017, at 00:12, Trevor Astrope <astrope@tabbweb.com> wrote:
>
> Hi Okash,
>
> Thanks for all the work you've done!
>
> What is the minimum kernel version these patches can be applied against?
>
> Trevor
>
>> On Sat, 25 Feb 2017, Okash Khawaja wrote:
>>
>> Hi,
>>
>> This patchset introduces a TTY-based way for the synths to communicate
>> with devices as an alternate for direct serial comms used by the synths
>> at the moment. It then migrates some of the synths to the TTY-based
>> comms.
>>
>> Synths migrated in this patchset are dummy, acntsa, bns, dectlk and
>> txprt. Please test them if possible.
>>
>> Thanks,
>> Okash
>> _______________________________________________
>> Speakup mailing list
>> Speakup@linux-speakup.org
>> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
> _______________________________________________
> Speakup mailing list
> Speakup@linux-speakup.org
> http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup

Re: Read this first :) Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Okash Khawaja]

Hi,

Thanks for the quick feedback and this helpful summary :)

On Sun, Feb 26, 2017 at 3:38 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> So, just a last mail, which could actually be read first :)
>
> First, I notice that it took you 12 minutes to send the patch series.
> This shows that you are not using quilt to do this :) Really, take the
> time to look up how "quilt mail" works, and notably its --send option,
> it will save you an awful lot of time (notably since I'm asking to
> split the series is rather small patches, which is preferred thing when
> pushing to Linux)

Quilt has been very helpful in preparing the patches. Sending was
tedious without it but today got quilt mail --send working which is
great.

>
> I also gave a lot of information, most of which are mostly notes for
> later.  I'd say that the priority is this:
>
> - work on the comments I made on the actual code (patch splitting,
>   migrating ltlk, not migrating dectlk, moving spk_synth_immediate,
>   exporting spk_stop_serial_interrupt, taking the ser parameter into
>   account, renaming spk_ttyio_immediate to spk_ttyio_synth_immediate,
>   and putting serial_out in a separate spk_io_ops structure)
>
> - maybe post a last round of patches here, so I make a last review
>   before we push to linux-kernel
>
> - then we push to linux-kernel and get flames :)
>
> - in the meanwhile, you can work on adding a "char *dev" synth parameter
>   which would replace the "ser" parameter, and writing a function which
>   translates it into an MKDEV(major,minor) pair (first implement it for
>   "ttyS*", then for "ttyUSB*", etc.)

Sounds good. Should this be done before pushing to linux-kernel?

>
> - hopefully at that point we can get that series commited mainstream,
>   even if we only migrate the trivial drivers.
>
> - then we can work on the other drivers as my mail series suggests.
>
> Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

John Covici, on sam. 25 févr. 2017 23:30:45 -0500, wrote:
> I wonder if applications should be allowed to open the tty?  What if
> you had a speech dispatcher serial driver or some such?

If you let an application send data in the middle of what speakup sends,
it'd get mixed, and who knows how the synthesizer will behave or even
brick...

Really not a good idea :)

Applications can emit synthesis via /dev/synth, there is the speechd-up
speech dispatcher module for that.

Samuel

Re: Read this first :) Re: [patch 0/6] staging: speakup: Introduce TTY-based comms

[Samuel Thibault]

Okash Khawaja, on dim. 26 févr. 2017 09:53:11 +0000, wrote:
> > - in the meanwhile, you can work on adding a "char *dev" synth parameter
> >   which would replace the "ser" parameter, and writing a function which
> >   translates it into an MKDEV(major,minor) pair (first implement it for
> >   "ttyS*", then for "ttyUSB*", etc.)
> 
> Sounds good. Should this be done before pushing to linux-kernel?

It'd say no need to: the discussion with linux-kernel will be about tty
stuff. Next to the MKDEV line you can put a "TODO: support more than
ttyS*" to make it clear that we'll work on it after that.

The idea is that the discussion with linux-kernel will probably take
some time, during which you'll probably have the time to implement it,
and we can push that after (or probably within the late iterations of
pushing to linux-kernel).

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Hello,

Samuel Thibault, on dim. 26 févr. 2017 03:48:32 +0100, wrote:
> Samuel Thibault, on dim. 26 févr. 2017 02:50:05 +0100, wrote:
> > Please note in your todo for the input part, that ttyio will have to
> > call read_buff_add for each received character, when that method is
> > not NULL (that's actually the only driver using it, so we will really
> > need a tester for this exact driver).
> 
> More precisely, it's the spk_ttyio_ldisc_ops->receive_buf2 method which
> should call read_buff_add for each received character.
> 
> 
> The step further will be implementing spk_ttyio_in and
> spk_ttyio_in_nowait.  I believe the easiest way is the following:

I forgot the part that stops the tty layer from sending characters:

> - define an spk_ldisc_data structure containing just one character (buf),
> and a semaphore.

and one int (buf_free).

> - on ldisc_open, allocate a pointer to such structure, set
> tty->disc_data to point to it, and initialized the semaphore to 0
> tokens.

and initialize buf_free to 1.

> - in the receive_buf2 method,
>   - if read_buff_add is defined, just call it for each character and be
>   done
>   - otherwise, store the first received character in
>     ((struct spk_ldisc_data *)tty->disc_data)->buf
>     , call up() on the semaphore, and return 1 (to tell that you ate the
>     character).

Here, *just before* storing the character in buf, check buf_free: if
it is 0, return 0. otherwise, call mb(), and continue with what I wrote
above.

> - in spk_serial_in, call down_timeout(usecs_to_jiffies(SPK_SERIAL_TIMEOUT)),
>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>   - on failure (timed out), return 0xff.
> 
> - in spk_serial_in_nowait, call down_trylock(),
>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>   - on failure, return 0.

In each case, right after the copy, call mb() and set buf_free to 1.


Actually, these two function could be factorized to just one, which
takes a long timeout parameter (in jiffies) and returns an int
instead of a char, and uses down_trylock when the timeout is 0 and
down_timeout otherwise, and returns -1 instead of 0 on failure. And then
make spk_serial_in use it and return 0xff when getting -1, and make
spk_serial_in_nowait return 0 when getting -1. Cleaning that returned
value convention can be another patch later.

Samuel

Re: [patch 3/6] staging: speakup: refactor spk_stop_serial_interrupt

[Okash Khawaja]

Hi,

> On 25 Feb 2017, at 23:03, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>
> Okash Khawaja, on sam. 25 févr. 2017 19:25:26 +0000, wrote:
>> This moves call to spk_stop_serial_interrupt() function out of synth_release()
>> and into release() method of specific spk_synth instances.
>
> Did you try to compile this as a module?  It seems this is missing an
> EXPORT_SYMBOL_GPL line for modules to be able to call it.

It should need EXPORT_SYMBOL_GPL for other synths to compile as
modules but I don't get a linker warning when compiling them as
modules. Am I missing something here?

>
> I'm now having a try, will report later.

Re: [patch 3/6] staging: speakup: refactor spk_stop_serial_interrupt

[Samuel Thibault]

Okash Khawaja, on dim. 26 févr. 2017 15:11:54 +0000, wrote:
> > On 25 Feb 2017, at 23:03, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> >
> > Okash Khawaja, on sam. 25 févr. 2017 19:25:26 +0000, wrote:
> >> This moves call to spk_stop_serial_interrupt() function out of synth_release()
> >> and into release() method of specific spk_synth instances.
> >
> > Did you try to compile this as a module?  It seems this is missing an
> > EXPORT_SYMBOL_GPL line for modules to be able to call it.
> 
> It should need EXPORT_SYMBOL_GPL for other synths to compile as
> modules

Yes.

> but I don't get a linker warning when compiling them as modules. Am I
> missing something here?

I don't know, perhaps some config option about modules.

What I'm sure of is that it's needed at least for some configurations :)

Samuel

Re: [patch 2/6] staging: speakup: Add serial_out method

[Okash Khawaja]

On Sun, Feb 26, 2017 at 3:27 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> So I sent a lot of mails :)
>
> A lot of them are asking to add yet more methods.  I'm starting thinking
> that it's tedious to change that in each and every driver, which is kind
> of dumb anyway since it's just the same everywhere.
>
> So I'd say before doing all that stuff, rework what we thought about
> adding methods: instead of adding spk_serial_out directly in struct
> spk_synth
>
> - in spk_types.h, just before struct spk_synth, define
>
> struct spk_io_ops {
>         int (*serial_out)(struct spk_synth *synth, const char ch);
> }

Along the way, thinking of renaming serial_out to synth_out - agnostic
of actual mechanism: serial or ttyio.

Re: [patch 2/6] staging: speakup: Add serial_out method

[Samuel Thibault]

Okash Khawaja, on dim. 26 févr. 2017 19:16:35 +0000, wrote:
> On Sun, Feb 26, 2017 at 3:27 AM, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> > So I sent a lot of mails :)
> >
> > A lot of them are asking to add yet more methods.  I'm starting thinking
> > that it's tedious to change that in each and every driver, which is kind
> > of dumb anyway since it's just the same everywhere.
> >
> > So I'd say before doing all that stuff, rework what we thought about
> > adding methods: instead of adding spk_serial_out directly in struct
> > spk_synth
> >
> > - in spk_types.h, just before struct spk_synth, define
> >
> > struct spk_io_ops {
> >         int (*serial_out)(struct spk_synth *synth, const char ch);
> > }
> 
> Along the way, thinking of renaming serial_out to synth_out - agnostic
> of actual mechanism: serial or ttyio.

Indeed :)

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

Hi,

On Sun, Feb 26, 2017 at 03:59:30AM +0100, Samuel Thibault wrote:
> Concerning speakup_apollo.c, it seems its only use of outb is to control
> the modem RTS line. To support that, yet another step will be:
> 
> - introduce spk_serial_tiocmset(unsigned int set, unsigned int clear),
>   which does:
> 
>   int old = inb(speakup_info.port_tts + UART_MCR);
>   outb((old & ~clear) | set, speakup_info.port_tts + UART_MCR);
> 
> - introduce spk_ttyio_tiocmset(unsigned int set, unsigned int clear),
>   which does:
> 
>   speakup_tty->ops->tiocmset(speakup_tty, set, clear);
> 
> and make them a tiocmset method of synths.
> 
> and then in speakup_apollo.c, instead of 
> 
> 	outb(UART_MCR_DTR, speakup_info.port_tts + UART_MCR);
> 	outb(UART_MCR_DTR | UART_MCR_RTS,
> 			speakup_info.port_tts + UART_MCR);

Is there a specific reason for keeping DTR set all the time? Or is it
just a defensive measure?

> 
> rather use
> 
> 	synth->tiocmset(0, UART_MCR_RTS);
> 	synth->tiocmset(UART_MCR_RTS, 0);
> 
> Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Okash Khawaja, on ven. 24 mars 2017 19:21:54 +0000, wrote:
> On Sun, Feb 26, 2017 at 03:59:30AM +0100, Samuel Thibault wrote:
> > and then in speakup_apollo.c, instead of 
> > 
> > 	outb(UART_MCR_DTR, speakup_info.port_tts + UART_MCR);
> > 	outb(UART_MCR_DTR | UART_MCR_RTS,
> > 			speakup_info.port_tts + UART_MCR);
> 
> Is there a specific reason for keeping DTR set all the time? Or is it
> just a defensive measure?

I guess the device requires it. That's normal RS232 signaling anyway.
With tiocmset, however, you won't have to care: the generic code will
already have enabled DTR, and you will only want to clear and set RTS.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

On Fri, Mar 24, 2017 at 08:24:22PM +0100, Samuel Thibault wrote:
> Okash Khawaja, on ven. 24 mars 2017 19:21:54 +0000, wrote:
> > On Sun, Feb 26, 2017 at 03:59:30AM +0100, Samuel Thibault wrote:
> > > and then in speakup_apollo.c, instead of 
> > > 
> > > 	outb(UART_MCR_DTR, speakup_info.port_tts + UART_MCR);
> > > 	outb(UART_MCR_DTR | UART_MCR_RTS,
> > > 			speakup_info.port_tts + UART_MCR);
> > 
> > Is there a specific reason for keeping DTR set all the time? Or is it
> > just a defensive measure?
> 
> I guess the device requires it. That's normal RS232 signaling anyway.
> With tiocmset, however, you won't have to care: the generic code will
> already have enabled DTR, and you will only want to clear and set RTS.

I noticed that spk_serial_init sets it and other places keep it set. By
generic code, do you mean that when migrated to TTY, we don't have to
explicitly set DTR?

> 
> Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Okash Khawaja, on ven. 24 mars 2017 19:40:42 +0000, wrote:
> On Fri, Mar 24, 2017 at 08:24:22PM +0100, Samuel Thibault wrote:
> > Okash Khawaja, on ven. 24 mars 2017 19:21:54 +0000, wrote:
> > > On Sun, Feb 26, 2017 at 03:59:30AM +0100, Samuel Thibault wrote:
> > > > and then in speakup_apollo.c, instead of 
> > > > 
> > > > 	outb(UART_MCR_DTR, speakup_info.port_tts + UART_MCR);
> > > > 	outb(UART_MCR_DTR | UART_MCR_RTS,
> > > > 			speakup_info.port_tts + UART_MCR);
> > > 
> > > Is there a specific reason for keeping DTR set all the time? Or is it
> > > just a defensive measure?
> > 
> > I guess the device requires it. That's normal RS232 signaling anyway.
> > With tiocmset, however, you won't have to care: the generic code will
> > already have enabled DTR, and you will only want to clear and set RTS.
> 
> I noticed that spk_serial_init sets it and other places keep it set. By
> generic code, do you mean that when migrated to TTY, we don't have to
> explicitly set DTR?

Yes, it'll already be enabled by the layers below.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

Hi,

On Sun, Feb 26, 2017 at 2:48 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Samuel Thibault, on dim. 26 févr. 2017 02:50:05 +0100, wrote:
>> Please note in your todo for the input part, that ttyio will have to
>> call read_buff_add for each received character, when that method is
>> not NULL (that's actually the only driver using it, so we will really
>> need a tester for this exact driver).
>
> More precisely, it's the spk_ttyio_ldisc_ops->receive_buf2 method which
> should call read_buff_add for each received character.
>
>
> The step further will be implementing spk_ttyio_in and
> spk_ttyio_in_nowait.  I believe the easiest way is the following:
>
> - define an spk_ldisc_data structure containing just one character (buf),
> and a semaphore.
>
> - on ldisc_open, allocate a pointer to such structure, set
> tty->disc_data to point to it, and initialized the semaphore to 0
> tokens.
>
> - in the receive_buf2 method,
>   - if read_buff_add is defined, just call it for each character and be
>   done
>   - otherwise, store the first received character in
>     ((struct spk_ldisc_data *)tty->disc_data)->buf
>     , call up() on the semaphore, and return 1 (to tell that you ate the
>     character).


I don't fully understand how the return value of receive_buf2 is used
in flow control. According to Documentation/serial/tty.txt it is the
number of bytes processed by it, whereas comments on top of
tty_ldisc_receive_buf function's definition - which returns value
returned by receive_buf2 - say it is the number of bytes not
processed.

Also, is the call to tty_schedule_flip in spk_serial_in because we
returned 1 receive_buf2 so we have to manually tell it to flip buffer?

Thanks,
Okash

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Hello,

Okash Khawaja, on mar. 28 mars 2017 15:35:08 +0100, wrote:
> On Sun, Feb 26, 2017 at 2:48 AM, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> > - in the receive_buf2 method,
> >   - if read_buff_add is defined, just call it for each character and be
> >   done
> >   - otherwise, store the first received character in
> >     ((struct spk_ldisc_data *)tty->disc_data)->buf
> >     , call up() on the semaphore, and return 1 (to tell that you ate the
> >     character).
> 
> 
> I don't fully understand how the return value of receive_buf2 is used
> in flow control. According to Documentation/serial/tty.txt it is the
> number of bytes processed by it, whereas comments on top of
> tty_ldisc_receive_buf function's definition - which returns value
> returned by receive_buf2 - say it is the number of bytes not
> processed.

I believe the comment is wrong: for instance n_tty_receive_buf_common
clearly returns the number of processed bytes, and users (such as
paste_selection, receive_buf from flush_to_ldisc) use it that way.  You
can probably submit a patch that fixes the comment already.

> Also, is the call to tty_schedule_flip in spk_serial_in because we
> returned 1 receive_buf2 so we have to manually tell it to flip buffer?

(note: I meant spk_ttyio_in, of course)

It's because we only returned 1, yes, i.e. we only ate 1 character,
while there probably were more to read, but we didn't eat them, so we
have to tell the tty layer when we are ready to eat more.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

On Tue, Mar 28, 2017 at 4:11 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on mar. 28 mars 2017 15:35:08 +0100, wrote:
>> On Sun, Feb 26, 2017 at 2:48 AM, Samuel Thibault
>> <samuel.thibault@ens-lyon.org> wrote:
>> > - in the receive_buf2 method,
>> >   - if read_buff_add is defined, just call it for each character and be
>> >   done
>> >   - otherwise, store the first received character in
>> >     ((struct spk_ldisc_data *)tty->disc_data)->buf
>> >     , call up() on the semaphore, and return 1 (to tell that you ate the
>> >     character).
>>
>>
>> I don't fully understand how the return value of receive_buf2 is used
>> in flow control. According to Documentation/serial/tty.txt it is the
>> number of bytes processed by it, whereas comments on top of
>> tty_ldisc_receive_buf function's definition - which returns value
>> returned by receive_buf2 - say it is the number of bytes not
>> processed.
>
> I believe the comment is wrong: for instance n_tty_receive_buf_common
> clearly returns the number of processed bytes, and users (such as
> paste_selection, receive_buf from flush_to_ldisc) use it that way.  You
> can probably submit a patch that fixes the comment already.
>
>> Also, is the call to tty_schedule_flip in spk_serial_in because we
>> returned 1 receive_buf2 so we have to manually tell it to flip buffer?
>
> (note: I meant spk_ttyio_in, of course)
>
> It's because we only returned 1, yes, i.e. we only ate 1 character,
> while there probably were more to read, but we didn't eat them, so we
> have to tell the tty layer when we are ready to eat more.

Right, so either we process all characters, return count (the argument
to receive_buf2) and don't bother with tty_schedule_flip, or we
process less than count and call tty_schedule_flip.

Thanks very much for explaining
Okash

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Okash Khawaja, on mar. 28 mars 2017 16:35:39 +0100, wrote:
> Right, so either we process all characters, return count (the argument
> to receive_buf2) and don't bother with tty_schedule_flip, or we
> process less than count and call tty_schedule_flip.

Yes.

And we don't want the former, because that'd mean buffering in our own
layer, while tty already does so for us.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

Hi,

On Sun, Feb 26, 2017 at 11:07 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Samuel Thibault, on dim. 26 févr. 2017 03:48:32 +0100, wrote:
>> Samuel Thibault, on dim. 26 févr. 2017 02:50:05 +0100, wrote:
>> > Please note in your todo for the input part, that ttyio will have to
>> > call read_buff_add for each received character, when that method is
>> > not NULL (that's actually the only driver using it, so we will really
>> > need a tester for this exact driver).
>>
>> More precisely, it's the spk_ttyio_ldisc_ops->receive_buf2 method which
>> should call read_buff_add for each received character.
>>
>>
>> The step further will be implementing spk_ttyio_in and
>> spk_ttyio_in_nowait.  I believe the easiest way is the following:
>
> I forgot the part that stops the tty layer from sending characters:
>
>> - define an spk_ldisc_data structure containing just one character (buf),
>> and a semaphore.
>
> and one int (buf_free).
>
>> - on ldisc_open, allocate a pointer to such structure, set
>> tty->disc_data to point to it, and initialized the semaphore to 0
>> tokens.
>
> and initialize buf_free to 1.
>
>> - in the receive_buf2 method,
>>   - if read_buff_add is defined, just call it for each character and be
>>   done
>>   - otherwise, store the first received character in
>>     ((struct spk_ldisc_data *)tty->disc_data)->buf
>>     , call up() on the semaphore, and return 1 (to tell that you ate the
>>     character).
>
> Here, *just before* storing the character in buf, check buf_free: if
> it is 0, return 0. otherwise, call mb(), and continue with what I wrote
> above.
>
>> - in spk_serial_in, call down_timeout(usecs_to_jiffies(SPK_SERIAL_TIMEOUT)),
>>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>>   - on failure (timed out), return 0xff.
>>
>> - in spk_serial_in_nowait, call down_trylock(),
>>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>>   - on failure, return 0.
>
> In each case, right after the copy, call mb() and set buf_free to 1.
>
>
> Actually, these two function could be factorized to just one, which
> takes a long timeout parameter (in jiffies) and returns an int
> instead of a char, and uses down_trylock when the timeout is 0 and
> down_timeout otherwise, and returns -1 instead of 0 on failure. And then
> make spk_serial_in use it and return 0xff when getting -1, and make
> spk_serial_in_nowait return 0 when getting -1. Cleaning that returned
> value convention can be another patch later.
>

Here's another approach using atomic_t instead of semaphore but still
employing same logic.

- spk_ldisc_data contains two members: char buf and atomic_t buf_free
- ldisc_open initialises buf_free to 1
- in receive_buf2, if read_buff_add is not defined:
        - check if buf_free is zero then return zero and finish
        - call mb()
        - copy first character to buf in spk_ldisc_data
        - call atomic_dec(&buf_free)
        - return 1
- in spk_ttyio_in:
        - countdown for SPK_SERIAL_TIMEOUT usecs - similar to
spk_serial_in - and check for atomic_read(&buf_free) == 0
        - if timed out, return 0xff
        - otherwise call mb()
        - read the char stored in buf and call atomic_inc(&buf_free)
        - call tty_schedule_flip()
        - return the char

spk_ttyio_in_nowait will be similar and can be factorised into
spk_ttyio_in as you suggested.

Using this approach we can avoid using down_timeout() which may need
justification when merging upstream. I might be wrong here but there
seems to be a somewhat less acceptance for timeout while acquiring
locks. For example in case of mutex_lock_timeout:
http://lkml.iu.edu/hypermail/linux/kernel/0611.3/0254.html.

If however there are plans of making buf bigger than one char, then
this will require reworking as the above scheme overloads buf_free
both for mutual exclusion and to count free bytes in buffer.

Okash

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Samuel Thibault]

Hello,

Okash Khawaja, on jeu. 30 mars 2017 15:45:08 +0100, wrote:
> - in spk_ttyio_in:
>         - countdown for SPK_SERIAL_TIMEOUT usecs - similar to
> spk_serial_in - and check for atomic_read(&buf_free) == 0

Such busy polling will be way less acceptable than down_timeout :)
And all the more so since it does not actually try to give CPU to the
part of the kernel which will provide the character, while down_timeout
exactly does that.

> seems to be a somewhat less acceptance for timeout while acquiring
> locks. For example in case of mutex_lock_timeout:
> http://lkml.iu.edu/hypermail/linux/kernel/0611.3/0254.html.

Here we are not acquiring a lock: the down_timeout call is used to
try to consume a character, that's a very different thing than a
mutex_lock_timeout, which is indeed a very questionable thing.

Samuel

Re: [patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

On Thu, Mar 30, 2017 at 4:19 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on jeu. 30 mars 2017 15:45:08 +0100, wrote:
>> - in spk_ttyio_in:
>>         - countdown for SPK_SERIAL_TIMEOUT usecs - similar to
>> spk_serial_in - and check for atomic_read(&buf_free) == 0
>
> Such busy polling will be way less acceptable than down_timeout :)
> And all the more so since it does not actually try to give CPU to the
> part of the kernel which will provide the character, while down_timeout
> exactly does that.

Of course. I misread the code in down_timeout() which uses spinlock
which made me think it's busy waiting.

>
>> seems to be a somewhat less acceptance for timeout while acquiring
>> locks. For example in case of mutex_lock_timeout:
>> http://lkml.iu.edu/hypermail/linux/kernel/0611.3/0254.html.
>
> Here we are not acquiring a lock: the down_timeout call is used to
> try to consume a character

Yes I can see now that we utilise waiting feature of semaphore here.

Thanks,
Okash

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Okash Khawaja]

Hi,

On Sun, Feb 26, 2017 at 1:05 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Samuel Thibault, on dim. 26 févr. 2017 01:53:42 +0100, wrote:
>> Okash Khawaja, on sam. 25 févr. 2017 19:21:32 +0000, wrote:
>> > Allow access to TTY device from kernel.
>>
>> When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
>> we get this:
>>
>> ttyS ttyS0: tty_open: tty->count(3) != #fd's(2)
>> ttyS ttyS0: tty_release: tty->count(3) != #fd's(2)
>>
>> This is because the number of files in tty_files doesn't match the
>> open count for tty. spk_ttyio_initialise_ldisc should thus mimic
>> tty_open a bit more: after calling tty_open_by_driver, it should call
>> tty_add_file(tty, NULL); to add an entry in the tty_files list (and why
>> not calling check_tty_count too).  And of course, the converse
>> (tty_del_file) should be called by spk_ttyio_release between the
>> tty_ldisc_flush call and tty_unlock.
>
> Oops, of course you don't have a filp to give to tty_del_file, so that
> can't work. Ok, let's ignore the issue for now, applications are not
> supposed to open the tty used by speakup anyway (and would get an EIO
> error anyway).

This issue with opening tty from userspace while it's in use from kernel. Wonder
if this will rear its ugly head with rescpect to the recent patches?

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

Okash Khawaja, on mer. 17 mai 2017 08:44:19 +0100, wrote:
> On Sun, Feb 26, 2017 at 1:05 AM, Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> > Samuel Thibault, on dim. 26 févr. 2017 01:53:42 +0100, wrote:
> >> Okash Khawaja, on sam. 25 févr. 2017 19:21:32 +0000, wrote:
> >> > Allow access to TTY device from kernel.
> >>
> >> When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
> >> we get this:
> >>
> >> ttyS ttyS0: tty_open: tty->count(3) != #fd's(2)
> >> ttyS ttyS0: tty_release: tty->count(3) != #fd's(2)
> >>
> >> This is because the number of files in tty_files doesn't match the
> >> open count for tty. spk_ttyio_initialise_ldisc should thus mimic
> >> tty_open a bit more: after calling tty_open_by_driver, it should call
> >> tty_add_file(tty, NULL); to add an entry in the tty_files list (and why
> >> not calling check_tty_count too).  And of course, the converse
> >> (tty_del_file) should be called by spk_ttyio_release between the
> >> tty_ldisc_flush call and tty_unlock.
> >
> > Oops, of course you don't have a filp to give to tty_del_file, so that
> > can't work. Ok, let's ignore the issue for now, applications are not
> > supposed to open the tty used by speakup anyway (and would get an EIO
> > error anyway).
> 
> This issue with opening tty from userspace while it's in use from kernel. Wonder
> if this will rear its ugly head with rescpect to the recent patches?

Ah, yes, we will have to think about that issue.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Okash Khawaja]

On Wed, May 17, 2017 at 09:45:46AM +0200, Samuel Thibault wrote:
> > This issue with opening tty from userspace while it's in use from kernel. Wonder
> > if this will rear its ugly head with rescpect to the recent patches?
> 
> Ah, yes, we will have to think about that issue.

Would you suggest me raising this with Alan Cox as he mentioned some
locking to prevent this scenario?

Thanks,
Okash

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

Hello,

Okash Khawaja, on mer. 17 mai 2017 14:38:32 +0100, wrote:
> On Wed, May 17, 2017 at 09:45:46AM +0200, Samuel Thibault wrote:
> > > This issue with opening tty from userspace while it's in use from kernel. Wonder
> > > if this will rear its ugly head with rescpect to the recent patches?
> > 
> > Ah, yes, we will have to think about that issue.
> 
> Would you suggest me raising this with Alan Cox as he mentioned some
> locking to prevent this scenario?

Mmm, I don't see which locking you refer to, I don't remember Alan Cox
talking about it?  AFAICS, tty_open() will just always manage to open
the tty, and reach the check_tty_count() call.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Okash Khawaja]

Hi,

On Mon, May 22, 2017 at 11:07 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on mer. 17 mai 2017 14:38:32 +0100, wrote:
>> On Wed, May 17, 2017 at 09:45:46AM +0200, Samuel Thibault wrote:
>> > > This issue with opening tty from userspace while it's in use from kernel. Wonder
>> > > if this will rear its ugly head with rescpect to the recent patches?
>> >
>> > Ah, yes, we will have to think about that issue.
>>
>> Would you suggest me raising this with Alan Cox as he mentioned some
>> locking to prevent this scenario?
>
> Mmm, I don't see which locking you refer to, I don't remember Alan Cox
> talking about it?
It's in the same RFC patch that from Alan:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1215095.html

Here's quote:

    "It doesn't deal with the case of a user opening a port that's also kernel
    opened and that would need some locking out (so it returned EBUSY if bound
    to a kernel device of some kind)."

> AFAICS, tty_open() will just always manage to open
> the tty, and reach the check_tty_count() call.
Yes. Wonder if that is okay then or do we need to do something about it?

Thanks,
Okash

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Samuel Thibault]

Okash Khawaja, on mar. 23 mai 2017 09:30:34 +0100, wrote:
> Here's quote:
> 
>     "It doesn't deal with the case of a user opening a port that's also kernel
>     opened and that would need some locking out (so it returned EBUSY if bound
>     to a kernel device of some kind)."

Ah, ok, so that doesn't exist yet and needs to be added. Something like
a flag added to the tty structure, which is set and cleared when opening
and closing the device from the kernel.

> > AFAICS, tty_open() will just always manage to open
> > the tty, and reach the check_tty_count() call.
> Yes. Wonder if that is okay then or do we need to do something about it?

On the contrary, that the problem: we don't want it to reach
check_tty_count(), since that's what warns out.

Samuel

Re: [patch 1/6] tty_port: allow a port to be opened with a tty that has no file handle

[Okash Khawaja]

On Tue, May 23, 2017 at 9:33 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Okash Khawaja, on mar. 23 mai 2017 09:30:34 +0100, wrote:
>> Here's quote:
>>
>>     "It doesn't deal with the case of a user opening a port that's also kernel
>>     opened and that would need some locking out (so it returned EBUSY if bound
>>     to a kernel device of some kind)."
>
> Ah, ok, so that doesn't exist yet and needs to be added. Something like
> a flag added to the tty structure, which is set and cleared when opening
> and closing the device from the kernel.
Right that sounds good. I can test it out on my side and then we can
plan what to do with the patch.

Okash