Is tty->receive_room no longer usable w/ SMP?

Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

A couple serial drivers I maintain check the value of tty->receive_room
to decide the max number of bytes to pull out of the UART's receive
FIFO and shove into a flip buffer.

After checking tty->receive room to decide how many bytes to read, one
of the drivers uses this sequence:

 tty_prepare_flip_string_flags(...)
 <fill up char buffer and flag buffer>
 tty_flip_buffer_push(...)

The other uses

 for (i=0; i<count; ++i)
   uart_insert_char(...);
 tty_flip_buffer_push(...);   
 
But, starting with kernel 3.12.0, whenSMP is enabled, tty->receive_room
is always 0 and never changes.  With SMP disabled, it seems to work the
way it always has.

Is use of tty->receive room no longer supported for SMP kernels?

How _should_ a serial driver decide how many rx characters there are
room for?

-- 
Grant Edwards               grant.b.edwards        Yow! What I need is a
                                  at               MATURE RELATIONSHIP with a
                              gmail.com            FLOPPY DISK ...

Re: Is tty->receive_room no longer usable w/ SMP?

[Peter Hurley]

On 02/12/2014 05:43 PM, Grant Edwards wrote:
> A couple serial drivers I maintain check the value of tty->receive_room
> to decide the max number of bytes to pull out of the UART's receive
> FIFO and shove into a flip buffer.

tty->receive_room is not part of the driver<->tty core interface.

> After checking tty->receive room to decide how many bytes to read, one
> of the drivers uses this sequence:
>
>   tty_prepare_flip_string_flags(...)
       ^^^^^^^^^^^^
This was removed for 3.14.

>   <fill up char buffer and flag buffer>
>   tty_flip_buffer_push(...)
>
> The other uses
>
>   for (i=0; i<count; ++i)
>     uart_insert_char(...);
>   tty_flip_buffer_push(...);
>
> But, starting with kernel 3.12.0, whenSMP is enabled, tty->receive_room
> is always 0 and never changes.  With SMP disabled, it seems to work the
> way it always has.

Starting with 3.12, receive_room is only used for line disciplines that
don't use flow control between the tty core and the line discipline
receive_buf (only N_TTY uses flow control).

This is because the N_TTY flow-controlled interface is lockless and
tty->receive_room is not supportable locklessly.

> Is use of tty->receive room no longer supported for SMP kernels?

The use of tty->receive_room by drivers is not supported on any kernel.

> How _should_ a serial driver decide how many rx characters there are
> room for?

All of the flip buffer functions that reserve/use buffer space return
the space reserved/consumed. Is rx overflowing the flip buffers before
you can throttle the sender?

Regards,
Peter Hurley

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 02/12/2014 05:43 PM, Grant Edwards wrote:
>> A couple serial drivers I maintain check the value of tty->receive_room
>> to decide the max number of bytes to pull out of the UART's receive
>> FIFO and shove into a flip buffer.
>
> tty->receive_room is not part of the driver<->tty core interface.

OK, fair enough.  Where is the driver<->tty core interface documented?

>> After checking tty->receive room to decide how many bytes to read, one
>> of the drivers uses this sequence:
>>
>>   tty_prepare_flip_string_flags(...)
>        ^^^^^^^^^^^^
> This was removed for 3.14.

Why? 

What is the replacement?

> The use of tty->receive_room by drivers is not supported on any
> kernel.

Got it.

>> How _should_ a serial driver decide how many rx characters there are
>> room for?
>
> All of the flip buffer functions that reserve/use buffer space return
> the space reserved/consumed. Is rx overflowing the flip buffers
> before you can throttle the sender?

Well, it certainly didn't when I used tty->receive_room to decide how
much data to read from the UART's rx FIFO. :)

I _was_ planning on removing the code that checks tty->receive_room
and instead rely instead on the return value from
tty_prepare_flip_string_flags().  But, as you noted, that's gone now.

Is there any documentation explaining how one is supposed to handle rx
data in a serial driver?  The topic doesn't seem to be mentioned at
all in Documentation/serial/driver.

If you can assume all rx data is TTY_NORMAL, then it looks like this
is the right way to do it:

  n = tty_prepare_flip_string()
  <copy n data bytes>  
  tty_flip_buffer_push()
    
What do you do if you can't assume all rx data is going to have a flag
value of TTY_NORMAL?

According to the comments in tty_buffer.c you can't use
tty_buffer_space_avail(), instead you're supposed to use
tty_prepare_flip_string(), but you can't use that unless you can
assume all rx data will be TTY_NORMAL.

Now that tty_prepare_flip_string_flags() is gone, it looks like
tty_insert_flip_string_flags() is the only option for transferring 
blocks of data/flags.  For that you need to know ahead of time how
many bytes can be guaranteed to be transferred because once you've
read data from the UART's rx FIFO it's lost if it can't be
transferred.

AFAICT, tty_buffer_request_room() can't be used in the case where you
you're passing a flags buffer: it can only be used to request a buffer
where all data will be TTY_NORMAL.

I've tried transferring one byte/flag pair at a time, but there are
two problems with that approach:

  1) You still need to know ahead of time how many bytes you can read
     from the UART's rx FIFO.

  2) All those calls with a large number of of ports running at high
     baud rates sucks up a _lot_ of CPU time.
     
-- 
Grant

Re: Is tty->receive_room no longer usable w/ SMP?

[Peter Hurley]

[ +cc Greg Kroah-Hartman]

On 02/12/2014 09:27 PM, Grant Edwards wrote:
> On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 02/12/2014 05:43 PM, Grant Edwards wrote:
>>> A couple serial drivers I maintain check the value of tty->receive_room
>>> to decide the max number of bytes to pull out of the UART's receive
>>> FIFO and shove into a flip buffer.
>>
>> tty->receive_room is not part of the driver<->tty core interface.
>
> OK, fair enough.  Where is the driver<->tty core interface documented?

Exported functions and the data mentioned in Documentation/serial/tty.txt,
like certain flag bits (eg., TTY_IO_ERROR).

But the documentation could use some modernization. Eg., I don't think
there's any docs on tty_port other than the example in-tree drivers.

>>> After checking tty->receive room to decide how many bytes to read, one
>>> of the drivers uses this sequence:
>>>
>>>    tty_prepare_flip_string_flags(...)
>>         ^^^^^^^^^^^^
>> This was removed for 3.14.
>
> Why?

The tty flip buffer code now uses an encoding method to maximize
memory usage for actual data (when the data is all TTY_NORMAL).
Since there were no in-tree users, it was pointless to rewrite the function.

> What is the replacement?

There isn't one. No in-tree drivers use this function.

>> The use of tty->receive_room by drivers is not supported on any
>> kernel.
>
> Got it.
>
>>> How _should_ a serial driver decide how many rx characters there are
>>> room for?
>>
>> All of the flip buffer functions that reserve/use buffer space return
>> the space reserved/consumed. Is rx overflowing the flip buffers
>> before you can throttle the sender?
>
> Well, it certainly didn't when I used tty->receive_room to decide how
> much data to read from the UART's rx FIFO. :)
>
> I _was_ planning on removing the code that checks tty->receive_room
> and instead rely instead on the return value from
> tty_prepare_flip_string_flags().  But, as you noted, that's gone now.

That's one of the hazards of out-of-tree drivers, for you and for me.
When I'm writing functionality, I can't know if some defunct code is
actually being used in the wild. For you, breakage ensues.

> Is there any documentation explaining how one is supposed to handle rx
> data in a serial driver?  The topic doesn't seem to be mentioned at
> all in Documentation/serial/driver.

This has been and continues to be one of the most active areas of
development with the tty subsystem, especially to support really high
speed tty (50+MB/sec).

> If you can assume all rx data is TTY_NORMAL, then it looks like this
> is the right way to do it:
>
>    n = tty_prepare_flip_string()
>    <copy n data bytes>
>    tty_flip_buffer_push()
>
> What do you do if you can't assume all rx data is going to have a flag
> value of TTY_NORMAL?

ATM (meaning in 3.14-rc1/2) there's no direct replacement for
tty_prepare_flip_string_flags() but see below.

> According to the comments in tty_buffer.c you can't use
> tty_buffer_space_avail(), instead you're supposed to use
> tty_prepare_flip_string(), but you can't use that unless you can
> assume all rx data will be TTY_NORMAL.
>
> Now that tty_prepare_flip_string_flags() is gone, it looks like
> tty_insert_flip_string_flags() is the only option for transferring
> blocks of data/flags.  For that you need to know ahead of time how
> many bytes can be guaranteed to be transferred because once you've
> read data from the UART's rx FIFO it's lost if it can't be
> transferred.
>
> AFAICT, tty_buffer_request_room() can't be used in the case where you
> you're passing a flags buffer: it can only be used to request a buffer
> where all data will be TTY_NORMAL.

Although tty_buffer_request_room() can be used in this situation
(because it does return a buffer suitable for 1/2 data + 1/2 flags),
I'd rather you didn't - see below.

> I've tried transferring one byte/flag pair at a time

Don't do that.

At this point, I would prefer to code up a tty_prepare_flip_string_flags(),
than have out-of-tree drivers hacking things up.

The thing is:
1. I'll have to convince Greg and he's not a fan of out-of-tree drivers :)
2. rc2 is pretty late for an exported function that will have almost no testing.
But I'll do what I can do.

You can help avoid this situation in the future by testing linux-next
(or at least Greg's tty-next tree if you only work on serial stuff) where
these tty buffer changes have been sitting for 2 months (since Dec 8).

Or better yet, submit your driver(s) for in-tree inclusion. That way
the situation doesn't happen in the first place.

Regards,
Peter Hurley

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:

>>>>    tty_prepare_flip_string_flags(...)
>>>         ^^^^^^^^^^^^
>>> This was removed for 3.14.
>>
>> Why?
>
> The tty flip buffer code now uses an encoding method to maximize
> memory usage for actual data (when the data is all TTY_NORMAL). Since
> there were no in-tree users,

I noticed that earlier this evening when browsing the in-kernel
drivers.

>> I _was_ planning on removing the code that checks tty->receive_room
>> and instead rely instead on the return value from
>> tty_prepare_flip_string_flags().  But, as you noted, that's gone now.
>
> That's one of the hazards of out-of-tree drivers, for you and for me.
> When I'm writing functionality, I can't know if some defunct code is
> actually being used in the wild. For you, breakage ensues.

Indeed.

>> Is there any documentation explaining how one is supposed to handle
>> rx data in a serial driver?  The topic doesn't seem to be mentioned
>> at all in Documentation/serial/driver.
>
> This has been and continues to be one of the most active areas of
> development with the tty subsystem, especially to support really high
> speed tty (50+MB/sec).

Thankfully, my drivers don't have to support baud rates that high. 
Our serial ports top out at 921K bits/sec, but in theory we support up
to 256 ports.  In practice I've never heard of anybody with more than
128 ports, and speeds above 115K are pretty rare -- but just running a
few dozen ports at 115K starts to add up.

>> If you can assume all rx data is TTY_NORMAL, then it looks like this
>> is the right way to do it:
>>
>>    n = tty_prepare_flip_string()
>>    <copy n data bytes>
>>    tty_flip_buffer_push()

Should I assume that this is the preferred method if I can know that I
have a specific amount of TTY_NORMAL data available?

>> I've tried transferring one byte/flag pair at a time
>
> Don't do that.

I'll have to do a little more investigating, but I think I can detect
the case where there are no error flags present in the rx FIFO, so I
can use the prepare/push method when that's true (which it should be
the vast majority of the time).

If I can do that, then I think I could live with tty_insert_char() in
the much rarer case where there are errors present -- but I still need
to know if there's room to insert a byte before reading the byte/flag
from the rx FIFO and subsequently calling tty_insert_char().

That's the part I don't understand:

How are drivers that use tty_insert_char() supposed to know when to
stop calling it?  tty_insert_char() doesn't return a status, so you
don't know even after the fact if the byte you just passed was
accepted. Do you?

Similarly, how are drivers that use tty_insert_flip_string_flags()
supposed to know how much data they can pass to it?

Are they expected to keep reading data from the rx FIFO and passing it
to tty_insert_flip_string_flags() until it returns a value smaller
than the requested transfer size and then buffer the "overflow" data
that has already been read from the rx FIFO but not accepted by
tty_insert_flip_string_flags()?  Then at some point in the future you
retry the old, buffered data, and once that's all been transferred you
start reading from the rx FIFO again? That would work, but it's a
little ugly.

Looking at existing in-tree users of tty_insert_flip_string_flags(),
they call tty_buffer_request_room() to determine how many char/flag
pairs can be transferred and then depend on the assumption that
tty_insert_flip_string_flags() will accept that much. I didn't think
that was valid because tty_buffer_request_room() assumes you're not
going to transfer flags -- doesn't it?

I must be missing something.

> At this point, I would prefer to code up a
> tty_prepare_flip_string_flags(), than have out-of-tree drivers
> hacking things up.

I would be perfectly happy using tty_insert_flip_string_flags()
instead of tty_prepare_flip_string_flags() if I knew how many
char/flag pairs I can pass to it.

> The thing is:
>
> 1. I'll have to convince Greg and he's not a fan of out-of-tree
>    drivers :)

I know.

> 2. rc2 is pretty late for an exported function that will have almost
>    no testing. But I'll do what I can do.

> You can help avoid this situation in the future by testing linux-next
> (or at least Greg's tty-next tree if you only work on serial stuff)
> where these tty buffer changes have been sitting for 2 months (since
> Dec 8).

Point well taken.  I'll try get get resources allocated for periodic
testing against linux-next.  Currently, we don't start testing with a
kernel until an "rc" version comes out.

> Or better yet, submit your driver(s) for in-tree inclusion. That way
> the situation doesn't happen in the first place.

We've tried in-kernel drivers before, but it may be time to look into
it again.

<sob-story>
In the past it didn't work out very well: maintaining both in-tree
drivers and out-of-tree drivers was too much work.  The problem with
in-tree drivers is that the only kernel version supported with fixes
and new features is "the next version". Unfortunately, none of our
customers are ever running "the next version". [One steady customer
just switched from 2.4 to 2.6 within the past year.] So even when
there is an in-tree driver, we still have to maintain a separate
out-of-tree driver the same way we would if there weren't an in-tree
driver.

Since the out-of-tree driver has to support a wide range of kernel
versions, the code had to be written in ways that are unacceptable for
an in-tree driver. As a result, the in-tree and out-of-tree drivers
diverge.  Every feature has to be added twice, bugs have to be fixed
twice, there's twice as much QA, twice as much documentation, etc.  On
top of that there's the additional work of getting those fixes and
changes _into_ the in-tree driver.

After struggling with that for a while, it was decided that the
benefits gained from maintaining a set of in-tree drivers weren't
enough to justify all additional work involved.  Whenever it's been
mentioned subsequently the response is something on the lines of "we
fell for that once, we're not going to fall for it again."
</sob-story>

-- 
Grant

Re: Is tty->receive_room no longer usable w/ SMP?

[Peter Hurley]

On 02/13/2014 12:38 AM, Grant Edwards wrote:
> On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> If you can assume all rx data is TTY_NORMAL, then it looks like this
>>> is the right way to do it:
>>>
>>>     n = tty_prepare_flip_string()
>>>     <copy n data bytes>
>>>     tty_flip_buffer_push()
>
> Should I assume that this is the preferred method if I can know that I
> have a specific amount of TTY_NORMAL data available?

Yes.

>>> I've tried transferring one byte/flag pair at a time
>>
>> Don't do that.
>
> I'll have to do a little more investigating, but I think I can detect
> the case where there are no error flags present in the rx FIFO, so I
> can use the prepare/push method when that's true (which it should be
> the vast majority of the time).
>
> If I can do that, then I think I could live with tty_insert_char() in
> the much rarer case where there are errors present -- but I still need
> to know if there's room to insert a byte before reading the byte/flag
> from the rx FIFO and subsequently calling tty_insert_char().

Ok.

> That's the part I don't understand:
>
> How are drivers that use tty_insert_char() supposed to know when to
> stop calling it?  tty_insert_char() doesn't return a status, so you
> don't know even after the fact if the byte you just passed was
> accepted. Do you?

What tree are you looking at?

include/linux/tty_flip.h:tty_insert_flip_char() takes 1 byte and 1 flag
and returns how many bytes were consumed (ie., 0 or 1). If it returned
0, the data was not accepted.

> Similarly, how are drivers that use tty_insert_flip_string_flags()
> supposed to know how much data they can pass to it?

Drivers pass _all_ of the rx data to tty_insert_flip_string_flags(),
which returns how much was accepted. Eg.,

         c = tty_insert_flip_string_flags(&my_port->port, rx_data, flags, n_rx);
         if (c < 0) {
                 /* actual error - none of the data was taken up */
         }
         if (c < n_rx) {
                 /* tty buffer did not take up all the data */
         }

> Are they expected to keep reading data from the rx FIFO and passing it
> to tty_insert_flip_string_flags() until it returns a value smaller
> than the requested transfer size and then buffer the "overflow" data
> that has already been read from the rx FIFO but not accepted by
> tty_insert_flip_string_flags()?  Then at some point in the future you
> retry the old, buffered data, and once that's all been transferred you
> start reading from the rx FIFO again? That would work, but it's a
> little ugly.

While this approach is possible, there is another way to look at
this problem:

The tty buffers are very deep (at least 64K+, as high as 128K).
If data can't be taken in but the sender has not yet been throttled,
then some error is occurring. Rather than buffering data
outside the tty buffers (because eventually that will run out too
causing a fifo overflow), just drop the current rx and signal a
condition to insert a TTY_OVERRUN at the beginning of the next rx
(look at drivers/staging/fwserial/fwserial.c:fwtty_rx() -- but
don't use tty_buffer_space_avail() throttling part; that's only
for very high speeds where the tty buffers can overflow even
before the input processing worker runs).

In addition, the tty buffer space is now configurable per-port at
initialization time, so you could monitor the tty_buffer_space_avail()
at each rx time and if it is below a given watermark, emit a
diagnostic. Then update your driver to add more buffer space and/or
look to see if there is some other problem further in the rx chain,
like the app not pulling enough data per read, etc.

You could use this monitoring purely in testing or continue to use
it in the wild, so if there is some previously unknown high watermark,
you're alerted and can fix it without there having been data loss.

> Looking at existing in-tree users of tty_insert_flip_string_flags(),
> they call tty_buffer_request_room() to determine how many char/flag
> pairs can be transferred and then depend on the assumption that
> tty_insert_flip_string_flags() will accept that much. I didn't think
> that was valid because tty_buffer_request_room() assumes you're not
> going to transfer flags -- doesn't it?
>
> I must be missing something.

tty_buffer_request_room() returns a buffer suitable for data + flags;
you're just misreading the code.

>> At this point, I would prefer to code up a
>> tty_prepare_flip_string_flags(), than have out-of-tree drivers
>> hacking things up.
>
> I would be perfectly happy using tty_insert_flip_string_flags()
> instead of tty_prepare_flip_string_flags() if I knew how many
> char/flag pairs I can pass to it.
>
>> The thing is:
>>
>> 1. I'll have to convince Greg and he's not a fan of out-of-tree
>>     drivers :)
>
> I know.
>
>> 2. rc2 is pretty late for an exported function that will have almost
>>     no testing. But I'll do what I can do.
>
>> You can help avoid this situation in the future by testing linux-next
>> (or at least Greg's tty-next tree if you only work on serial stuff)
>> where these tty buffer changes have been sitting for 2 months (since
>> Dec 8).
>
> Point well taken.  I'll try get get resources allocated for periodic
> testing against linux-next.  Currently, we don't start testing with a
> kernel until an "rc" version comes out.

At least build against it; that's automatable.

>> Or better yet, submit your driver(s) for in-tree inclusion. That way
>> the situation doesn't happen in the first place.
>
> We've tried in-kernel drivers before, but it may be time to look into
> it again.
>
> <sob-story>
> In the past it didn't work out very well: maintaining both in-tree
> drivers and out-of-tree drivers was too much work.  The problem with
> in-tree drivers is that the only kernel version supported with fixes
> and new features is "the next version". Unfortunately, none of our
> customers are ever running "the next version". [One steady customer
> just switched from 2.4 to 2.6 within the past year.] So even when
> there is an in-tree driver, we still have to maintain a separate
> out-of-tree driver the same way we would if there weren't an in-tree
> driver.
>
> Since the out-of-tree driver has to support a wide range of kernel
> versions, the code had to be written in ways that are unacceptable for
> an in-tree driver. As a result, the in-tree and out-of-tree drivers
> diverge.  Every feature has to be added twice, bugs have to be fixed
> twice, there's twice as much QA, twice as much documentation, etc.  On
> top of that there's the additional work of getting those fixes and
> changes _into_ the in-tree driver.
>
> After struggling with that for a while, it was decided that the
> benefits gained from maintaining a set of in-tree drivers weren't
> enough to justify all additional work involved.  Whenever it's been
> mentioned subsequently the response is something on the lines of "we
> fell for that once, we're not going to fall for it again."
> </sob-story>

Yeah, that sucks.

[
   I don't mean to be critical but I can't really see that development
   model being sustainable. There's no realistic way to single-source
   a driver across hundreds of kernel versions.

   What was still being "maintained" for your driver for a 2.4 kernel
   (since any other active development there stopped years ago)?
]

One way to manage an in-kernel driver is to only have it support a
minimum/common feature set. The advantage is you get automatic patches
when the core changes -- and I don't just mean tty core but also
locking, workqueues, etc -- which you can then pull into the out-of-tree
driver (still adhering to the license requirements, of course). You
just maintain the actual hardware changes/bug fixes.

You have to deal with superseding the in-tree driver with the out-of-tree
driver when both are present, but that's not insurmountable.

[ For my own edification, why is the driver not a serial mini-port? ]

Regards,
Peter Hurley

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 02/13/2014 12:38 AM, Grant Edwards wrote:

>> That's the part I don't understand:
>>
>> How are drivers that use tty_insert_char() supposed to know when to
>> stop calling it?  tty_insert_char() doesn't return a status, so you
>> don't know even after the fact if the byte you just passed was
>> accepted. Do you?
>
> What tree are you looking at?

Rats.  I wasn't looking at tty_insert_char(), I was looking at
uart_insert_char() since that's what all the other drivers use.

> include/linux/tty_flip.h:tty_insert_flip_char() takes 1 byte and 1 flag
> and returns how many bytes were consumed (ie., 0 or 1). If it returned
> 0, the data was not accepted.

Right.

>> Similarly, how are drivers that use tty_insert_flip_string_flags()
>> supposed to know how much data they can pass to it?
>
> Drivers pass _all_ of the rx data to tty_insert_flip_string_flags(),

Except they don't.

They call tty_buffer_request_room() to decide how much data to pull
out of the rx FIFO/buffer.  Then they read only that much rx data and
pass it to tty_insert_flip_string_flags() ignoring the return value,
because the rx data has passed the point of no return. Once you pull
data out of the rx FIFO/buffer, you've got to do something with it.
If you can't transfer it to the line discipline layer, things get ugly
quickly.

> which returns how much was accepted. Eg.,
>
>          c = tty_insert_flip_string_flags(&my_port->port, rx_data, flags, n_rx);
>          if (c < 0) {
>                  /* actual error - none of the data was taken up */
>          }
>          if (c < n_rx) {
>                  /* tty buffer did not take up all the data */
>          }

That's not how tty_insert_flip_string_flags() is used by in-tree
drivers (at least not in 3.14rc2).

>> Are they expected to keep reading data from the rx FIFO and passing it
>> to tty_insert_flip_string_flags() until it returns a value smaller
>> than the requested transfer size and then buffer the "overflow" data
>> that has already been read from the rx FIFO but not accepted by
>> tty_insert_flip_string_flags()?  Then at some point in the future you
>> retry the old, buffered data, and once that's all been transferred you
>> start reading from the rx FIFO again? That would work, but it's a
>> little ugly.
>
> While this approach is possible, there is another way to look at
> this problem:
>
> The tty buffers are very deep (at least 64K+, as high as 128K).
> If data can't be taken in but the sender has not yet been throttled,
> then some error is occurring.

Not necessarily.

With modern UARTs, the way you throttle the sender is by not reading
data from the rx FIFO when there's nowhere to put that data.  When the
rx FIFO starts getting too full, the UART will send an XOFF or assirt
or de-assert RTS or DTR or whatever.

In order for that mechanism to work, you need to know when to stop
reading the rx FIFO.  That's what we had been using tty->receive_room
for, and now it appears I should be using tty_buffer_request_room()
for.

> Rather than buffering data outside the tty buffers (because
> eventually that will run out too causing a fifo overflow),

Not if flow control is enabled.  When you end up with data buffered in
the driver, the driver stops reading the rx FIFO, the rx FIFO fills
up, and the UART throttles the sender.  Once the buffered data has
been transferred, you start read the rx FIFO again and the UART
unthrottles the sender.  If flow control _isn't_ enable, and the
application isn't reading receive data, then there's going to be an
overflow error and data is lost no matter what approach you take. It
makes a lot more sense to let the UART do the overflow error detection
and discard the data than to do that in driver code.

> just drop the current rx and signal a condition to insert a
> TTY_OVERRUN at the beginning of the next rx (look at
> drivers/staging/fwserial/fwserial.c:fwtty_rx().

That seems like writing driver code to emulate functions that already
exist in the UART hardware.  I'd much rather just use the UART for
that.

> but don't use tty_buffer_space_avail() throttling part; that's only
> for very high speeds where the tty buffers can overflow even before
> the input processing worker runs).

The whole point of knowing when to stop reading the rx FIFO is to
_prevent_ data loss/overrun by allowing the UART to throttle the
sender before we get to the point where we've got nowhere to put data
we've read from the rx FIFO.

>> I must be missing something.
>
> tty_buffer_request_room() returns a buffer suitable for data + flags;
> you're just misreading the code.

Great!

That solves my problem.  I can call tty_buffer_request_room(), read
the indicated number of chars or char/flag pairs and then depend on
being able to call _either_ tty_insert_flip_string() or
tty_insert_flip_string_flags() to transfer the data I read.

When the application stops reading data and buffers fill up,
tty_buffer_request_room() will tell me to stop reading data from the
UART and everything will work the way it should.

> At least build against it; that's automatable.

Good point.  That would catch the obvious stuff with very little work.

> I don't mean to be critical but I can't really see that development
> model being sustainable.

We've been doing it for 20+ years (though I've only been involved for
the last 15).  We can't beat the Chinese on the price of boards, so we
had better beat them on support.

> There's no realistic way to single-source a driver across hundreds of
> kernel versions.

Maybe not hundreds.  Our current drivers only support 2.6.24 and
later.  We can still support previous driver versions if required for
customers running older 2.6 kernels.  We officially stopped supporting
2.4 several years ago.

> What was still being "maintained" for your driver for a 2.4 kernel
> (since any other active development there stopped years ago)?

We hadn't supported 2.4 for several years at that point -- I was just
illustrating how reluctant many customers are to upgrade kernels and
the infeasibility of depending on in-tree drivers to support customers
like that.

> One way to manage an in-kernel driver is to only have it support a
> minimum/common feature set. The advantage is you get automatic
> patches when the core changes -- and I don't just mean tty core but
> also locking, workqueues, etc -- which you can then pull into the
> out-of-tree driver (still adhering to the license requirements, of
> course). You just maintain the actual hardware changes/bug fixes.

A couple of our products do have in-tree drivers, but they've diverged
so far from the out-of-tree drivers that they aren't particularly
useful resources for maintaining the out-of-tree drivers.  It's
actually been more useful over the years to look at changes made to
other in-tree drivers.

> You have to deal with superseding the in-tree driver with the out-of-tree
> driver when both are present, but that's not insurmountable.

That doesn't seem to be an issue.  It's only been a problem a couple
times in the past couple decades when a customer has built a custom
configured kernel and configured the in-tree driver as built-in rather
than a module.

> [ For my own edification, why is the driver not a serial mini-port? ]

I don't know what a serial mini-port is.  We recently transitioned two
of our drivers from being tty drivers to being serial-core drivers.
Is mini-port something different that the serial core?

For another driver, it's still a tty driver because the serial-core
API just didn't fit the device well enough to make it work.  One issue
I recall is that our driver for that device needs to be able to cause
specific error returns for write() calls that are made when the
hardware is unavailable (and I think there are different errors
depending on why it's unavailable).

-- 
Grant Edwards               grant.b.edwards        Yow! BARBARA STANWYCK makes
                                  at               me nervous!!
                              gmail.com            

Re: Is tty->receive_room no longer usable w/ SMP?

[Peter Hurley]

On 02/13/2014 12:52 PM, Grant Edwards wrote:
> On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
>> On 02/13/2014 12:38 AM, Grant Edwards wrote:
>> While this approach is possible, there is another way to look at
>> this problem:
>>
>> The tty buffers are very deep (at least 64K+, as high as 128K).
>> If data can't be taken in but the sender has not yet been throttled,
>> then some error is occurring.
>
> Not necessarily.
>
> With modern UARTs, the way you throttle the sender is by not reading
> data from the rx FIFO when there's nowhere to put that data.  When the
> rx FIFO starts getting too full, the UART will send an XOFF or assirt
> or de-assert RTS or DTR or whatever.
>
> In order for that mechanism to work, you need to know when to stop
> reading the rx FIFO.  That's what we had been using tty->receive_room
> for, and now it appears I should be using tty_buffer_request_room()
> for.
>
>> Rather than buffering data outside the tty buffers (because
>> eventually that will run out too causing a fifo overflow),
>
> Not if flow control is enabled.  When you end up with data buffered in
> the driver, the driver stops reading the rx FIFO, the rx FIFO fills
> up, and the UART throttles the sender.  Once the buffered data has
> been transferred, you start read the rx FIFO again and the UART
> unthrottles the sender.  If flow control _isn't_ enable, and the
> application isn't reading receive data, then there's going to be an
> overflow error and data is lost no matter what approach you take. It
> makes a lot more sense to let the UART do the overflow error detection
> and discard the data than to do that in driver code.

Ok. I assumed your hardware didn't do this because otherwise you wouldn't
need to use the tty_flip_* interface directly because your driver would
be a UART miniport (ie., serial-core port).

>> just drop the current rx and signal a condition to insert a
>> TTY_OVERRUN at the beginning of the next rx (look at
>> drivers/staging/fwserial/fwserial.c:fwtty_rx().
>
> That seems like writing driver code to emulate functions that already
> exist in the UART hardware.  I'd much rather just use the UART for
> that.

Again, I assumed your hardware didn't do this for you (which is why
I pointed you to an example of how to simulate it for hardware that
isn't a UART/doesn't auto-flow-control).

>> but don't use tty_buffer_space_avail() throttling part; that's only
>> for very high speeds where the tty buffers can overflow even before
>> the input processing worker runs).
>
> The whole point of knowing when to stop reading the rx FIFO is to
> _prevent_ data loss/overrun by allowing the UART to throttle the
> sender before we get to the point where we've got nowhere to put data
> we've read from the rx FIFO.
>
>>> I must be missing something.
>>
>> tty_buffer_request_room() returns a buffer suitable for data + flags;
>> you're just misreading the code.
>
> Great!
>
> That solves my problem.  I can call tty_buffer_request_room(), read
> the indicated number of chars or char/flag pairs and then depend on
> being able to call _either_ tty_insert_flip_string() or
> tty_insert_flip_string_flags() to transfer the data I read.

Yep.

> When the application stops reading data and buffers fill up,
> tty_buffer_request_room() will tell me to stop reading data from the
> UART and everything will work the way it should.
>
>> At least build against it; that's automatable.
>
> Good point.  That would catch the obvious stuff with very little work.
>
>> I don't mean to be critical but I can't really see that development
>> model being sustainable.
>
> We've been doing it for 20+ years (though I've only been involved for
> the last 15).  We can't beat the Chinese on the price of boards, so we
> had better beat them on support.

Just trying to be helpful. Please don't take my comments as an attack
on either your business model or your development processes; I don't
know enough about either to criticize.

>> There's no realistic way to single-source a driver across hundreds of
>> kernel versions.
>
> Maybe not hundreds.  Our current drivers only support 2.6.24 and
> later.  We can still support previous driver versions if required for
> customers running older 2.6 kernels.  We officially stopped supporting
> 2.4 several years ago.

By "support", do you mean "add new features" or "workaround hardware bugs"?

>> What was still being "maintained" for your driver for a 2.4 kernel
>> (since any other active development there stopped years ago)?
>
> We hadn't supported 2.4 for several years at that point -- I was just
> illustrating how reluctant many customers are to upgrade kernels and
> the infeasibility of depending on in-tree drivers to support customers
> like that.
>
>> One way to manage an in-kernel driver is to only have it support a
>> minimum/common feature set. The advantage is you get automatic
>> patches when the core changes -- and I don't just mean tty core but
>> also locking, workqueues, etc -- which you can then pull into the
>> out-of-tree driver (still adhering to the license requirements, of
>> course). You just maintain the actual hardware changes/bug fixes.
>
> A couple of our products do have in-tree drivers, but they've diverged
> so far from the out-of-tree drivers that they aren't particularly
> useful resources for maintaining the out-of-tree drivers.  It's
> actually been more useful over the years to look at changes made to
> other in-tree drivers.
>
>> You have to deal with superseding the in-tree driver with the out-of-tree
>> driver when both are present, but that's not insurmountable.
>
> That doesn't seem to be an issue.  It's only been a problem a couple
> times in the past couple decades when a customer has built a custom
> configured kernel and configured the in-tree driver as built-in rather
> than a module.
>
>> [ For my own edification, why is the driver not a serial mini-port? ]
>
> I don't know what a serial mini-port is.  We recently transitioned two
> of our drivers from being tty drivers to being serial-core drivers.
> Is mini-port something different that the serial core?

No, same thing.

> For another driver, it's still a tty driver because the serial-core
> API just didn't fit the device well enough to make it work.  One issue
> I recall is that our driver for that device needs to be able to cause
> specific error returns for write() calls that are made when the
> hardware is unavailable (and I think there are different errors
> depending on why it's unavailable).

And this is the driver that uses tty_flip_* interface directly?

Regards,
Peter Hurley

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> I must be missing something.
>>>
>>> tty_buffer_request_room() returns a buffer suitable for data + flags;
>>> you're just misreading the code.
>>
>> Great!
>>
>> That solves my problem.  I can call tty_buffer_request_room(), read
>> the indicated number of chars or char/flag pairs and then depend on
>> being able to call _either_ tty_insert_flip_string() or
>> tty_insert_flip_string_flags() to transfer the data I read.
>
> Yep.

Problem is solved.  Or rather more accurately: Problem was imaginary.

>>> I don't mean to be critical but I can't really see that development
>>> model being sustainable.
>>
>> We've been doing it for 20+ years (though I've only been involved for
>> the last 15).  We can't beat the Chinese on the price of boards, so we
>> had better beat them on support.
>
> Just trying to be helpful.

I realize that and I appreciate it.

> Please don't take my comments as an attack on either your business
> model or your development processes;

Of course not.

> I don't know enough about either to criticize.

Nonsense, this is the internet!

>>> There's no realistic way to single-source a driver across hundreds of
>>> kernel versions.
>>
>> Maybe not hundreds.  Our current drivers only support 2.6.24 and
>> later.  We can still support previous driver versions if required for
>> customers running older 2.6 kernels.  We officially stopped
>> supporting 2.4 several years ago.
>
> By "support", do you mean "add new features" or "workaround hardware
> bugs"?

Both.  "New features" mostly meant support for new models, but there
were also some actual new features and fixes for old ones that didn't
work.  For example, it turns out almost nobody on the planet uses
IXANY. It got left out of both our automated and manual regression
testing, and (embarassingly) it took 10+ years for somebody to realize
that it didn't work.  And that "somebody" was a customer that still
had 2.4 kenels running in production machines.


>>> [ For my own edification, why is the driver not a serial mini-port? ]
>>
>> I don't know what a serial mini-port is.  We recently transitioned
>> two of our drivers from being tty drivers to being serial-core
>> drivers. Is mini-port something different that the serial core?
>
> No, same thing.

Ah, good.

