Heads Up: Next Batch Of Serial/TTY Changes

Heads Up: Next Batch Of Serial/TTY Changes

[Alan Cox]

Firstly some architecture maintainers still haven't updated their
platform for arbitary tty speeds. The kernel is going to start whining
and issuing warnings on your platform if you don't keep up with the
programme (its been 6 months).

Secondly a lot of driver level code for termios methods is very very wrong
indeed. I've been trawling through and fixing the USB stuff in part but
some of it will need maintainer attention once the next changes go in.

- The termios interface says that you hand back the actual mode you set.
This means that if your driver sets a different speed, can't do the
selected parity etc you *MUST* update the passed tty->termios to reflect
your hardware. Mostly this seems to be the lack of support for mark/space
parity but if you've got a very limited specialist device you may need to
write a lot more (indeed the empeg effectively writes the entire termios)

- If your hardware has no termios features don't provide a dummy handler
just don't provide one. In that case the kernel will (as of updates) do
the right thing and preserve the previous speed and other hardware
parameters while updating the software ones.

- We now support seperate input and output speeds. Hardware that does
this wants updating accordingly

- If your hardware is initializing termios structures or copying and
editing them itself it is responsible for encoding c_ispeed/c_ospeed.
Right now there are some hacks to do it for you if you forget in most
cases, they will go away.

- The new code will add two types of helper to deal with the more horrible
bits of all this

	tty_encode_baud_rate(tty, inputbaud, outputbaud)
	tty_termios_encode_baudrate(termios, inputbaud, outputbaud)

and also

	tty_termios_copy_hw(new, old)

which will copy all the hardware implemented bits and speed from old to
new, which may be useful if your hardware needs to copy the old state
over and implements very little.

The speed encoding routines know about Bfoo encoding, arbitary rates and
also provide error tolerances so that programs using the Bfoo style speed
setup generally get Bfoo not BOTHER responses. Please don't put hacks for
this in drivers - if a case breaks then let me know and I'll fix the
encoder centrally.

Once all this is in the kernel will then acquire correct POSIX semantics
and error the ioctl if no requested change can be made.

Next stop after that is likely to be the open/close/hangup path.

Alan

Re: Heads Up: Next Batch Of Serial/TTY Changes

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Fri, 31 Aug 2007 22:11:05 +0100

> Firstly some architecture maintainers still haven't updated their
> platform for arbitary tty speeds. The kernel is going to start whining
> and issuing warnings on your platform if you don't keep up with the
> programme (its been 6 months).

I took a look at this for sparc and I'm currently balking the same way
you did :-) The current bit usage on sparc just don't work properly
for what you're trying to do.

Re: Heads Up: Next Batch Of Serial/TTY Changes

[Alan Cox]

On Fri, 31 Aug 2007 14:41:15 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Date: Fri, 31 Aug 2007 22:11:05 +0100
> 
> > Firstly some architecture maintainers still haven't updated their
> > platform for arbitary tty speeds. The kernel is going to start whining
> > and issuing warnings on your platform if you don't keep up with the
> > programme (its been 6 months).
> 
> I took a look at this for sparc and I'm currently balking the same way
> you did :-) The current bit usage on sparc just don't work properly
> for what you're trying to do.

I don't see a real problem. You aren't using

	c_cflags & CBAUD = 0x00001000

so that could become BOTHER.

the input bits also appear to be reserved and free ?

Re: Heads Up: Next Batch Of Serial/TTY Changes

[Jeff Garzik]

Alan Cox wrote:
> On Fri, 31 Aug 2007 14:41:15 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>> Date: Fri, 31 Aug 2007 22:11:05 +0100
>>
>>> Firstly some architecture maintainers still haven't updated their
>>> platform for arbitary tty speeds. The kernel is going to start whining
>>> and issuing warnings on your platform if you don't keep up with the
>>> programme (its been 6 months).
>> I took a look at this for sparc and I'm currently balking the same way
>> you did :-) The current bit usage on sparc just don't work properly
>> for what you're trying to do.
> 
> I don't see a real problem. You aren't using
> 
> 	c_cflags & CBAUD = 0x00001000
> 
> so that could become BOTHER.

