diff mbox

more about serial ports: do they even work?

Message ID 49876547.1080904@cisco.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

David S. Ahern Feb. 2, 2009, 9:27 p.m. UTC 
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:


Is this approach palatable to folks?

david


Michael Tokarev wrote:
> Michael Tokarev ?????:
>> 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 .
>>
>> 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])
> 
> Here's the possible solution (NotAPAtch(tm)):
> 
> In kvm-xx/qemu/qemu-char.c:
> 
>     case CHR_IOCTL_SERIAL_SET_TIOCM:
>         {
>             int sarg = *(int *)arg;
>             int targ = 0;        <==== change this 0 to 0x4000
>             if (sarg | CHR_TIOCM_DTR)
>                 targ |= TIOCM_DTR;
>             if (sarg | CHR_TIOCM_RTS)
>                 targ |= TIOCM_RTS;
>             ioctl(s->fd_in, TIOCMSET, &targ);
>         }
>         break;
> 
> This is obviously a hack, esp. since this bit is not
> always present even on linux (after reading 8250.c
> driver).
> 
> Real fix will be, I guess, to read the full set
> first, and combine it with DTR|RTS received from
> guest, something like this:
> 
>     case
>         {
>             int sarg = *(int *)arg;
>             int targ = 0;
>             ioctl(s->fd_in, TIOCMGET, &targ);
>             if (!(sarg | CHR_TIOCM_DTR))
>                 targ &= ~TIOCM_DTR;
>             if (!(sarg | CHR_TIOCM_RTS))
>                 targ ~= ~TIOCM_RTS;
>             ioctl(s->fd_in, TIOCMSET, &targ);
>         }
>         break;
> 
> I.e., to always keep all the other bits, but
> allow changing DTR and RTS.
> 
> Again, I don't know how it's linux-specific, but
> it seems the solution above should work on other
> platforms just fine.
> 
> /mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Michael Tokarev Feb. 2, 2009, 9:32 p.m. UTC | #1 
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);

>              if (sarg | CHR_TIOCM_DTR)
>                  targ |= TIOCM_DTR;
>              if (sarg | CHR_TIOCM_RTS)
>                  targ |= TIOCM_RTS;
>              ioctl(s->fd_in, TIOCMSET, &targ);
>          }
>          break;
>      default:
>          return -ENOTSUP;
>      }
> 
> Is this approach palatable to folks?
> 
> david

/mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David S. Ahern Feb. 2, 2009, 9:36 p.m. UTC | #2 
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.

david


>>              if (sarg | CHR_TIOCM_DTR)
>>                  targ |= TIOCM_DTR;
>>              if (sarg | CHR_TIOCM_RTS)
>>                  targ |= TIOCM_RTS;
>>              ioctl(s->fd_in, TIOCMSET, &targ);
>>          }
>>          break;
>>      default:
>>          return -ENOTSUP;
>>      }
>>
>> Is this approach palatable to folks?
>>
>> david
> 
> /mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Tokarev Feb. 3, 2009, 8:13 a.m. UTC | #3 
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;

Without that line above, the code never drops the two bits, once
set they can't be "removed" anymore.

By the way, this is upstream qemu issue, not kvm one, and has to be
pushed as such.  Good you CC'd qemu list.

/mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Marshall Feb. 3, 2009, 8:41 a.m. UTC | #4 
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).

MM

> 
> Without that line above, the code never drops the two bits, once
> set they can't be "removed" anymore.
> 
> By the way, this is upstream qemu issue, not kvm one, and has to be
> pushed as such.  Good you CC'd qemu list.
> 
> /mjt
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefano Stabellini Feb. 4, 2009, 11:17 a.m. UTC | #5 
Michael Tokarev wrote:

> David S. Ahern wrote:
>> Michael Tokarev wrote:
>>> David S. Ahern wrote:
>>>>      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;
> 
> Without that line above, the code never drops the two bits, once
> set they can't be "removed" anymore.
> 
> By the way, this is upstream qemu issue, not kvm one, and has to be
> pushed as such.  Good you CC'd qemu list.
> 


Either the two lines above or we could parse the whole set of possible
flags, like we do in the CHR_IOCTL_SERIAL_GET_TIOCM case.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- 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);
             if (sarg | CHR_TIOCM_DTR)
                 targ |= TIOCM_DTR;
             if (sarg | CHR_TIOCM_RTS)
                 targ |= TIOCM_RTS;
             ioctl(s->fd_in, TIOCMSET, &targ);
         }
         break;
     default:
         return -ENOTSUP;
     }