[Qemu-devel] Re: more about serial ports: do they even work?

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Feb 03, 2009 at 08:41:14AM +0000, Mark Marshall wrote:
> Michael Tokarev wrote:
>> David S. Ahern wrote:
>>> Michael Tokarev wrote:
>>>> David S. Ahern wrote:
>>>>> I don't recall seeing a followup to this post.
>>>>>
>>>>> To put Michael's second suggestion into patch form, the following fixes
>>>>> the problem for me:
>>>>>
>>>>> --- kvm-81.orig/qemu/qemu-char.c        2008-12-14 06:16:27.000000000 -0700
>>>>> +++ kvm-81/qemu/qemu-char.c     2009-02-02 14:12:20.000000000 -0700
>>>>> @@ -1078,20 +1078,21 @@
>>>>>              if (sarg | TIOCM_DTR)
>>>>>                  *targ |= CHR_TIOCM_DTR;
>>>>>              if (sarg | TIOCM_RTS)
>>>>>                  *targ |= CHR_TIOCM_RTS;
>>>>>          }
>>>>>          break;
>>>>>      case CHR_IOCTL_SERIAL_SET_TIOCM:
>>>>>          {
>>>>>              int sarg = *(int *)arg;
>>>>>              int targ = 0;
>>>>> +            ioctl(s->fd_in, TIOCMGET, &targ);
>>>> here, one more operation is necessary:
>>>>                targ &= ~(TIOCM_DTR|TIOCM_RTS);
>>> Interesting. that change was not needed to fix my problem.
>>
>> It just means you (or, rather, your guests) never really needed to
>> DROP those signal lines, only to raise them.
>>
>>>>>              if (sarg | CHR_TIOCM_DTR)
>>>>>                  targ |= TIOCM_DTR;
>>>>>              if (sarg | CHR_TIOCM_RTS)
>>>>>                  targ |= TIOCM_RTS;
> Is this code really correct.  If it is is there a comment somewhere  
> describing why it's correct?  I would have expected the lines above to 
> be:
>
>               if (sarg & CHR_TIOCM_DTR)
>                   targ |= TIOCM_DTR;
>               if (sarg & CHR_TIOCM_RTS)
>                   targ |= TIOCM_RTS;
>
> (Just an observation as this went past).

Right. I don't understand the point of converting to an "internal"
representation of TIOCM control bits. 

CHR_IOCTL_SERIAL_SET_TIOCM is clearly broken as mentioned. It should at 
least preserve the bits it does not control.

diff --git a/qemu/qemu-char.c b/qemu/qemu-char.c
index ac431c7..66971e1 100644
--- a/qemu/qemu-char.c
+++ b/qemu/qemu-char.c
@@ -1063,33 +1063,12 @@ static int tty_serial_ioctl(CharDriverState *chr, int cmd, void *arg)
         break;
     case CHR_IOCTL_SERIAL_GET_TIOCM:
         {
-            int sarg = 0;
-            int *targ = (int *)arg;
-            ioctl(s->fd_in, TIOCMGET, &sarg);
-            *targ = 0;
-            if (sarg | TIOCM_CTS)
-                *targ |= CHR_TIOCM_CTS;
-            if (sarg | TIOCM_CAR)
-                *targ |= CHR_TIOCM_CAR;
-            if (sarg | TIOCM_DSR)
-                *targ |= CHR_TIOCM_DSR;
-            if (sarg | TIOCM_RI)
-                *targ |= CHR_TIOCM_RI;
-            if (sarg | TIOCM_DTR)
-                *targ |= CHR_TIOCM_DTR;
-            if (sarg | TIOCM_RTS)
-                *targ |= CHR_TIOCM_RTS;
+            ioctl(s->fd_in, TIOCMGET, arg);
         }
         break;
     case CHR_IOCTL_SERIAL_SET_TIOCM:
         {
-            int sarg = *(int *)arg;
-            int targ = 0;
-            if (sarg | CHR_TIOCM_DTR)
-                targ |= TIOCM_DTR;
-            if (sarg | CHR_TIOCM_RTS)
-                targ |= TIOCM_RTS;
-            ioctl(s->fd_in, TIOCMSET, &targ);
+            ioctl(s->fd_in, TIOCMSET, arg);
         }
         break;
     default: