Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 19 Dec 2015 05:20:14 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Eugene Grosbein <eugen@grosbein.net>
Cc:        freebsd-bugs@freebsd.org
Subject:   Re: [Bug 205398] [regression] [tty] tty_drain() kernel function lacks timeout support it had before
Message-ID:  <20151219042026.Y1835@besplex.bde.org>
In-Reply-To: <56743674.2090104@grosbein.net>
References:  <bug-205398-8@https.bugs.freebsd.org/bugzilla/> <20151219013916.N930@besplex.bde.org> <56743674.2090104@grosbein.net>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 18 Dec 2015, Eugene Grosbein wrote:

> 18.12.2015 23:05, Bruce Evans =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
>> ...
>> There is a hack for last-close that is supposed to give a hard-coded=20
>> timeout
>> of 1 second.  Not sure why this doesn't work for you.  My quick fix that
>> restores the timeout uses slightly different logic where this hack was.
>
> I've made a mistake (now corrected) while filling PR: my system is 9.3-ST=
ABLE
> and not 10.2-STABLE. It has no "leaving" case hack.

I forgot to check that before.  I first fixed this in FreeBSD-9, and that
till doesn't have the hack.  Later I merged my fix into FreeBSD-10 and
reorganized it to be more similar to the surrounding code.  The patch
in my previous mail is relative to FreeBSD-10.  Not much tty code has
changed since then, and I don't use my complete tty patch set for
-current (only parts for my drivers).

> ...
>> If the device is really disconnected, then the tty should be in a zombie
>> state and should not wait.  I think this still works.  CLOCAL or lack of
>> modem signals may break detection of last-close.
>
> The device does not get disconnected in process, it was not connected
> from the moment of open().
>
>> Did you have CRTSCTS flow control enabled?  This is probably the main
>> source of hangs.  The RTS and CTS signals are not ignored in CLOCAL mode=
,
>> flow control should be invoked when they go away when th device goes
>> away.
>
> It has both of CRTSCTS flow control and CLOCAL enabled and
> I'd like to keep them both enabled and working.

Reasonable, but I try to do the opposite for various reasons:
- drivers should be able to keep up with low speeds like 115200 bps
   provided the CPU is an i386 at 20 MHz or faster.  OS interrupt latency
   may break this.  Data loss without RTSCTS serves as a warning about
   OS breakage.  However, higher speeds are more of a problem (I test with
   32 times faster and that has an inter-char time of 2.5 usec), and
   sometimes it is the external device that needs the flow control.
- carrier detect (CD) is useful even for local connections.  Sometimes you
   actually want unplugging a cable to cleanly terminate the connection.

Bidirectional devices make CLOCAL mostly unnecessary.  Note that they
are mostly broken in -current.  With non-broken ones, CLOCAL remains
off throughout; CD is ignored for the purpose of opening, but acts
later for the purpose of hanging up if it drops later (to drop, it
must have been raised at some point either earlier or later).  This
works for most uses of modems.  You open the callout device, usually
before carrier is up, and program it.  Then a connection raises carrier
and dropping carrier should terminate the connection.  Perhaps it shouldn't
turn the device into a zombie, and the open fd should remain usable for
further programming of the modem.  But without CLOCAL, there is no way
short of busy-waiting polling to get tell if the connection went away.
The old version of the tty driver has some complications to track the
TS_CONNECTED state independently of the TS_ZOMBIE state.  I forget if
this was needed for further programming of the modem.

CLOCAL is now forced for callout devices, although this ignores the
setting in the initial state device.  This is to work around some other
bugs.  The correct workaround is the change the default for CLOCAL.

