serious termios flaw?

[Thomas Doerfler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Till,

today I had a closer look to the interaction between ppp_tty.c and
termios.c. I think, your patch is not really a good solution, although
it is better than the current code.

IMHO these are the relevant functions:

ppp-tty.c: pppstart()
<device driver>: write and interrupt functions
termios.c: termios_dequeue_characters()

Let me write down my thoughts as they form in my mind:

These functions will be executed one after the other in a loop:

- - Some function calls pppstart (in task context)

- - pppstart determines which character(s) to send and calls the device
driver's write function: (*tp->device.write)(tp->minor, ...)
- - the device driver send one or more characters to the hardware device
and returns.

- - when the character(s) have been sent out, the device raises an
interrupt, the device driver's interrupt function is called and in turn
calls "termios_dequeue_characters" with the number of characters
actually sent.

- - termios_dequeue_characters is executed in interrupt context, counts up
the characters sent into tty->t_dqlen and calls the line discipline
"start" function (this is pppstart again) to send more characters

In my opinion, this has several consequences:

- - pppstart is called some times in task context and sometimes in
interrupt context. I am not quite sure, whether the instances called in
task context and in interrupt context are properly locked against each
other concerning variable usage. At a first glance, it might be that
this case cannot happen.

- - pppstart seems to keep track of the number of characters sent out in
its variable sc->sc_outoff. But there, it only counts the characters
actually passed to the device driver's "write" function, not the number
of characters actually reported by "termios_dequeue_characters".

- - An additional issue is, that the interaction for interrupt-driven
device drivers will differ from polled drivers

- - reducing the number of characters passed to the device driver to 1
would increase the processor load for many systems with FIFO/DMA
equipped UARTs, so I think this is not really an ultimate solution.

- ------------------------
My suggestion is to synchronize the variable "sc->outoff" with
"tty->t_dqlen". I think it would even be possible to replace the usage
of sc->sc_outoff with tp->t_dqlen in ppp_tty.c, with the fatal line

      sc->sc_outoff += ioffset;

being dropped completely. Escape character handling might be an issue in
this solution. Handling of polled drivers might be an additional issue,
but I don't think that using polled drivers in realtime systems for PPP
lines would be a really good solution ;-)

Till, do you see a way to modify ppp_tty.c in this direction?

wkr,
Thomas.





Till Straumann schrieb:
> Till Straumann wrote:
> 
>> Thomas.
>>
>> Thanks for clarifying.
>> Where I seem to have problems is 'ppp_tty.c:pppstart,
>> lines 637-648'.
>> I can't see how characters dropped by the 'write' routine
>> are saved and written later.
> 
> I'm even more confident now that this is true. I got lots of
> dropped packets (as a matter of fact: all packets were
> missing at least one char and were dropped for FCS mismatch).
> 
> Reducing the number of chars passed to 'write' fixed that.
> I have now filed PR1113 with a patch. Hopefully someone
> can look at it.
> 
> -- T.
> 
>>
>> Till
>>
>> Thomas Doerfler wrote:
>>
> Till,
> 
> I am not sure which mode of termios drivers you are referring to.
> Generally the termios driver API can pass multiple characters to a write
> routine. in polled mode, the driver's polled write function must send
> all given characters to the hardware. I can see this implemented e.g. in
> the "pc386" BSP in console.c:
> 
> static int
> ibmpc_console_write(int minor, const char *buf, int len)
> {
>  int count;
>  for (count = 0; count < len; count++)
>  {
>    _IBMPC_outch( buf[ count ] );
>    if( buf[ count ] == '\n')
>      _IBMPC_outch( '\r' );            /* LF = LF + CR */
>  }
>  return 0;
> }
> 
> In interrput mode, termios can pass a rather long buffer to the driver's
> write function, but the driver is not forced to handle that size.
> Instead, when the driver's interrupt function calls
> "rtems_termios_dequeue_characters", it passes to termios how many
> characters have been sent out to the device. Simple devices without FIFO
> or DMA support may always pass a "1" to this function, then termios will
> pass the rest of the send buffer once more to the device driver's write
> function.
> 
> wkr,
> Thomas.
> 
> 
> 
> 
> 
> Till Straumann schrieb:
>  
> 
>>>>> I found that termios (and, BTW: ppp_tty) call
>>>>> the low-level driver's 'write' routine, possibly supplying
>>>>> more than one character and without checking
>>>>> the return code of 'write'.
>>>>>
>>>>> Most UART drivers OTOH (I checked i386, ppc, uC5282[coldfire]),
>>>>> silently discard anything after the first char in the buffer.
>>>>> This is not reflected in the return code.
>>>>>
>>>>> IMO, the low-level drivers should return the # of chars
>>>>> actually written and the upper layers should check for and
>>>>> handle this case.
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>> Till
>>>>>   
> 
> 
> 
> --
> --------------------------------------------
> IMD Ingenieurbuero fuer Microcomputertechnik
> Thomas Doerfler           Herbststrasse 8
> D-82178 Puchheim          Germany
> email:    Thomas.Doerfler at imd-systems.de
> PGP public key available at:
>     http://www.imd-systems.de/pgpkey_en.html

>>>
>>
>>


- --
- --------------------------------------------
IMD Ingenieurbuero fuer Microcomputertechnik
Thomas Doerfler           Herbststrasse 8
D-82178 Puchheim          Germany
email:    Thomas.Doerfler at imd-systems.de
PGP public key available at:
     http://www.imd-systems.de/pgpkey_en.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEo4AIwHyg4bDtfjQRAq3vAJ9H4suZ5Rg7N0cnMfQvzcT7UJtRVQCfTw8l
9LdyjiJOfWyoBmouoQ4nUSw=
=DPfJ
-----END PGP SIGNATURE-----