serious termios flaw?

[Till Straumann]

Thomas Doerfler wrote:

>-----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.
>  
>
Agreed. Note that the current code only works on hardware
that can buffer an entire packet (worst case).

>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)
>  
>
Hmm - I don't think so. pppstart is the line discipline 'start
method'. It is only called either a) from ISR context (in interrupt
driven mode, from termios_dequeue_characters() or
b) from the termios txdaemon (in task driven mode)

There is another routine 'pppasyncstart()' that is used to
kick-off transmission of a packet. It notifies  the
ppp TX task (not to be confused with the
serial/termios txdaemon) which then uses the device
write function to write the first char out. The following
interrupt then initiates a sequence of
termios_dequeue_characters - pppstart to do the bulk
of the transmission.

What doesn't seem to be properly locked is the
device 'write' function which is called from different
contexts (ppp TX daemon and pppstart (i.e., ISR or
termios txdaemon, depending on the mode and
then some other places in termios but those
are probably unused if ppp is active [except
maybe for flow-control]).

The consequence could be at worst some lost characters.

>- - 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,
>
well, in task-driven mode it merely posts to the task which
then does the work.

> 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.
>
As I pointed out, I believe this is not the case.

> 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.
>  
>
I have the impression that the device 'write' function could
be called from different contexts and one of them might
find the device not ready to send.

>- - 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".
>
>  
>
That's THE problem.

>- - An additional issue is, that the interaction for interrupt-driven
>device drivers will differ from polled drivers
>  
>
Well - leaving those aside for the moment. Dunno if it makes
much sense to use PPP in polled mode.

>- - 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.
>  
>
True - but for the many systems w/o FIFO/DMA it's as
good as it gets anyways.

>- ------------------------
>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.
>
That's an idea worthwile exploring. Unfortunately, my time
for playing with ppp is up.

> 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?
>  
>
Probably - but I can't do it myself, I'm sorry.

Regards
-- T.

>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-----
>  
>