On Fri, May 31, 2019 at 06:48:48PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> TL;DR: The key question in this thread is, if a driver should drain fifo
> and transmitter in .set_termios or not.
>
> On Fri, May 31, 2019 at 07:15:21PM +0300, Sergey Organov wrote:
> > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> > > On Fri, May 31, 2019 at 09:14:56AM +0300, Sergey Organov wrote:
> > >> Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> > >>
> > >> > On Thu, May 30, 2019 at 06:29:46PM +0300, Sergey Organov wrote:
> > >> >> imx_set_termios(): remove busy-waiting "drain Tx FIFO" loop. Worse
> > >> >> yet, it was potentially unbounded wait due to RTS/CTS (hardware)
> > >> >> handshake.
> > >> >>
> > >> >> Let user space ensure draining is done before termios change, if
> > >> >> draining is needed in the first place.
> > >> >
> > >> > I don't know for sure what the intended behaviour is here, but I tend to
> > >> > think that changing the unbounded wait to a timeout and then return
> > >> > -EBUSY (?) would be more suitable.
> > >>
> > >> No, please! Bytes in Tx FIFO are not an excuse to exit with error
> > >> instead of setting new termios as asked to.
> > >
> > > Well, my suggestion is more defensive. It at least tells the user that
> > > they do something wrong. If they already care for having the FIFO and
> > > transmitter empty before changing the baud rate the behaviour of both
> > > your and my approach are identical. With yours however it undefined if
> > > characters written to the device before the change are sent with the old
> > > or new settings. So my suggestions yields a deterministic behaviour
> > > which is good. And it tells the user when they do something wrong, which
> > > is good, too.
> > >
> > >> > With your change you're possibly breaking existent software.
> > >>
> > >> Well, I suspect the software is already broken then, as most widely used
> > >> drivers out there seem to do no Tx FIFO draining on set_termios() call,
> > >> or do they?
> > >>
> > >> I mean I tried to find similar code in some of the other drivers, to
> > >> replicate it, but I failed to find one.
> > >
> > > The first (and only) driver I checked does. (sa1100.c)
> >
> > I think I'd rather take 8250 as reference implementation, as being most
> > widely used. Can anybody please tell how 8250 code handles this? Does it
> > attempt to drain Tx FIFO on termios changes?
>
> Well, there are so many 8250 variants that the driver is rather
> complicated. Also the original 8250 doesn't have a FIFO at all.
>
> Given that it might not be so easy to judge if a given driver drains the
> FIFO and transmitter without consulting the reference manual I'd rather
> rely on an authority for the serial core. (Apart from that I bet we're
> finding examples for both variants.)
>
> @gregkh, rmk: What do you think?
We generally follow POSIX general terminal interface for TTY stuff.
The behaviour when setting termios is set out very clearly there,
and it's up to the application to decide how what it wants: it
may be important for the serial protocol to conform exactly to this.
This is what POSIX says:
The tcsetattr() function shall set the parameters associated with the
terminal referred to by the open file descriptor fildes (an open file
descriptor associated with a terminal) from the termios structure
referenced by termios_p as follows:
* If optional_actions is TCSANOW, the change shall occur immediately.
* If optional_actions is TCSADRAIN, the change shall occur after all
output written to fildes is transmitted. This function should be
used when changing parameters that affect output.
* If optional_actions is TCSAFLUSH, the change shall occur after all
output written to fildes is transmitted, and all input so far
received but not read shall be discarded before the change is made.
This is very explicit, and is what we implement. These are implemented
via the TCSETSF*, TCSETSW* and TCSETS* ioctls(), which use
uart_wait_until_sent() to implement the "wait for output data written
to be transmitted". Low level drivers implement this via their
tx_empty() callback, which returns zero if the transmitter is not
empty, or non-zero if the transmitter is empty.
However, we bound that by 2x the amount of time to send a FIFO-full
worth of data.
Hence, there is no requirement to implement any transmiter draining
in the .set_termios method - the core code already handles that for
drivers according to the requests of the userspace program.
.set_termios does not have sufficient information to know whether
it should or should not drain the transmitter (the transmitter
should have been drained, except when a timeout situation occurs
or the userspace program requests an immediate change of termios.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