Pooh says:  oh, bother!

	Jeff

Re: Heads Up: Next Batch Of Serial/TTY Changes

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Fri, 31 Aug 2007 23:16:13 +0100

> I don't see a real problem. You aren't using
> 
> 	c_cflags & CBAUD = 0x00001000
> 
> so that could become BOTHER.
> 
> the input bits also appear to be reserved and free ?

Nevermind, I missed how you were doing the new termios2
struct.

I'm trying to wrap things up before jumping onto a plan early tomorrow
morning, but I still tried to whip together a patch and while mostly
straightforward I ran into a few problems.

n_tty_ioctl() for instance:

drivers/char/tty_ioctl.c:799: error: ^[$,1rx^[(Bstruct termios^[$,1ry^[(B has no member named ^[$,1rx^[(Bc_ispeed^[$,1ry^[(B

This is calling the copy interface that is supposed to be using
a termios2 when the new interfaces are defined, however:

		case TIOCGLCKTRMIOS:
			if (kernel_termios_to_user_termios((struct termios __user *)arg, real_tty->termios_locked))
				return -EFAULT;
			return 0;

This is going to write over the end of the userspace
structure by a few bytes, and wasn't caught by you yet
because the i386 implementation is simply copy_to_user()
which does zero type checking.

Who knows what other gremlins like this now live in the tree :-)

There was a similar spot a few lines down, both fixed
as follows:

====================
diff --git a/drivers/char/tty_ioctl.c b/drivers/char/tty_ioctl.c
index 3423e9e..4a8969c 100644
--- a/drivers/char/tty_ioctl.c
+++ b/drivers/char/tty_ioctl.c
@@ -796,14 +796,14 @@ int n_tty_ioctl(struct tty_struct * tty, struct file * file,
 				retval = inq_canon(tty);
 			return put_user(retval, (unsigned int __user *) arg);
 		case TIOCGLCKTRMIOS:
-			if (kernel_termios_to_user_termios((struct termios __user *)arg, real_tty->termios_locked))
+			if (kernel_termios_to_user_termios_1((struct termios __user *)arg, real_tty->termios_locked))
 				return -EFAULT;
 			return 0;
 
 		case TIOCSLCKTRMIOS:
 			if (!capable(CAP_SYS_ADMIN))
 				return -EPERM;
-			if (user_termios_to_kernel_termios(real_tty->termios_locked, (struct termios __user *) arg))
+			if (user_termios_to_kernel_termios_1(real_tty->termios_locked, (struct termios __user *) arg))
 				return -EFAULT;
 			return 0;
 
====================

And here is the sparc patch, could you please add it to
your serial queue?  Thanks Alan.

[SPARC]: Add support for arbitrary serial speeds.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/asm-sparc/ioctls.h b/include/asm-sparc/ioctls.h
index bdf77b0..058c206 100644
--- a/include/asm-sparc/ioctls.h
+++ b/include/asm-sparc/ioctls.h
@@ -15,6 +15,10 @@
 #define TCSETS		_IOW('T', 9, struct termios)
 #define TCSETSW		_IOW('T', 10, struct termios)
 #define TCSETSF		_IOW('T', 11, struct termios)
+#define TCGETS2		_IOR('T', 12, struct termios2)
+#define TCSETS2		_IOW('T', 13, struct termios2)
+#define TCSETSW2	_IOW('T', 14, struct termios2)
+#define TCSETSF2	_IOW('T', 15, struct termios2)
 
 /* Note that all the ioctls that are not available in Linux have a 
  * double underscore on the front to: a) avoid some programs to
diff --git a/include/asm-sparc/termbits.h b/include/asm-sparc/termbits.h
index 5eb00a1..90cf221 100644
--- a/include/asm-sparc/termbits.h
+++ b/include/asm-sparc/termbits.h
@@ -31,6 +31,18 @@ struct termios {
 #endif
 };
 
+struct termios2 {
+	tcflag_t c_iflag;		/* input mode flags */
+	tcflag_t c_oflag;		/* output mode flags */
+	tcflag_t c_cflag;		/* control mode flags */
+	tcflag_t c_lflag;		/* local mode flags */
+	cc_t c_line;			/* line discipline */
+	cc_t c_cc[NCCS];		/* control characters */
+	cc_t _x_cc[2];                  /* padding to match ktermios */
+	speed_t c_ispeed;		/* input speed */
+	speed_t c_ospeed;		/* output speed */
+};
+
 struct ktermios {
 	tcflag_t c_iflag;		/* input mode flags */
 	tcflag_t c_oflag;		/* output mode flags */
@@ -160,6 +172,7 @@ struct ktermios {
 #define CLOCAL	  0x00000800
 #define CBAUDEX   0x00001000
 /* We'll never see these speeds with the Zilogs, but for completeness... */
+#define  BOTHER   0x00001000
 #define  B57600   0x00001001
 #define  B115200  0x00001002
 #define  B230400  0x00001003
@@ -189,6 +202,8 @@ struct ktermios {
 #define CMSPAR	  0x40000000  /* mark or space (stick) parity */
 #define CRTSCTS	  0x80000000  /* flow control */
 
+#define IBSHIFT	  16		/* Shift from CBAUD to CIBAUD */
+
 /* c_lflag bits */
 #define ISIG	0x00000001
 #define ICANON	0x00000002
diff --git a/include/asm-sparc/termios.h b/include/asm-sparc/termios.h
index d767f20..ff16a8e 100644
--- a/include/asm-sparc/termios.h
+++ b/include/asm-sparc/termios.h
@@ -108,6 +108,48 @@ struct winsize {
 
 #define user_termios_to_kernel_termios(k, u) \
 ({ \
+	int err; \
+	err  = get_user((k)->c_iflag, &(u)->c_iflag); \
+	err |= get_user((k)->c_oflag, &(u)->c_oflag); \
+	err |= get_user((k)->c_cflag, &(u)->c_cflag); \
+	err |= get_user((k)->c_lflag, &(u)->c_lflag); \
+	err |= get_user((k)->c_line,  &(u)->c_line); \
+	err |= copy_from_user((k)->c_cc, (u)->c_cc, NCCS); \
+	if((k)->c_lflag & ICANON) { \
+		err |= get_user((k)->c_cc[VEOF], &(u)->c_cc[VEOF]); \
+		err |= get_user((k)->c_cc[VEOL], &(u)->c_cc[VEOL]); \
+	} else { \
+		err |= get_user((k)->c_cc[VMIN],  &(u)->c_cc[_VMIN]); \
+		err |= get_user((k)->c_cc[VTIME], &(u)->c_cc[_VTIME]); \
+	} \
+	err |= get_user((k)->c_ispeed,  &(u)->c_ispeed); \
+	err |= get_user((k)->c_ospeed,  &(u)->c_ospeed); \
+	err; \
+})
+
+#define kernel_termios_to_user_termios(u, k) \
+({ \
+	int err; \
+	err  = put_user((k)->c_iflag, &(u)->c_iflag); \
+	err |= put_user((k)->c_oflag, &(u)->c_oflag); \
+	err |= put_user((k)->c_cflag, &(u)->c_cflag); \
+	err |= put_user((k)->c_lflag, &(u)->c_lflag); \
+	err |= put_user((k)->c_line, &(u)->c_line); \
+	err |= copy_to_user((u)->c_cc, (k)->c_cc, NCCS); \
+	if(!((k)->c_lflag & ICANON)) { \
+		err |= put_user((k)->c_cc[VMIN],  &(u)->c_cc[_VMIN]); \
+		err |= put_user((k)->c_cc[VTIME], &(u)->c_cc[_VTIME]); \
+	} else { \
+		err |= put_user((k)->c_cc[VEOF], &(u)->c_cc[VEOF]); \
+		err |= put_user((k)->c_cc[VEOL], &(u)->c_cc[VEOL]); \
+	} \
+	err |= put_user((k)->c_ispeed, &(u)->c_ispeed); \
+	err |= put_user((k)->c_ospeed, &(u)->c_ospeed); \
+	err; \
+})
+
+#define user_termios_to_kernel_termios_1(k, u) \
+({ \
 	get_user((k)->c_iflag, &(u)->c_iflag); \
 	get_user((k)->c_oflag, &(u)->c_oflag); \
 	get_user((k)->c_cflag, &(u)->c_cflag); \
@@ -124,7 +166,7 @@ struct winsize {
 	0; \
 })
 
-#define kernel_termios_to_user_termios(u, k) \
+#define kernel_termios_to_user_termios_1(u, k) \
 ({ \
 	put_user((k)->c_iflag, &(u)->c_iflag); \
 	put_user((k)->c_oflag, &(u)->c_oflag); \
diff --git a/include/asm-sparc64/ioctls.h b/include/asm-sparc64/ioctls.h
index 2223b6d..083c9a0 100644
--- a/include/asm-sparc64/ioctls.h
+++ b/include/asm-sparc64/ioctls.h
@@ -16,6 +16,10 @@
 #define TCSETS		_IOW('T', 9, struct termios)
 #define TCSETSW		_IOW('T', 10, struct termios)
 #define TCSETSF		_IOW('T', 11, struct termios)
+#define TCGETS2		_IOR('T', 12, struct termios2)
+#define TCSETS2		_IOW('T', 13, struct termios2)
+#define TCSETSW2	_IOW('T', 14, struct termios2)
+#define TCSETSF2	_IOW('T', 15, struct termios2)
 
 /* Note that all the ioctls that are not available in Linux have a 
  * double underscore on the front to: a) avoid some programs to
diff --git a/include/asm-sparc64/termbits.h b/include/asm-sparc64/termbits.h
index 705cd44..ebe31c1 100644
--- a/include/asm-sparc64/termbits.h
+++ b/include/asm-sparc64/termbits.h
@@ -5,8 +5,6 @@
 
 typedef unsigned char   cc_t;
 typedef unsigned int    speed_t;
-
-/* XXX is this right for sparc64?  it was an unsigned long... XXX */
 typedef unsigned int    tcflag_t;
 
 #define NCC 8
@@ -33,6 +31,18 @@ struct termios {
 #endif
 };
 
+struct termios2 {
+	tcflag_t c_iflag;		/* input mode flags */
+	tcflag_t c_oflag;		/* output mode flags */
+	tcflag_t c_cflag;		/* control mode flags */
+	tcflag_t c_lflag;		/* local mode flags */
+	cc_t c_line;			/* line discipline */
+	cc_t c_cc[NCCS];		/* control characters */
+	cc_t _x_cc[2];                  /* padding to match ktermios */
+	speed_t c_ispeed;		/* input speed */
+	speed_t c_ospeed;		/* output speed */
+};
+
 struct ktermios {
 	tcflag_t c_iflag;		/* input mode flags */
 	tcflag_t c_oflag;		/* output mode flags */
@@ -161,6 +171,7 @@ struct ktermios {
 #define HUPCL	  0x00000400
 #define CLOCAL	  0x00000800
 #define CBAUDEX   0x00001000
+#define  BOTHER   0x00001000
 #define  B57600   0x00001001
 #define  B115200  0x00001002
 #define  B230400  0x00001003
@@ -190,6 +201,8 @@ struct ktermios {
 #define CMSPAR    0x40000000  /* mark or space (stick) parity */
 #define CRTSCTS	  0x80000000  /* flow control */
 
+#define IBSHIFT	  16		/* Shift from CBAUD to CIBAUD */
+
 /* c_lflag bits */
 #define ISIG	0x00000001
 #define ICANON	0x00000002
diff --git a/include/asm-sparc64/termios.h b/include/asm-sparc64/termios.h
index f05d390..ef52721 100644
--- a/include/asm-sparc64/termios.h
+++ b/include/asm-sparc64/termios.h
@@ -123,6 +123,8 @@ struct winsize {
 		err |= get_user((k)->c_cc[VMIN],  &(u)->c_cc[_VMIN]); \
 		err |= get_user((k)->c_cc[VTIME], &(u)->c_cc[_VTIME]); \
 	} \
+	err |= get_user((k)->c_ispeed,  &(u)->c_ispeed); \
+	err |= get_user((k)->c_ospeed,  &(u)->c_ospeed); \
 	err; \
 })
 
@@ -142,6 +144,46 @@ struct winsize {
 		err |= put_user((k)->c_cc[VEOF], &(u)->c_cc[VEOF]); \
 		err |= put_user((k)->c_cc[VEOL], &(u)->c_cc[VEOL]); \
 	} \
+	err |= put_user((k)->c_ispeed, &(u)->c_ispeed); \
+	err |= put_user((k)->c_ospeed, &(u)->c_ospeed); \
+	err; \
+})
+
+#define user_termios_to_kernel_termios_1(k, u) \
+({ \
+	int err; \
+	err  = get_user((k)->c_iflag, &(u)->c_iflag); \
+	err |= get_user((k)->c_oflag, &(u)->c_oflag); \
+	err |= get_user((k)->c_cflag, &(u)->c_cflag); \
+	err |= get_user((k)->c_lflag, &(u)->c_lflag); \
+	err |= get_user((k)->c_line,  &(u)->c_line); \
+	err |= copy_from_user((k)->c_cc, (u)->c_cc, NCCS); \
+	if((k)->c_lflag & ICANON) { \
+		err |= get_user((k)->c_cc[VEOF], &(u)->c_cc[VEOF]); \
+		err |= get_user((k)->c_cc[VEOL], &(u)->c_cc[VEOL]); \
+	} else { \
+		err |= get_user((k)->c_cc[VMIN],  &(u)->c_cc[_VMIN]); \
+		err |= get_user((k)->c_cc[VTIME], &(u)->c_cc[_VTIME]); \
+	} \
+	err; \
+})
+
+#define kernel_termios_to_user_termios_1(u, k) \
+({ \
+	int err; \
+	err  = put_user((k)->c_iflag, &(u)->c_iflag); \
+	err |= put_user((k)->c_oflag, &(u)->c_oflag); \
+	err |= put_user((k)->c_cflag, &(u)->c_cflag); \
+	err |= put_user((k)->c_lflag, &(u)->c_lflag); \
+	err |= put_user((k)->c_line, &(u)->c_line); \
+	err |= copy_to_user((u)->c_cc, (k)->c_cc, NCCS); \
+	if(!((k)->c_lflag & ICANON)) { \
+		err |= put_user((k)->c_cc[VMIN],  &(u)->c_cc[_VMIN]); \
+		err |= put_user((k)->c_cc[VTIME], &(u)->c_cc[_VTIME]); \
+	} else { \
+		err |= put_user((k)->c_cc[VEOF], &(u)->c_cc[VEOF]); \
+		err |= put_user((k)->c_cc[VEOL], &(u)->c_cc[VEOL]); \
+	} \
 	err; \
 })
 

Re: Heads Up: Next Batch Of Serial/TTY Changes

[Alan Cox]

> Who knows what other gremlins like this now live in the tree :-)
> 
> There was a similar spot a few lines down, both fixed
> as follows:

Thanks I'll take a harder look over those. My test suite didn't check
them just the main termios ioctls didn't scribble.

> And here is the sparc patch, could you please add it to
> your serial queue?  Thanks Alan.
> 
> [SPARC]: Add support for arbitrary serial speeds.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>cc[VEOF]); \


Thanks

Alan