Line discipline

Line discipline

[Okash Khawaja]

Hi,

Just to be sure, this is what we want for speakup_dummy.

1. register ldisc structure in spk_tty_probe().
2. in spk_tty_probe(), attach line discipline - like you would for
/dev/ttyX from user space
3. in open() method of ldisc structure, take the tty_struct and cache it.
4. in spk_serial_out_tty() call tty->ops->write() instead of outb().

What have a I missed here?

Thanks,
Okash

Re: Line discipline

[Samuel Thibault]

Hello,

Okash Khawaja, on Mon 21 Nov 2016 22:32:52 +0000, wrote:
> 1. register ldisc structure in spk_tty_probe().

Registering the speakup line discipline should be done on speakup module
insertion (speakup_init), since we're supposed to do it just once.

> 2. in spk_tty_probe(), attach line discipline - like you would for /dev/ttyX
> from user space

spk_tty_probe also needs to open ttySX itsef.

> 3. in open() method of ldisc structure, take the tty_struct and cache it.

Yes.

> 4. in spk_serial_out_tty() call tty->ops->write() instead of outb().

Yes.

Samuel

Re: Line discipline

[Okash Khawaja]

On Mon, Nov 21, 2016 at 10:36 PM, Samuel Thibault <
samuel.thibault@ens-lyon.org> wrote:

> Hello,
>
> Okash Khawaja, on Mon 21 Nov 2016 22:32:52 +0000, wrote:
> > 1. register ldisc structure in spk_tty_probe().
>
> Registering the speakup line discipline should be done on speakup module
> insertion (speakup_init), since we're supposed to do it just once.
>
> > 2. in spk_tty_probe(), attach line discipline - like you would for
> /dev/ttyX
> > from user space
>
> spk_tty_probe also needs to open ttySX itsef.
>
> > 3. in open() method of ldisc structure, take the tty_struct and cache it.
>
> Yes.
>
> > 4. in spk_serial_out_tty() call tty->ops->write() instead of outb().
>
> Yes.
>
> Samuel
>

Thanks.

So I have a simple kernel module that registers a new ldisc. When userspace
attaches that ldisc to /dev/ttyS0, the kernel module is able to write to
that serial port using tty->ops->write().

Currently I'm investigating the core problem of opening /dev/ttyS0 from
kernel space and attaching our ldisc to it. Are there any existing ideas
around this? Is this worth investigating: to find correct tty without
opening /dev/ttyS0 and assigning our ldisc to it. Realistically, how early
in boot process do we want the ldisc assigned?

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Wed 23 Nov 2016 14:46:59 +0000, wrote:
> So I have a simple kernel module that registers a new ldisc. When userspace
> attaches that ldisc to /dev/ttyS0, the kernel module is able to write to that
> serial port using tty->ops->write().

Ok, good for a start :)

> Currently I'm investigating the core problem of opening /dev/ttyS0 from kernel
> space and attaching our ldisc to it. Are there any existing ideas around this?

What I wrote before:

“
how can speakup open
the port?  We don't have a process context or /dev/, so we can't just
use sys_open and alike.  What we could use is some function which takes
a minor/major pair or a device name, and returns a filp, then we can
tty_set_ldisc(N_SPEAKUP) on file_tty(filp), but I don't know if such
thing exists?  That would probably be building a struct inode (getting
inspired from fs/ramfs/), then just open it? Something like:

struct inode *inode = new_inode(sb);

init_special_inode(inode, S_IFCHR, MKDEV(major, minor));
filp = get_empty_filp();
do_dentry_open(filp, inode, NULL, NULL);
struct tty_struct *tty = file_tty(filp);
tty_set_ldisc(tty, N_SPEAKUP);
”

> Is this worth investigating: to find correct tty without opening /dev/ttyS0 and
> assigning our ldisc to it.

> Realistically, how early in boot process do we want the ldisc
> assigned?

As early as possible :)

That'll mean after tty initialization and after serial driver initialization

Samuel

Re: Line discipline

[Okash Khawaja]

Hi,

Thanks for explanation. I have not been able to work on this recently -
pulled away for something urgent. As soon as it's over, I'll be back on it.
Probably third week of December.


Cheers,
Okash

On Wed, Nov 23, 2016 at 2:56 PM, Samuel Thibault <
samuel.thibault@ens-lyon.org> wrote:

> Okash Khawaja, on Wed 23 Nov 2016 14:46:59 +0000, wrote:
> > So I have a simple kernel module that registers a new ldisc. When
> userspace
> > attaches that ldisc to /dev/ttyS0, the kernel module is able to write to
> that
> > serial port using tty->ops->write().
>
> Ok, good for a start :)
>
> > Currently I'm investigating the core problem of opening /dev/ttyS0 from
> kernel
> > space and attaching our ldisc to it. Are there any existing ideas around
> this?
>
> What I wrote before:
>
> “
> how can speakup open
> the port?  We don't have a process context or /dev/, so we can't just
> use sys_open and alike.  What we could use is some function which takes
> a minor/major pair or a device name, and returns a filp, then we can
> tty_set_ldisc(N_SPEAKUP) on file_tty(filp), but I don't know if such
> thing exists?  That would probably be building a struct inode (getting
> inspired from fs/ramfs/), then just open it? Something like:
>
> struct inode *inode = new_inode(sb);
>
> init_special_inode(inode, S_IFCHR, MKDEV(major, minor));
> filp = get_empty_filp();
> do_dentry_open(filp, inode, NULL, NULL);
> struct tty_struct *tty = file_tty(filp);
> tty_set_ldisc(tty, N_SPEAKUP);
> ”
>
> > Is this worth investigating: to find correct tty without opening
> /dev/ttyS0 and
> > assigning our ldisc to it.
>
> > Realistically, how early in boot process do we want the ldisc
> > assigned?
>
> As early as possible :)
>
> That'll mean after tty initialization and after serial driver
> initialization
>
> Samuel
>

Re: Line discipline

[Samuel Thibault]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

Hello,

Okash Khawaja, on Sun 27 Nov 2016 20:41:50 +0000, wrote:
> Thanks for explanation. I have not been able to work on this recently - pulled
> away for something urgent. As soon as it's over, I'll be back on it. Probably
> third week of December.

I've taken some time to have a look, I have attached the result:

- tty_NULL is a patch against the kernel to make tty functions accept
  being called with filp == NULL
- mymodule.tgz is a dumb module which opens ttyS0 (major 4 minor 64),
  and writes to it through the tty write operation.

So this, combined with your work on the line discipline, should get
something working, and opening other serial ports (e.g. ttyUSB0) is a
matter of changing the major/minor pair (e.g. 188, 0).

Samuel

[-- Attachment #2: tty_NULL --]
[-- Type: text/plain, Size: 2938 bytes --]

Index: linux-4.8/drivers/tty/tty_port.c
===================================================================
--- linux-4.8.orig/drivers/tty/tty_port.c
+++ linux-4.8/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-4.8/drivers/tty/tty_io.c
===================================================================
--- linux-4.8.orig/drivers/tty/tty_io.c
+++ linux-4.8/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-4.8/include/linux/tty.h
===================================================================
--- linux-4.8.orig/include/linux/tty.h
+++ linux-4.8/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)
 { }

Re: Line discipline

[Okash Khawaja]

Hi Samuel,

That's great. Let me get back on this soon as I can. 

Thanks,
Okash

> On 11 Dec 2016, at 20:22, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> Hello,
> 
> Okash Khawaja, on Sun 27 Nov 2016 20:41:50 +0000, wrote:
>> Thanks for explanation. I have not been able to work on this recently - pulled
>> away for something urgent. As soon as it's over, I'll be back on it. Probably
>> third week of December.
> 
> I've taken some time to have a look, I have attached the result:
> 
> - tty_NULL is a patch against the kernel to make tty functions accept
>  being called with filp == NULL
> - mymodule.tgz is a dumb module which opens ttyS0 (major 4 minor 64),
>  and writes to it through the tty write operation.
> 
> So this, combined with your work on the line discipline, should get
> something working, and opening other serial ports (e.g. ttyUSB0) is a
> matter of changing the major/minor pair (e.g. 188, 0).
> 
> Samuel
> <mymodule.tgz>
> <tty_NULL>

Re: Line discipline

[Okash Khawaja]

Also, the exit function speakup_test_cleanup() needs to contain code from
your module's exit function, if it is used as a kernel module, rather than
being built into kernel image.

On Thu, Dec 15, 2016 at 10:17 PM, Okash Khawaja <okash.khawaja@gmail.com>
wrote:

> Hi,
>
> I quickly put together speakup-test.c and compiled it into the kernel
> binary. Loading the kernel outputs test messages to ttyS0 and logs the
> event in kernel log. "got tty ffff8fcfe74b0000" message appears at time
> 3.258357. Both speakup-test.c and dmesg.log are attached. N_SPEAKUP is 26
> in /include/uapi/linux/tty.h.
>
> Also tested this as kernel module which seemed to work fine too. That
> required EXPORT_SYMBOL(tty_set_ldisc); in /drivers/tty/tty_ldisc.c. Will be
> able to spend more time over the weekend.
>
> Thanks!
> Okash
>
> On Sun, Dec 11, 2016 at 10:33 PM, Okash Khawaja <okash.khawaja@gmail.com>
> wrote:
>
>> Hi Samuel,
>>
>> That's great. Let me get back on this soon as I can.
>>
>> Thanks,
>> Okash
>>
>> > On 11 Dec 2016, at 20:22, Samuel Thibault <samuel.thibault@ens-lyon.org>
>> wrote:
>> >
>> > Hello,
>> >
>> > Okash Khawaja, on Sun 27 Nov 2016 20:41:50 +0000, wrote:
>> >> Thanks for explanation. I have not been able to work on this recently
>> - pulled
>> >> away for something urgent. As soon as it's over, I'll be back on it.
>> Probably
>> >> third week of December.
>> >
>> > I've taken some time to have a look, I have attached the result:
>> >
>> > - tty_NULL is a patch against the kernel to make tty functions accept
>> >  being called with filp == NULL
>> > - mymodule.tgz is a dumb module which opens ttyS0 (major 4 minor 64),
>> >  and writes to it through the tty write operation.
>> >
>> > So this, combined with your work on the line discipline, should get
>> > something working, and opening other serial ports (e.g. ttyUSB0) is a
>> > matter of changing the major/minor pair (e.g. 188, 0).
>> >
>> > Samuel
>> > <mymodule.tgz>
>> > <tty_NULL>
>>
>
>

Re: Line discipline

[Okash Khawaja]

Hi,

Looks like my previous email didn't get through due to large attachment
size.

I've combined your module with my code and created speakup-test.c module (
http://pastebin.com/pXqRgyF1). When compiled into the kernel binary, it is
able to communicate during the boot process. Relevant part of kernel log
reproduced below. I've posted full log here: http://pastebin.com/UqMAktFm

[    3.382675] GHES: HEST is not enabled!
[    3.383142] ACPI: Battery Slot [BAT0] (battery present)
[    3.383161] Serial: 8250/16550 driver, 32 ports, IRQ sharing enabled
[    3.404736] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a
16550A
[    3.405554] Linux agpgart interface v0.103
[    3.405616] got tty ffff97ed674b0000
[    3.405805] speakup_test_open(): tty->name: ttyS0;
tty->ldisc->ops->name: speakup_test_ldisc
[    3.405894] speakup_test_open(): done writing. rv=9
[    3.407209] brd: module loaded
[    3.407722] loop: module loaded
[    3.407789] ata_piix 0000:00:01.1: version 2.13
[    3.408060] scsi host0: ata_piix

Comms work fine both as a LKM and when built in.

Now I see three items to address, listed below in no particular order.

1. Supplying major and minor dev numbers, instead of hardcoding.
2. Integrating the changes into speakup_dummy and testing it.
3. Strategy for kernel patch. Do we try to have it accepted? Not sure if
there is a standard way of addressing it.

Thanks,
Okash

On Mon, Nov 21, 2016 at 10:32 PM, Okash Khawaja <okash.khawaja@gmail.com>
wrote:

> Hi,
>
> Just to be sure, this is what we want for speakup_dummy.
>
> 1. register ldisc structure in spk_tty_probe().
> 2. in spk_tty_probe(), attach line discipline - like you would for
> /dev/ttyX from user space
> 3. in open() method of ldisc structure, take the tty_struct and cache it.
> 4. in spk_serial_out_tty() call tty->ops->write() instead of outb().
>
> What have a I missed here?
>
> Thanks,
> Okash
>

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Sun 18 Dec 2016 11:24:11 +0000, wrote:
> Now I see three items to address, listed below in no particular order.
> 
> 1. Supplying major and minor dev numbers, instead of hardcoding.

This could be a mere module parameter string that a function turns into
major/minor. Not a big deal :)

> 2. Integrating the changes into speakup_dummy and testing it.

Yep!

> 3. Strategy for kernel patch. Do we try to have it accepted? Not sure if there
> is a standard way of addressing it.

It will never be accepted before step 2. is done.  In the end we'll want
to get it accepted, sure, but we have to make speakup able to use it
first, otherwise the kernel patch will be rejected.

Thanks!
Samuel

Re: Line discipline

[Okash Khawaja]

Thanks. It seems relatively straightforward from here on. Job switch combined with an unexpectedly busy holiday period means that I haven't been able to work on this recently. Things will settle next week when I will be back on it. 

Merry Christmas and happy new year!

Okash

> On 20 Dec 2016, at 01:08, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> Okash Khawaja, on Sun 18 Dec 2016 11:24:11 +0000, wrote:
>> Now I see three items to address, listed below in no particular order.
>> 
>> 1. Supplying major and minor dev numbers, instead of hardcoding.
> 
> This could be a mere module parameter string that a function turns into
> major/minor. Not a big deal :)
> 
>> 2. Integrating the changes into speakup_dummy and testing it.
> 
> Yep!
> 
>> 3. Strategy for kernel patch. Do we try to have it accepted? Not sure if there
>> is a standard way of addressing it.
> 
> It will never be accepted before step 2. is done.  In the end we'll want
> to get it accepted, sure, but we have to make speakup able to use it
> first, otherwise the kernel patch will be rejected.
> 
> Thanks!
> Samuel

Re: Line discipline

[Okash Khawaja]

Hi Samuel,

Combined the refactor changes with your tty code to test speakup_dummy. It
seems okay except for problem when unloading speakup.ko - it is in use so
can't be unloaded. I have just got this so investigating it.

One question. Using your code, we can obtain tty, cache it and use it
directly for all subsequent writes. Why then use ldisc?

Thanks,
Okash

On Mon, Dec 26, 2016 at 4:02 PM, Okash Khawaja <okash.khawaja@gmail.com>
wrote:

> Thanks. It seems relatively straightforward from here on. Job switch
> combined with an unexpectedly busy holiday period means that I haven't been
> able to work on this recently. Things will settle next week when I will be
> back on it.
>
> Merry Christmas and happy new year!
>
> Okash
>
> > On 20 Dec 2016, at 01:08, Samuel Thibault <samuel.thibault@ens-lyon.org>
> wrote:
> >
> > Okash Khawaja, on Sun 18 Dec 2016 11:24:11 +0000, wrote:
> >> Now I see three items to address, listed below in no particular order.
> >>
> >> 1. Supplying major and minor dev numbers, instead of hardcoding.
> >
> > This could be a mere module parameter string that a function turns into
> > major/minor. Not a big deal :)
> >
> >> 2. Integrating the changes into speakup_dummy and testing it.
> >
> > Yep!
> >
> >> 3. Strategy for kernel patch. Do we try to have it accepted? Not sure
> if there
> >> is a standard way of addressing it.
> >
> > It will never be accepted before step 2. is done.  In the end we'll want
> > to get it accepted, sure, but we have to make speakup able to use it
> > first, otherwise the kernel patch will be rejected.
> >
> > Thanks!
> > Samuel
>

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Tue 24 Jan 2017 06:16:46 +0000, wrote:
> Combined the refactor changes with your tty code to test speakup_dummy. It
> seems okay

Good :)

> except for problem when unloading speakup.ko - it is in use so can't
> be unloaded. I have just got this so investigating it.

I guess it's a release issue on the tty or such. Could you share your
code so we can investigate too?

> One question. Using your code, we can obtain tty, cache it and use it directly
> for all subsequent writes. Why then use ldisc?

To be able to read characters. There is no "read" operation in the tty.
We'll receive characters through the receive_buf callback.

Samuel

Re: Line discipline

[Okash Khawaja]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

Hi,

On Tue, Jan 24, 2017 at 8:30 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Okash Khawaja, on Tue 24 Jan 2017 06:16:46 +0000, wrote:
>> Combined the refactor changes with your tty code to test speakup_dummy. It
>> seems okay
>
> Good :)
>
>> except for problem when unloading speakup.ko - it is in use so can't
>> be unloaded. I have just got this so investigating it.
>
> I guess it's a release issue on the tty or such. Could you share your
> code so we can investigate too?
>
>> One question. Using your code, we can obtain tty, cache it and use it directly
>> for all subsequent writes. Why then use ldisc?
>
> To be able to read characters. There is no "read" operation in the tty.
> We'll receive characters through the receive_buf callback.

Thanks for clarification.
>
> Samuel

I have attached the relevant diff. This is applied after the refactor
changes which allow us to use tty version of spk_serial_out(). Also
I've just hacked it together to test speakup_dummy - other synths
won't work after this change. Please not that since I haven't been
able to look into it yet, it might be a trivial error on my part.

[-- Attachment #2: speakup-ldisc.diff --]
[-- Type: text/plain, Size: 4485 bytes --]

diff --git a/drivers/staging/speakup/serialio.c b/drivers/staging/speakup/serialio.c
index d8b891f..61c82ba 100644
--- a/drivers/staging/speakup/serialio.c
+++ b/drivers/staging/speakup/serialio.c
@@ -217,9 +217,19 @@ int spk_serial_out(const char ch)
 }
 EXPORT_SYMBOL_GPL(spk_serial_out);
 
+extern struct tty_struct *speakup_tty;
+
 int spk_serial_out_tty(const char ch)
 {
-	return spk_serial_out(ch);
+	// return spk_serial_out(ch);
+	
+	pr_warn("spk_serial_out_tty(): ch=%c, speakup_tty=0x%x\n", ch, speakup_tty);
+
+	if (speakup_tty && speakup_tty->ops->write) {
+		speakup_tty->ops->write(speakup_tty, &ch, 1);
+		return 1;
+	}
+	return 0;
 }
 EXPORT_SYMBOL_GPL(spk_serial_out_tty);
 
diff --git a/drivers/staging/speakup/synth.c b/drivers/staging/speakup/synth.c
index 425438b..dd192a6 100644
--- a/drivers/staging/speakup/synth.c
+++ b/drivers/staging/speakup/synth.c
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/timer.h>
 #include <linux/kthread.h>
+#include <linux/tty.h>
 
 #include "spk_priv.h"
 #include "speakup.h"
@@ -44,8 +45,116 @@ EXPORT_SYMBOL_GPL(speakup_info);
 
 static int do_synth_init(struct spk_synth *in_synth);
 
+struct tty_struct *speakup_tty;
+EXPORT_SYMBOL_GPL(speakup_tty);
+
+static int speakup_ldisc_open(struct tty_struct *tty)
+{   
+    int rv;
+
+    // okashTODO: check for errors
+
+    pr_warn("speakup_test_open(): tty->name: %s; tty->ldisc->ops->name: %s\n",
+        tty->name, tty->ldisc->ops->name);
+    rv = tty->ops->write(tty, "hi ldisc\n", 9);
+    pr_warn("speakup_test_open(): done writing. rv=%d\n", rv);
+    speakup_tty = tty;
+
+    return 0;
+}
+
+static void speakup_ldisc_close(struct tty_struct *tty)
+{
+    pr_warn("speakup_test_close()\n");
+    speakup_tty = NULL;
+}
+
+static struct tty_ldisc_ops speakup_ldisc_ops = {
+    .owner          = THIS_MODULE,
+    .magic          = TTY_LDISC_MAGIC,
+    .name           = "speakup_ldisc",
+    .open           = speakup_ldisc_open,
+    .close          = speakup_ldisc_close,
+};
+
+
+static int initialise_ldisc(void)
+{
+    int ret = 0;
+    struct tty_struct *tty;
+
+    ret = tty_register_ldisc(N_SPEAKUP, &speakup_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);
+
+    printk("got tty %p\n", tty);
+
+    if (tty->ops->open)
+        ret = tty->ops->open(tty, NULL);
+    else
+        ret = -ENODEV;
+
+/* to be put in ldisc open */
+    if (tty->ops->write == NULL)
+        ret = -EOPNOTSUPP;
+/* */
+
+    if (ret) {
+        tty_unlock(tty);
+        return ret;
+    }
+        
+    clear_bit(TTY_HUPPED, &tty->flags);
+    tty_unlock(tty);
+
+    tty_set_ldisc(tty, N_SPEAKUP);
+    pr_warn(">>> finished calling tty_set_ldisc\n");
+
+    tty->ops->write(tty, "test\n", 5);
+
+    return ret;
+}
+
+static void release_ldisc(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_unlock(speakup_tty);
+#if 0
+        tty_flush_works(tty);
+#endif
+#if 0
+        mutex_lock(&tty_mutex);
+        release_tty(tty, idx);
+        mutex_unlock(&tty_mutex);
+#endif
+}
+
 int spk_serial_synth_probe(struct spk_synth *synth)
 {
+/*
 	const struct old_serial_port *ser;
 	int failed = 0;
 
@@ -68,7 +177,10 @@ int spk_serial_synth_probe(struct spk_synth *synth)
 	}
 	pr_info("%s: ttyS%i, Driver Version %s\n",
 		synth->long_name, synth->ser, synth->version);
+*/
+	initialise_ldisc();
 	synth->alive = 1;
+	
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spk_serial_synth_probe);
@@ -421,6 +533,10 @@ void synth_release(void)
 	struct var_t *var;
 	unsigned long flags;
 
+	// okashTODO: maybe speakup_tty should be part of synth.
+	
+	release_ldisc();
+
 	if (synth == NULL)
 		return;
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
@@ -432,7 +548,7 @@ void synth_release(void)
 		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();
+//	spk_stop_serial_interrupt();
 	synth->release();
 	synth = NULL;
 }

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Tue 24 Jan 2017 06:16:46 +0000, wrote:
> Combined the refactor changes with your tty code to test speakup_dummy. It
> seems okay except for problem when unloading speakup.ko - it is in use so can't
> be unloaded. I have just got this so investigating it.

Just to make sure: it's expected that the speakup module is busy while
speakup_dummy is still loaded, since there is a dependency between the
two, you need to rmmod speakup_dummy before speakup.

Samuel

Re: Line discipline

[Okash Khawaja]

Hi,

> On 25 Jan 2017, at 00:51, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> Okash Khawaja, on Tue 24 Jan 2017 06:16:46 +0000, wrote:
>> Combined the refactor changes with your tty code to test speakup_dummy. It
>> seems okay except for problem when unloading speakup.ko - it is in use so can't
>> be unloaded. I have just got this so investigating it.
> 
> Just to make sure: it's expected that the speakup module is busy while
> speakup_dummy is still loaded, since there is a dependency between the
> two, you need to rmmod speakup_dummy before speakup.
> 
> Samuel

Yes. So I'm using insmod and rmmod instead of modprobe. Here are the steps:

insmod speakup.ko
insmod speakup_dummy.ko
rmmod speakup_dummy
rmmod speakup # this fails with message that speakup is in use

Also speakup module, on it own, can be loaded and removed without problem.

Re: Line discipline

[Samuel Thibault]

Hello,

Okash Khawaja, on Tue 24 Jan 2017 21:52:26 +0000, wrote:
> >> except for problem when unloading speakup.ko - it is in use so can't
> >> be unloaded. I have just got this so investigating it.

Very probably a missing thing at speakup_dummy removal time.

> +static void speakup_ldisc_close(struct tty_struct *tty)
> +{
> +    pr_warn("speakup_test_close()\n");

Does it get called at speakup_dummy removal time?

> +static void release_ldisc(void)
> +{

Does it get called at speakup_dummy removal time?

> @@ -421,6 +533,10 @@ void synth_release(void)
>  	struct var_t *var;
>  	unsigned long flags;
>  
> +	// okashTODO: maybe speakup_tty should be part of synth.
> +	
> +	release_ldisc();

Mmm, I believe calling release_ldisc() rather belongs to the release
method of the synths. And notably:

> @@ -432,7 +548,7 @@ void synth_release(void)
>  		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();
> +//	spk_stop_serial_interrupt();
>  	synth->release();

The spk_stop_serial_interrupt() call should be moved into
spk_serial_release, accent_release, dtpc_release, dtlk_release, and
keynote_release, and we'd have another function like spk_serial_release
for the tty case.

Samuel

Re: Line discipline

[Okash Khawaja]

Hi,

On Thu, Jan 26, 2017 at 12:18 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on Tue 24 Jan 2017 21:52:26 +0000, wrote:
>> >> except for problem when unloading speakup.ko - it is in use so can't
>> >> be unloaded. I have just got this so investigating it.
>
> Very probably a missing thing at speakup_dummy removal time.
>
>> +static void speakup_ldisc_close(struct tty_struct *tty)
>> +{
>> +    pr_warn("speakup_test_close()\n");
>
> Does it get called at speakup_dummy removal time?

No

>
>> +static void release_ldisc(void)
>> +{
>
> Does it get called at speakup_dummy removal time?

Yes

>
>> @@ -421,6 +533,10 @@ void synth_release(void)
>>       struct var_t *var;
>>       unsigned long flags;
>>
>> +     // okashTODO: maybe speakup_tty should be part of synth.
>> +
>> +     release_ldisc();
>
> Mmm, I believe calling release_ldisc() rather belongs to the release
> method of the synths. And notably:
>
>> @@ -432,7 +548,7 @@ void synth_release(void)
>>               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();
>> +//   spk_stop_serial_interrupt();
>>       synth->release();
>
> The spk_stop_serial_interrupt() call should be moved into
> spk_serial_release, accent_release, dtpc_release, dtlk_release, and
> keynote_release, and we'd have another function like spk_serial_release
> for the tty case.

Sure, absolutely. I hacked this together just to test it in action so
it's not final yet :)
>
> Samuel

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Thu 26 Jan 2017 08:24:29 +0000, wrote:
> >> +static void speakup_ldisc_close(struct tty_struct *tty)
> >> +{
> >> +    pr_warn("speakup_test_close()\n");
> >
> > Does it get called at speakup_dummy removal time?
> 
> No

Ok.  I guess what we are missing is some part of the rest of
tty_release(). We notably seem to be lacking tty_ldisc_release(tty);
between tty_unlock and tty_flush_works.

Samuel

Re: Line discipline

[Okash Khawaja]

Hi,

On Thu, Jan 26, 2017 at 8:33 AM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Okash Khawaja, on Thu 26 Jan 2017 08:24:29 +0000, wrote:
>> >> +static void speakup_ldisc_close(struct tty_struct *tty)
>> >> +{
>> >> +    pr_warn("speakup_test_close()\n");
>> >
>> > Does it get called at speakup_dummy removal time?
>>
>> No
>
> Ok.  I guess what we are missing is some part of the rest of
> tty_release(). We notably seem to be lacking tty_ldisc_release(tty);
> between tty_unlock and tty_flush_works.

Added tty_ldisc_release() which resolved the problem. Also added
tty_ldisc_flush() before tty_ldisc_release(). Wouldn't it be safer to
add those functions before tty_unlock()?

The only output I get from speakup_dummy is:

Speakup
RATE 8
PITCH 8
VOL 8
TONE 8
Dummy found

That's same before and after this ldisc change.

I looked around for tty release code, specifically tty_release() in
drivers/tty/tty_io.c. Not sure if there's something else missing here.
The modules seem to load and unload (multiple times) okay. Kernel log
doesn't show anything odd. I'll still do some more tests and make the
changes you suggested above before sharing the patch.

>
> Samuel

Thanks,
Okash

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Sat 28 Jan 2017 17:49:01 +0000, wrote:
> Added tty_ldisc_release() which resolved the problem.

Ok, good.

> Also added tty_ldisc_flush() before tty_ldisc_release().

I don't think it's really needed since the user is asking for speakup to
go away anyway, but it shouldn't hurt.

> Wouldn't it be safer to add those functions before tty_unlock()?

See callers of these, they don't lock the tty.

> The only output I get from speakup_dummy is:
> 
> Speakup
> RATE 8
> PITCH 8
> VOL 8
> TONE 8
> Dummy found

But are you working on the linux text console?

Samuel

Re: Line discipline

[Okash Khawaja]

On Sat, Jan 28, 2017 at 5:57 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Okash Khawaja, on Sat 28 Jan 2017 17:49:01 +0000, wrote:
>> Added tty_ldisc_release() which resolved the problem.
>
> Ok, good.
>
>> Also added tty_ldisc_flush() before tty_ldisc_release().
>
> I don't think it's really needed since the user is asking for speakup to
> go away anyway, but it shouldn't hurt.
>
>> Wouldn't it be safer to add those functions before tty_unlock()?
>
> See callers of these, they don't lock the tty.
>
>> The only output I get from speakup_dummy is:
>>
>> Speakup
>> RATE 8
>> PITCH 8
>> VOL 8
>> TONE 8
>> Dummy found
>
> But are you working on the linux text console?

It's SSH into Linux VM which runs speakup_dummy, so running pseudo
terminal. Should be okay?

>
> Samuel

Re: Line discipline

[Samuel Thibault]

Okash Khawaja, on Sat 28 Jan 2017 18:29:03 +0000, wrote:
> It's SSH into Linux VM which runs speakup_dummy, so running pseudo
> terminal. Should be okay?

Yes, but then don't be surprised that speakup doesn't print anything:
nothing is happening on the linux text console :) You need to log into
the linux text console and do stuff there to get speakup to speak it.

Samuel

Re: Line discipline

[Okash Khawaja]

On Sat, Jan 28, 2017 at 6:30 PM, Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
> Okash Khawaja, on Sat 28 Jan 2017 18:29:03 +0000, wrote:
>> It's SSH into Linux VM which runs speakup_dummy, so running pseudo
>> terminal. Should be okay?
>
> Yes, but then don't be surprised that speakup doesn't print anything:
> nothing is happening on the linux text console :) You need to log into
> the linux text console and do stuff there to get speakup to speak it.

My mistake. For some reason I thought espeakup was working with my
xterm which also uses pseudo terminal which led me to (misguided)
belief that it should work for SSH connection too. Thanks, it's
looking better now :)
>
> Samuel

Re: Line discipline

[Okash Khawaja]

On Thu, Feb 2, 2017 at 11:51 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
> Hi,
>
> On Sat, Jan 28, 2017 at 6:56 PM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
>> On Sat, Jan 28, 2017 at 6:30 PM, Samuel Thibault
>> <samuel.thibault@ens-lyon.org> wrote:
>>> Okash Khawaja, on Sat 28 Jan 2017 18:29:03 +0000, wrote:
>>>> It's SSH into Linux VM which runs speakup_dummy, so running pseudo
>>>> terminal. Should be okay?
>>>
>>> Yes, but then don't be surprised that speakup doesn't print anything:
>>> nothing is happening on the linux text console :) You need to log into
>>> the linux text console and do stuff there to get speakup to speak it.
>>
>> My mistake. For some reason I thought espeakup was working with my
>> xterm which also uses pseudo terminal which led me to (misguided)
>> belief that it should work for SSH connection too. Thanks, it's
>> looking better now :)
>>>
>>> Samuel
>
> I have attached the patch which uses tty for speakup_dummy. It applies
> without conflicts to 4.10.x kernel code but showed conflicts for
> 4.8.6. Please note that major and minor device numbers are hard coded
> as 4 and 64 inside spk_ttyio_initialise() function. I have changed
> names slightly, so there is spk_ttyio_* prefix which identifies
> functions related to tty-based comms. The related code is grouped
> inside spk_ttyio.c file.
>
> I noticed that `startup` member of synth_dummy instance was set to
> SYNTH_START which meant that probe function for dummy won't be called
> when built into kernel image. May be I didn't understand it fully, but
> for now I have changed to always be the literal 1.
>
> Tested dummy and also soft synth/espeakup for regressions. Both seemed
> okay but I didn't fully test them. Any ideas for testing will be
> useful.
>
> Thanks,
> Okash

Attaching gzipped patch due to attachment size limits on the mailing list.

Re: Line discipline

[Samuel Thibault]

Hello,

Okash Khawaja, on Thu 02 Feb 2017 23:51:26 +0000, wrote:
> I have attached the patch which uses tty for speakup_dummy.

It looks good, cool :)

> It applies without conflicts to 4.10.x kernel code but showed
> conflicts for 4.8.6.

No problem, it'll take some time to get to a point where it is
submittable, so no need to target old kernels.

> Please note that major and minor device numbers are hard coded as 4
> and 64 inside spk_ttyio_initialise() function.

Sure, that's fine for now.

> I have changed names slightly, so there is spk_ttyio_* prefix which
> identifies functions related to tty-based comms. The related code is
> grouped inside spk_ttyio.c file.

Good :)

> I noticed that `startup` member of synth_dummy instance was set to
> SYNTH_START which meant that probe function for dummy won't be called
> when built into kernel image.

Yes, AIUI that is on purpose, so that one can build a kernel with all
drivers compiled in, and use e.g. the synth=dummy kernel parameter to
make the dummy driver started. So please do not change that part, leave
it as it is.

> Tested dummy and also soft synth/espeakup for regressions. Both seemed
> okay but I didn't fully test them. Any ideas for testing will be
> useful.

>From looking at the patch, I wouldn't expect any regression anyway.
Ideally you'd do some daily work like hacking source code, compiling,
etc. with the dummy driver enabled, to exercice speakup in harsh
conditions.

Last but not least, you will need to maintain a patch queue, so that the
whole thing is easy to review.  So before developping more, please learn
about quilt, and split what you have into several parts already, which
can be easily reviewed and make sense separately, I'd say five patches
in the patch queue:

- the drivers/tty/*+include/linux/tty.h
- the addition of the serial_out method, ploggued onto spk_serial_out
  for all drivers
- the move of spk_stop_serial_interrupt into spk_serial_release
- the addition of spk_ttyio and N_SPEAKUP
- making dummy use ttyio methods instead of serial

The idea is that when applying patches one after the other, everything
should be working fine at each step. Each patch needs a changelog to
explain what it does, see patch submissions on the linux kernel mailing
list for examples of changelogs.

Then you'll be able to insert a patch for the serial_in method, and
another patch that would add a parameter to be parsed to produce
e.g. 4,64 in the ttyS0 case, 188,0 in the ttyUSB0 case, etc. (see
Documentation/devices.txt), and a patch to make other drivers use it.

Samuel

Re: Line discipline

[Samuel Thibault]

Samuel Thibault, on Fri 03 Feb 2017 01:11:44 +0100, wrote:
> see patch submissions on the linux kernel mailing list for examples of
> changelogs.

And of course Documentation/SubmittingPatches.

But really for now, learn about quilt, so that you get at ease with
changing code in the different patches of your series.

Samuel

Re: Line discipline

[Okash Khawaja]

> On 3 Feb 2017, at 00:13, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
> 
> Samuel Thibault, on Fri 03 Feb 2017 01:11:44 +0100, wrote:
>> see patch submissions on the linux kernel mailing list for examples of
>> changelogs.
> 
> And of course Documentation/SubmittingPatches.
> 
> But really for now, learn about quilt, so that you get at ease with
> changing code in the different patches of your series.
> 
> Samuel

Thanks all that is very helpful. I'll find out about quilt. 

Re: Line discipline

[Okash Khawaja]

On Fri, Feb 3, 2017 at 12:20 AM, Okash Khawaja <okash.khawaja@gmail.com> wrote:
>
>> On 3 Feb 2017, at 00:13, Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:
>>
>> Samuel Thibault, on Fri 03 Feb 2017 01:11:44 +0100, wrote:
>>> see patch submissions on the linux kernel mailing list for examples of
>>> changelogs.
>>
>> And of course Documentation/SubmittingPatches.
>>
>> But really for now, learn about quilt, so that you get at ease with
>> changing code in the different patches of your series.
>>
>> Samuel
>
> Thanks all that is very helpful. I'll find out about quilt.

About to send this out as series of patches from quilt. That still
means major and minor numbers are fixed at 4, 64 and serial_in is
still the same. Those changes will be the next step.

Thanks,
Okash