>> This is mostly fixed in my version.  I started to cut out the patches,
>> but they were too entwined with other fixes.  Here is the part that
>> replaces the hard-coded 1 second timeout:
>>=20
>> X diff -c2 ./kern/tty.c~ ./kern/tty.c
>> X *** ./kern/tty.c~    Thu Mar 19 18:23:08 2015
>> X --- ./kern/tty.c    Sat Aug  8 11:40:23 2015
>> X ***************
>> X *** 133,155 ****
>> X           return (0);
>> X X !     while (ttyoutq_bytesused(&tp->t_outq) > 0) {
>> X           ttydevsw_outwakeup(tp);
>> X           /* Could be handled synchronously. */
>> X           bytesused =3D ttyoutq_bytesused(&tp->t_outq);
>> X !         if (bytesused =3D=3D 0)
>> X               return (0);
>> X X           /* Wait for data to be drained. */
>> X !         if (leaving) {
>> X               revokecnt =3D tp->t_revokecnt;
>> X !             error =3D tty_timedwait(tp, &tp->t_outwait, hz);
>> X               switch (error) {
>> X               case ERESTART:
>> X                   if (revokecnt !=3D tp->t_revokecnt)
>> X                       error =3D 0;
>> X                   break;
>> X               case EWOULDBLOCK:
>> X !                 if (ttyoutq_bytesused(&tp->t_outq) < bytesused)
>> X                       error =3D 0;
>> X                   break;
>> X               }
>> X --- 196,225 ----
>> X           return (0);
>> X X !     while (ttyoutq_bytesused(&tp->t_outq) !=3D 0 || tp->t_flags &=
=20
>> TS_BUSY) {
>
> Strange diff format... Should patch(1) apply this with all those X'es ?

The X is just quoting for mail, to try to prevent corruption of leading
whitspace by mail programs.  Unfortunately, so mail programs corrupt the
X's instead, by joining them when one is on an empty line (the quoting
is to add a leading "X " and that gives a trailing space for empty lines).

patch(1) should indeed work with all those X's.  Only the mailer corruption
prevents it working.  I didn't know this when I started using this quoting,
and thought that the patch needed manual editing.  I used to use a prefix
of "% ".  The prefix is chosen to be different from normal quoting, so
that patch lines can be extracted using "grep '^X '" and then edited using
sed (or use just sed if you know it well).  Apparently patch was designed
to work directly from quoted lines in mail format, or maybe shar format.

I also like to interleave comments with the patch.  The quoting and
manual extraction is necessary for removing these lines.

> Thank you for answer, anyway! I'll try to understand and test patches nex=
t=20
> week.

Here is a test program with interesting draining.  It is mostly for
debugging revoke(), but this required timing of the draining to get
the revoke() to race with other activity.

If the device is (mis)emulated or possibly if it has large buffers, or
both, then timing test might fail, but it is probably easier to emulate
the timing for large times than small ones.

X /* XXX draining for smaller speeds/buffers is even more broken. */
X #define=09SPEED=099600
X #define=09ttys
X #define=09uart
X=20
X #include <fcntl.h>
X #include <stdio.h>
X #include <string.h>
X #include <termios.h>
X #include <unistd.h>
X=20
X #ifdef ttys
X #ifdef uart
X #define=09CODEV=09"/dev/cuau0"
X #define=09DEV=09"/dev/cuau0"
X #else
X #define=09CODEV=09"/dev/cuad0"
X #define=09DEV=09"/dev/cuad0"
X #endif
X #else
X #define=09CODEV=09"/dev/ptyp0"
X #define=09DEV=09"/dev/ttyp0"
X #endif
X=20
X #define CLOSEDRAINTIME=091
X #define LARGETIME=0910
X #define SMALLTIME=093
X=20
X char largebuf[SPEED / 10 * LARGETIME];
X char smallbuf[SPEED / 10 * SMALLTIME];
X=20
X int
X main(void)
X {
X =09struct termios t;
X =09int fd, fd0;
X=20
X #ifdef CODEV
X =09fd0 =3D open(CODEV, O_RDWR | O_NONBLOCK);
X #endif
X =09fd =3D open(DEV, O_RDWR | O_NONBLOCK);
X =09tcgetattr(fd, &t);
X =09cfsetspeed(&t, SPEED);
X =09t.c_cflag &=3D ~CRTSCTS;
X =09tcsetattr(fd, TCSADRAIN, &t);
X =09fcntl(fd, F_SETFL, fcntl(fd, F_GETFL) & ~O_NONBLOCK);
X =09memset(largebuf, 'a', sizeof(largebuf));
X =09memset(smallbuf, 'a', sizeof(smallbuf));
X =09if (fork() =3D=3D 0) {
X =09=09write(fd, smallbuf, sizeof(smallbuf));
X =09=09printf(
X =09=09"draining should take %d seconds at %d bps (but is broken)\n",
X =09=09    sizeof(smallbuf) / (SPEED / 10), SPEED);
X =09=09printf("drain smallbuf start\n");
X =09=09tcdrain(fd);
X =09=09printf("drain smallbuf done\n");
X =09=09write(fd, largebuf, sizeof(largebuf));
X =09=09printf(
X =09=09"draining should take %d seconds at %d bps (but is broken)\n",
X =09=09    sizeof(largebuf) / (SPEED / 10), SPEED);
X #ifdef hack
X =09exit(0);
X #endif
X =09=09printf("drain largebuf start\n");
X =09=09tcdrain(fd);
X =09=09close(fd);
X =09=09printf("drain largebuf done\n");
X =09} else if (fork() =3D=3D 0) {
X #ifdef testread
X =09exit(0);
# #endif
X =09=09printf("read start\n");
X =09=09read(fd, smallbuf, sizeof(smallbuf));
X =09=09printf("read done\n");
X =09} else {
X #ifdef hack
X =09close(fd);
X =09close(fd0);
X #endif
X =09=09usleep(1000000 * SMALLTIME | 1000000 * CLOSEDRAINTIME / 2);
X =09=09printf("revoke\n");
X =09=09printf(
X "revoke should flush, but actually drains, so should take a long time, bu=
t\n");
X =09=09printf(
X "it actually takes its hard-coded timeout of 1 second\n");
X =09=09revoke(DEV);
X =09=09printf("revoke done\n");
X =09=09sleep(1);
X =09=09printf("parent done\n");
X =09}
X =09return (0);
X }

Bruce
From owner-freebsd-bugs@freebsd.org  Sat Dec 19 06:53:17 2015
Return-Path: <owner-freebsd-bugs@freebsd.org>
Delivered-To: freebsd-bugs@mailman.ysv.freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org
 [IPv6:2001:1900:2254:206a::19:1])
 by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3F618A4B943
 for <freebsd-bugs@mailman.ysv.freebsd.org>;
 Sat, 19 Dec 2015 06:53:17 +0000 (UTC)
 (envelope-from bugzilla-noreply@freebsd.org)
Received: from kenobi.freebsd.org (kenobi.freebsd.org
 [IPv6:2001:1900:2254:206a::16:76])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (Client did not present a certificate)
 by mx1.freebsd.org (Postfix) with ESMTPS id 306EC1C39
 for <freebsd-bugs@FreeBSD.org>; Sat, 19 Dec 2015 06:53:17 +0000 (UTC)
 (envelope-from bugzilla-noreply@freebsd.org)
Received: from bugs.freebsd.org ([127.0.1.118])
 by kenobi.freebsd.org (8.15.2/8.15.2) with ESMTP id tBJ6rHvW023025
 for <freebsd-bugs@FreeBSD.org>; Sat, 19 Dec 2015 06:53:17 GMT
 (envelope-from bugzilla-noreply@freebsd.org)
From: bugzilla-noreply@freebsd.org
To: freebsd-bugs@FreeBSD.org
Subject: [Bug 205424] i386/boot: use macros from <machine/psl.h> to represent
 eflags's bits
Date: Sat, 19 Dec 2015 06:53:16 +0000
X-Bugzilla-Reason: AssignedTo
X-Bugzilla-Type: new
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: Base System
X-Bugzilla-Component: misc
X-Bugzilla-Version: 11.0-CURRENT
X-Bugzilla-Keywords: 
X-Bugzilla-Severity: Affects Only Me
X-Bugzilla-Who: kuleshovmail@gmail.com
X-Bugzilla-Status: New
X-Bugzilla-Priority: ---
X-Bugzilla-Assigned-To: freebsd-bugs@FreeBSD.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags: 
X-Bugzilla-Changed-Fields: bug_id short_desc product version rep_platform
 op_sys bug_status bug_severity priority component assigned_to reporter cc
 attachments.created
Message-ID: <bug-205424-8@https.bugs.freebsd.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: https://bugs.freebsd.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-BeenThere: freebsd-bugs@freebsd.org
X-Mailman-Version: 2.1.20
Precedence: list
List-Id: Bug reports <freebsd-bugs.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/freebsd-bugs>,
 <mailto:freebsd-bugs-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-bugs/>;
List-Post: <mailto:freebsd-bugs@freebsd.org>
List-Help: <mailto:freebsd-bugs-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/freebsd-bugs>,
 <mailto:freebsd-bugs-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 19 Dec 2015 06:53:17 -0000

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205424

            Bug ID: 205424
           Summary: i386/boot: use macros from <machine/psl.h> to
                    represent  eflags's bits
           Product: Base System
           Version: 11.0-CURRENT
          Hardware: Any
                OS: Any
            Status: New
          Severity: Affects Only Me
          Priority: ---
         Component: misc
          Assignee: freebsd-bugs@FreeBSD.org
          Reporter: kuleshovmail@gmail.com
                CC: avg@FreeBSD.org, jhb@FreeBSD.org

Created attachment 164376
  --> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=164376&action=edit
i386/boot: use macros from <machine/psl.h> to represent  eflags's bits

Hello,

Some i386/boot's source code contains some checks of the EFLAGS's bits. In
the same time the <machine/psl.h> header file provides macros which
represents these bits. Let's use human readable names instead of
numbers.

Additionally indentation fixed and empty lines removed.

-- 
You are receiving this mail because:
You are the assignee for the bug.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151219042026.Y1835>