>> For another driver, it's still a tty driver because the serial-core
>> API just didn't fit the device well enough to make it work.  One
>> issue I recall is that our driver for that device needs to be able to
>> cause specific error returns for write() calls that are made when the
>> hardware is unavailable (and I think there are different errors
>> depending on why it's unavailable).
>
> And this is the driver that uses tty_flip_* interface directly?

Well, they all do (under some circumstances). AFAICT, the method
provided by serial core is uart_insert_char().  When we initially
moved from tty driver to serial driver, we did everything in the 
simplest, most obvious way (e.g. uart_insert_char()).  But we moved
away from that for performance reasons.  Our hardware is optimized for
transferring data to/from continuous buffers using 32-bit bus cycles
rather than reading/writing characters one at a time.  At one point in
history our drivers would use inline assembly to execute a single
machine instruction to write an entire block of tx data to the
hardware or read an entire block of data from the hardware.  There
were also some prototypes that did bus-master DMA for rx/tx data, but
that turned out to be more overhead than programmed I/O, and it was
abandoned.  Now that CPU speeds have increased so much faster than bus
speeds, the inline assembly is no longer needed, but making a function
call for every rx byte is still enough to use an alarming amount of
CPU time.

-- 
Grant Edwards               grant.b.edwards        Yow! I'm reporting for duty
                                  at               as a modern person.  I want
                              gmail.com            to do the Latin Hustle now!

Re: Is tty->receive_room no longer usable w/ SMP?

[Peter Hurley]

On 02/13/2014 01:50 PM, Grant Edwards wrote:
> On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
>> By "support", do you mean "add new features" or "workaround hardware
>> bugs"?
>
> Both.  "New features" mostly meant support for new models, but there
> were also some actual new features and fixes for old ones that didn't
> work.  For example, it turns out almost nobody on the planet uses
> IXANY. It got left out of both our automated and manual regression
> testing, and (embarassingly) it took 10+ years for somebody to realize
> that it didn't work.  And that "somebody" was a customer that still
> had 2.4 kenels running in production machines.

Not unusual. The N_TTY ldisc just had a bug fixed in 3.10 where turning off
IXON with an already-stopped tty permanently hung the tty.

>>> For another driver, it's still a tty driver because the serial-core
>>> API just didn't fit the device well enough to make it work.  One
>>> issue I recall is that our driver for that device needs to be able to
>>> cause specific error returns for write() calls that are made when the
>>> hardware is unavailable (and I think there are different errors
>>> depending on why it's unavailable).
>>
>> And this is the driver that uses tty_flip_* interface directly?
>
> Well, they all do (under some circumstances). AFAICT, the method
> provided by serial core is uart_insert_char().  When we initially
> moved from tty driver to serial driver, we did everything in the
> simplest, most obvious way (e.g. uart_insert_char()).  But we moved
> away from that for performance reasons.  Our hardware is optimized for
> transferring data to/from continuous buffers using 32-bit bus cycles
> rather than reading/writing characters one at a time.  At one point in
> history our drivers would use inline assembly to execute a single
> machine instruction to write an entire block of tx data to the
> hardware or read an entire block of data from the hardware.  There
> were also some prototypes that did bus-master DMA for rx/tx data, but
> that turned out to be more overhead than programmed I/O, and it was
> abandoned.  Now that CPU speeds have increased so much faster than bus
> speeds, the inline assembly is no longer needed, but making a function
> call for every rx byte is still enough to use an alarming amount of
> CPU time.

We should extend the serial-core to add rx methods that work
with your UARTs in their fastest and most native way. The idea behind
serial-core is only to abstract the busy-work away from the i/o, not
become an ill-fitting shoe that enforces The One True Way.

Regards,
Peter Hurley

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 02/13/2014 01:50 PM, Grant Edwards wrote:

>> For example, it turns out almost nobody on the planet uses IXANY. It
>> got left out of both our automated and manual regression testing, and
>> (embarassingly) it took 10+ years for somebody to realize that it
>> didn't work.  And that "somebody" was a customer that still had 2.4
>> kenels running in production machines.
>
> Not unusual. The N_TTY ldisc just had a bug fixed in 3.10 where
> turning off IXON with an already-stopped tty permanently hung the
> tty.

Yea, over the years, Xon/Xoff flow control is probably a larger source
of headaches than any other feature.  In fact, I think our serial core
drivers currently have to disable the UART's hardware Xon/Xoff
handling because the serial core API doesn't notify the driver when
the user makes a termios call to change the Xon/Xoff characters.  [Not
that anybody really uses that feature either...]

> We should extend the serial-core to add rx methods that work with
> your UARTs in their fastest and most native way. The idea behind
> serial-core is only to abstract the busy-work away from the i/o, not
> become an ill-fitting shoe that enforces The One True Way.

The prepare/push flip buffer API was pretty close to ideal.

It eliminates one set of buffer copy operations involved in the
room/insert/push method.  But, given the speed of in-memory buffer
copies, I doubt the difference is noticeable.

-- 
Grant Edwards               grant.b.edwards        Yow! for ARTIFICIAL
                                  at               FLAVORING!!
                              gmail.com            

Re: Is tty->receive_room no longer usable w/ SMP?

[Grant Edwards]

On 2014-02-13, Grant Edwards <grant.b.edwards@gmail.com> wrote:
> On 2014-02-13, Peter Hurley <peter@hurleysoftware.com> wrote:

>> There's no realistic way to single-source a driver across hundreds of
>> kernel versions.
>
> Maybe not hundreds.  Our current drivers only support 2.6.24 and
> later.

It turns out I was wrong on both counts. The oldest supported version
is 2.6.25, not 2.6.24, and yes that is hundreds of versions.  :)

I got curious and counted: we currently have single-source support for
541 official Linux kernel versions. That's not counting current "rc"
versions (which are now supported), or RedHat's backport abominiations
(which occasionally require special support).

Within those 541 versions there are only about five or six distinct
incompatible API subsets to worry about, so it's not as impressive as
it sounds.

-- 
Grant Edwards               grant.b.edwards        Yow! HOORAY, Ronald!!
                                  at               Now YOU can marry LINDA
                              gmail.com            RONSTADT too!!