Date:	Sat, 28 Jan 2012 15:11:58 +0100
From:	richard -rw- weinberger <richard.weinberger@...il.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Kay Sievers <kay.sievers@...y.org>, Greg KH <gregkh@...e.de>,
	stian@...ia.no, LKML <linux-kernel@...r.kernel.org>,
	Linux-Arch <linux-arch@...r.kernel.org>,
	user-mode-linux-devel@...ts.sourceforge.net
Subject: Re: [uml-devel] /sys/class/tty/tty0/active?

On Sat, Jan 28, 2012 at 12:55 AM, richard -rw- weinberger
<richard.weinberger@...il.com> wrote:
> On Fri, Jan 27, 2012 at 3:02 PM, Alan Cox <alan@...rguk.ukuu.org.uk> wrote:
>> On Fri, 27 Jan 2012 13:04:37 +0100
>> richard -rw- weinberger <richard.weinberger@...il.com> wrote:
>>
>>> On Fri, Jan 27, 2012 at 12:51 PM, Alan Cox <alan@...rguk.ukuu.org.uk> wrote:
>>> >> 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.
>>>
>>> >> 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.
>
> Thanks for your help!
> tty_port makes sense and I think I understood it.
>
> But after modifying the driver to use tty_port the kernel crashes in
> tty_io.c:__tty_fasync() because filp->f_path.dentry is NULL.
>
> Any idea which kind of error can cause this?

Never mind. :-)

UML's line driver works fine now.
But it needs some more cleanups.
Especially the serial line and management console code.

-- 
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists