[PATCH, finally]: more about serial ports: do they even work?

[Michael Tokarev <mjt@tls.msk.ru>]

[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]

Michael Tokarev wrote:
> After some debugging and debugging, with a help
> Hollis Blanchard on #kvm@freenode, I discovered
> that kvm (or, rather, qemu) does not work correctly
> with serial ports, at least on linux.  One problem
> report has already here, author Cc'd -- see e.g.
> http://marc.info/?l=kvm&m=122995568009533&w=2 .

... [quoted in full below]...

Ok, It's a real shame to see SO many wrong attempts to
do it all, with so many idiotic mistakes all over...
But c'est la vie, it seems... ;)

So here we go.  Attached is a patch that fixes two
problems with serial ports &qemu (yes it's a qemu
issue, and, as far as I can see, both probs were
here for a long time).

First is completely f*cked up flags reporting and
setup for TIOCMGET and TIOCMSET ioctls, where ALL
known flags were reported and set in case at least
one flag is set, misusing "if(a|b) foo" instead of
"if(a&b) foo" -- just a typo I assume, but heck of
a typo... ;)

And second - for TIOCMSET it preserves other, unknown
flags.  Which fixes the problem that started it all,
since there was a bit set internally in kernel which,
when unset, makes serial port non-working, but TIOCMSET
dropped all "other" bits on the floor.

And for this second one, I'm still unsure.  The patch
I'm sending only tries to remove TIOCM_DTR and _RTS
bits (RTS is useless since it's controlled by the connected
device, isn't it?), leaving all others, incl., say,
CAR, RI, CTS etc, in place.  The question is -- some
of those bits are "input" lines, i.e., the ones controlled
by the attached device, and I don't know if all platforms
will ignore those instead of reporting error.  I.e, maybe
filter also those who are known "inputs"?  And while we're
at it, still, how about RTS?

Signed-off-By: Michael Tokarev <mjt@tls.msk.ru>

Thanks!

/ashamed mjt

--- original content follows ---
> Here's what's going on.
> 
> When opening a host's port, kvm resets the status
> lines, doing this:
> 
> ioctl(13, TIOCMGET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_DSR|0x4000])
> ioctl(13, TIOCMSET, [TIOCM_DTR|TIOCM_RTS])
> 
> which results in the following set
> 
> ioctl(13, TIOCMGET, [TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_DSR])
> 
> Note the difference between the default set and new one: the
> missing bit, 0x4000, which is unknown to strace(1) but is defined
> as TIOCM_OUT2 in linux headers.
> 
> After that change (resetting the TIOCM_OUT2 bit), no writes
> to the serial port works anymore, they're all gets accepted
> by host kernel and are buffered in the kernel.
> 
> After some time, when the kernel buffer fills up, and since
> the port (on host side) is opened in non-blocking mode, the
> writes starts returning EAGAIN, and kvm process starts
> endless loop, which were seen by David.
> 
> Here's the trivial program to demonstrate the idea:
> 
> ---- cut ---
> #include <sys/types.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <sys/ioctl.h>
> #include <termios.h>
> 
> int main(int argc, char **argv) {
>   fd = open("/dev/ttyS0", O_RDWR|O_NONBLOCK);
>   fcntl(fd, F_SETFL, O_RDWR);
>   x = TIOCM_DTR|TIOCM_RTS|TIOCM_CTS|TIOCM_DSR // |0x4000
>   ;
>   ioctl(fd, TIOCMSET, &x);
>   ioctl(fd, TIOCMGET, &x);
>   write(fd, "at\r", 3);
>   read(fd, buf, 20);
>   close(fd);
> 
>   return 0;
> }
> --- cut ---
> 
> 
> Run it under strace while a dialup modem is connected to /dev/ttyS0
> (i used this way for testing).  It will stuck at read, and nothing
> will be written, even when write() will happily return 3.  Un-comment
> the |0x4000 thing, and it will work.
> 
> I'm not sure what should be done with this, and how much this is
> linux-specific.  But it is obvious that bit (TIOCM_OUT2) should
> be left in-place (after which the thing works), at least on linux.
> 
> Note that this bit is NOT shown in /proc/tty/driver/serial file
> (which shows other bits).
> 
> Note also that this file (/proc/tty/driver/serial) helps to see
> if any write were performed: compare the counters.  In 'tx'
> there's number of bytes actually sent to device, as opposed to
> accepted by the kernel.  When you write something to /dev/ttyS0,
> that number increases, IF that something actually reached the
> device.
> 
> Thanks.
> 
> /mjt


[-- Attachment #2: qemu-serial.diff --]
[-- Type: text/x-patch, Size: 1513 bytes --]

--- kvm-83/qemu/qemu-char.c.orig	2009-01-13 16:29:42.000000000 +0300
+++ kvm-83/qemu/qemu-char.c	2009-02-05 21:19:35.972015110 +0300
@@ -1067,17 +1067,17 @@ static int tty_serial_ioctl(CharDriverSt
             int *targ = (int *)arg;
             ioctl(s->fd_in, TIOCMGET, &sarg);
             *targ = 0;
-            if (sarg | TIOCM_CTS)
+            if (sarg & TIOCM_CTS)
                 *targ |= CHR_TIOCM_CTS;
-            if (sarg | TIOCM_CAR)
+            if (sarg & TIOCM_CAR)
                 *targ |= CHR_TIOCM_CAR;
-            if (sarg | TIOCM_DSR)
+            if (sarg & TIOCM_DSR)
                 *targ |= CHR_TIOCM_DSR;
-            if (sarg | TIOCM_RI)
+            if (sarg & TIOCM_RI)
                 *targ |= CHR_TIOCM_RI;
-            if (sarg | TIOCM_DTR)
+            if (sarg & TIOCM_DTR)
                 *targ |= CHR_TIOCM_DTR;
-            if (sarg | TIOCM_RTS)
+            if (sarg & TIOCM_RTS)
                 *targ |= CHR_TIOCM_RTS;
         }
         break;
@@ -1085,9 +1085,11 @@ static int tty_serial_ioctl(CharDriverSt
         {
             int sarg = *(int *)arg;
             int targ = 0;
-            if (sarg | CHR_TIOCM_DTR)
+            ioctl(s->fd_in, TIOCMGET, &targ);
+            targ &= ~(TIOCM_DTR|TIOCM_RTS);
+            if (sarg & CHR_TIOCM_DTR)
                 targ |= TIOCM_DTR;
-            if (sarg | CHR_TIOCM_RTS)
+            if (sarg & CHR_TIOCM_RTS)
                 targ |= TIOCM_RTS;
             ioctl(s->fd_in, TIOCMSET, &targ);
         }