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

Previous thread: [GIT PULL] please pull infiniband.git for-linus branch by Roland Dreier on Friday, August 31, 2007 - 2:00 pm. (1 message)

Next thread: [PATCH 1/5] Add a missing 00-INDEX file for Documentation/vm/ by Jesper Juhl on Friday, August 31, 2007 - 2:20 pm. (1 message)
From: Alan Cox
Date: Friday, August 31, 2007 - 2:11 pm

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 ...
From: David Miller
Date: Friday, August 31, 2007 - 2:41 pm

From: Alan Cox <alan@lxorguk.ukuu.org.uk>

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.
-

From: Alan Cox
Date: Friday, August 31, 2007 - 3:16 pm

On Fri, 31 Aug 2007 14:41:15 -0700 (PDT)

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 ?

-

From: Jeff Garzik
Date: Friday, August 31, 2007 - 3:21 pm

Pooh says:  oh, bother!

	Jeff



-

From: David Miller
Date: Friday, August 31, 2007 - 3:23 pm

From: Alan Cox <alan@lxorguk.ukuu.org.uk>

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: $,1rxstruct termios$,1ry has no member named $,1rxc_ispeed$,1ry

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 ...
From: Alan Cox
Date: Friday, August 31, 2007 - 3:47 pm

Thanks I'll take a harder look over those. My test suite didn't check


Thanks

Alan
-

Previous thread: [GIT PULL] please pull infiniband.git for-linus branch by Roland Dreier on Friday, August 31, 2007 - 2:00 pm. (1 message)

Next thread: [PATCH 1/5] Add a missing 00-INDEX file for Documentation/vm/ by Jesper Juhl on Friday, August 31, 2007 - 2:20 pm. (1 message)