lkml.org 
[lkml]   [2013]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 43/44] tty/metag_da: Add metag DA TTY driver

> >> +static int put_data(void *arg)
> >> +{
> >> + struct dashbuf *dbuf;
> >> + int number_written;
> >> +
> >> + __set_current_state(TASK_RUNNING);
> >> + while (!kthread_should_stop()) {
> >> + /*
> >> + * Pick up all the output buffers and write them out.
> >> + *
> >> + * FIXME: should we check with ASE how much room we have?
> >> + * Ideally, this will already have been done by write_room ??
> >> + */
> >
> >
> > No because your writer is asynchronous to your query so you need to
> > manage the difference yourself. You also need to get it right on the
> > queue length for close to work right.
>
> Regarding closing working right, do you mean making sure that all data
> in the asynchronous output buffer is written out by the final close
> (e.g. port_shutdown before freeing it)? Is that something the driver
> needs to do itself? (at the moment it does seem to be necessary, however
> I could be missing something)

You have no locking guaranteeing that the write_room is correct any given
put while put_data is running - both threads of execution can end up
changing it at the same time.

> >> + /* Determine the channel */
> >> + channel = FIRST_TTY_CHANNEL + tty->index;
> >> + dtty = dashtty_ttys[channel];
> >> + BUG_ON(!dtty);
> >
> > What is your locking model to prevent this ?
> > What is your reference counting model for the ttys - I don't see one.
>
> The new version has an array of dashtty_ports (not pointers, each
> containing a tty_port) and uses that to get at the output buffer.
>
> Would it need to ensure that the tty isn't hung up though, or is a write
> prevented from occurring in that case?

The core tty code will hold a reference to the tty object when calling
your methods which pass a "tty" pointer, so it won't vanish itself under
those conditions. For any other way of accessing you need to handle any
locking between write and hangup yourself - eg by deferring any clearing
down of the array until the tty_port itself is destroyed.

> Agreed and done. I didn't realise the TTY layer was so prone to
> providing lots of itty bitty fragments (even with a single 4k write from
> userland). The thing that really hurt was not the allocations but that
> each one was written out to the DA separately each incurring it's own
> large latency. Using a write buffer makes it a lot faster :)

Think about typing a command and the echo - you get a lot of very small
writes !

> It's still possible that the debugger isn't draining the DA buffers in
> which case they'll fill up, and the output buffer in the driver will
> fill and then the space reported to the tty layer will drop to 0.

At that point the tty layer will throttle.

>
> >
> >> +}
> >
> >> +static const struct tty_operations dashtty_ops = {
> >> + .open = dashtty_open,
> >> + .close = dashtty_close,
> >> + .write = dashtty_write,
> >
> > You need a hangup method. You may need a termios method.
>
> Is it sufficient for a hangup method to just call tty_port_hangup or
> should it drop the contents of the output buffer too? (as opposed to
> waiting for it to drain in port_shutdown).

The behaviour there is undefined - up to you, whatever makes sense.

> As far as I can tell none of the termios stuff is really relevant to
> this driver.

In which case the default behaviour will be fine - and it will allow non
hardware parameters to change but not hardware ones like baud rates.

> >> + channel_driver->init_termios = tty_std_termios;
> >> + channel_driver->init_termios.c_cflag =
> >> + B38400 | CS8 | CREAD | HUPCL | CLOCAL;
> >> + channel_driver->flags = TTY_DRIVER_REAL_RAW;
> >
> > Need to set the speed flags
>
> Do you mean c_ispeed, and c_ospeed? These are set in tty_std_termios,
> and they aren't meaningful in the context of this driver anyway, so are
> they still necessary?

You update c_cflag to B38400 so you should set c_ispeed/c_ospeed
accordingly. For a virtual interface it really only exists so that
applications have an answer. You want a higher speed like 38400 so that
apps don't try and do clever stuff for low speed links, that is all
really.

Alan


\
 
 \ /
  Last update: 2013-01-04 18:42    [W:0.091 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site