Discussion:
[patch 0/6] staging: speakup: Introduce TTY-based comms
Okash Khawaja
2017-02-25 19:18:01 UTC
Permalink
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
Okash Khawaja
2017-02-25 19:21:32 UTC
Permalink
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 <***@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)
{ }
Samuel Thibault
2017-02-26 00:53:42 UTC
Permalink
Post by Okash Khawaja
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
Samuel Thibault
2017-02-26 01:05:43 UTC
Permalink
Post by Samuel Thibault
Post by Okash Khawaja
Allow access to TTY device from kernel.
When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
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
John Covici
2017-02-26 04:30:45 UTC
Permalink
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,
Post by Samuel Thibault
Post by Samuel Thibault
Post by Okash Khawaja
Allow access to TTY device from kernel.
When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
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
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
***@ccs.covici.com
Samuel Thibault
2017-02-26 10:25:58 UTC
Permalink
Post by 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?
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
Samuel Thibault
2017-05-17 07:45:46 UTC
Permalink
On Sun, Feb 26, 2017 at 1:05 AM, Samuel Thibault
Post by Samuel Thibault
Post by Samuel Thibault
Post by Okash Khawaja
Allow access to TTY device from kernel.
When opening the TTY from an application (e.g. echo foo > /dev/ttyS0),
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
Okash Khawaja
2017-05-17 13:38:32 UTC
Permalink
Post by Samuel Thibault
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
Samuel Thibault
2017-05-22 22:07:52 UTC
Permalink
Hello,
Post by Okash Khawaja
Post by Samuel Thibault
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
Okash Khawaja
2017-05-23 08:30:34 UTC
Permalink
Hi,

On Mon, May 22, 2017 at 11:07 PM, Samuel Thibault
Post by Samuel Thibault
Hello,
Post by Okash Khawaja
Post by Samuel Thibault
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-***@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)."
Post by Samuel Thibault
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
Okash Khawaja
2017-05-23 08:38:18 UTC
Permalink
On Tue, May 23, 2017 at 9:33 AM, Samuel Thibault
Post by Okash Khawaja
"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

Samuel Thibault
2017-02-26 01:34:09 UTC
Permalink
-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
Samuel Thibault
2017-02-26 03:27:00 UTC
Permalink
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
Okash Khawaja
2017-02-26 19:16:35 UTC
Permalink
On Sun, Feb 26, 2017 at 3:27 AM, Samuel Thibault
Post by 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);
}
Along the way, thinking of renaming serial_out to synth_out - agnostic
of actual mechanism: serial or ttyio.
Samuel Thibault
2017-02-26 19:29:08 UTC
Permalink
Post by Okash Khawaja
On Sun, Feb 26, 2017 at 3:27 AM, Samuel Thibault
Post by 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);
}
Along the way, thinking of renaming serial_out to synth_out - agnostic
of actual mechanism: serial or ttyio.
Indeed :)

Samuel
Trevor Astrope
2017-02-26 00:12:38 UTC
Permalink
Hi Okash,

Thanks for all the work you've done!

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

Trevor
Post by 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
_______________________________________________
Speakup mailing list
http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
Okash Khawaja
2017-02-26 09:43:05 UTC
Permalink
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
Post by 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
Post by 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
_______________________________________________
Speakup mailing list
http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
_______________________________________________
Speakup mailing list
http://linux-speakup.org/cgi-bin/mailman/listinfo/speakup
Samuel Thibault
2017-02-26 01:24:25 UTC
Permalink
Post by Okash Khawaja
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
Samuel Thibault
2017-02-26 01:31:06 UTC
Permalink
Post by Samuel Thibault
Post by Okash Khawaja
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
Samuel Thibault
2017-02-26 01:59:21 UTC
Permalink
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
Samuel Thibault
2017-02-26 03:38:11 UTC
Permalink
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
Okash Khawaja
2017-02-26 09:53:11 UTC
Permalink
Hi,

Thanks for the quick feedback and this helpful summary :)

On Sun, Feb 26, 2017 at 3:38 AM, Samuel Thibault
Post by 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)
Quilt has been very helpful in preparing the patches. Sending was
tedious without it but today got quilt mail --send working which is
great.
Post by Samuel Thibault
I also gave a lot of information, most of which are mostly notes for
- 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?
Post by Samuel Thibault
- 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
Samuel Thibault
2017-02-26 10:29:37 UTC
Permalink
Post by Okash Khawaja
Post by Samuel Thibault
- 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
Loading...