On Fri, 27 Jan 2012 13:04:37 +0100
richard -rw- weinberger [off-list ref] wrote:

On Fri, Jan 27, 2012 at 12:51 PM, Alan Cox [off-list ref] wrote:

quoted

quoted

UML's console driver (arch/um/drivers/line.c) implements tty_operations.
The crash happens because the tty subsystem calls the driver's close()
function and later
write_room() or chars_in_buffer().

write_room() and chars_in_buffer() fail badly because close() already
cleaned up the driver's private data...

You don't want to do that.

That's what i thought.

quoted

quoted

Greg, is UML's assumption wrong that after closing the tty no call to
write_room() or chars_in_buffer() can happen?
I have no idea why systemd is able to trigger this, UML's console
driver is old and has always worked quite well.

It's always been untrue but it's even more untrue nowdays. The tty layer
objects are refcounted, and the code has had significant rewrites. line.c
hidden away in uml hasn't been updated.

I added a comment about 3 years ago pointing out another older change
that was needed and that wasn't acted on either..

Take a look at how all the other tty drivers use tty_port, how the ioctls
have been supposed to work for the past few years and the callback
changes, then use them.

Can you recommend a well-written driver?

drivers/mmc/card/sdio_uart.c

uses just about all the features including handling hotplug and stuff you
don't need.

drivers/usb/serial/usb-serial.c

may also be handy as it provides the interface but then calls into other
driver code to do the work.

Basically though you want a struct tty_port in your private data, either
created at open, or usually more cleanly for the physical port lifetime

	tty_port_init()

Sets it up, then set the port ops

	tty_port_open()
	tty_port_close()
	tty_port_hangup()

do almost all of the rest of the work for you. They call back to your
activate and shutdown port methods, they serialize them, they call them
on first open/last close in matching pairs.

For the tty itself

	tty_port_tty_get()

gets you a reference to the tty from the port (or NULL) - so handles a
close/hangup racing with data arrival

	tty_kref_put()

releases a reference

and 
	tty->ops->cleanup()

is called on the final destruction of the tty object (ie its where you
can free tty lifetime data in tty->private_data)

So for a simple non pluggable tty it tends to look like

	int my_tty_open(struct tty_struct *tty, struct file *filp)
	{
		tty->driver_data = &my_port;
		return tty_port_open(&my_port, tty, filp);
	}

	void my_tty_close(struct tty_struct *tty, struct file *filp)
	{
		struct my_tty *m = tty->driver_data;
		if (m == NULL)
			return;
		tty_port_close(&m->port, tty, filp);
	}

	void my_tty_hangup(struct tty_struct *tty)
	{
		struct my_tty *m = tty->driver_data;
		tty_port_hangup(&m->port);
	}

provide the needed callbacks and it'll do the locking and the like for
you.

On the ioctl side as far as I can see you should simply get rid of the
method entirely.

For buffer_data you might want to allocate the buffer sanely at open time
(tty_port has a function for this too) so it can't fail weirdly

And your termios method is a bit odd but makes sense if you are just
pretending anything works and is supported.

Alan