Date: Tue, 21 Sep 2010 07:58:38 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Schouten <ed@80386.nl> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r212867 - head/sys/kern Message-ID: <20100921064714.N31073@besplex.bde.org> In-Reply-To: <20100920195134.GS56986@hoeg.nl> References: <201009191635.o8JGZgF3008282@svn.freebsd.org> <20100920224337.E63497@besplex.bde.org> <20100920195134.GS56986@hoeg.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 20 Sep 2010, Ed Schouten wrote:
> * Bruce Evans <brde@optusnet.com.au> wrote:
>> Er, my review explained how wrong this is. Please back it out.
>
> So basically you want to have it in its current form, with the only
> difference that CLOCAL is not enforced for callout devices, but still
> want callout devices to skip waiting for DCD? If so, I can change that
> if you like.
It would also be good to fix the original problem with missing locking
for consoles.
% Index: tty.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/kern/tty.c,v
% retrieving revision 1.338
% retrieving revision 1.340
% diff -u -2 -r1.338 -r1.340
% --- tty.c 6 Aug 2010 09:42:15 -0000 1.338
% +++ tty.c 19 Sep 2010 16:35:42 -0000 1.340
% ...
% @@ -264,10 +264,12 @@
% if (!tty_opened(tp)) {
% /* Set proper termios flags. */
% - if (TTY_CALLOUT(tp, dev)) {
% + if (TTY_CALLOUT(tp, dev))
% tp->t_termios = tp->t_termios_init_out;
% - } else {
% + else
% tp->t_termios = tp->t_termios_init_in;
% - }
Good to fix the style bug.
% ttydevsw_param(tp, &tp->t_termios);
% + /* Prevent modem control on callout devices and /dev/console. */
% + if (TTY_CALLOUT(tp, dev) || dev == dev_console)
% + tp->t_termios.c_cflag |= CLOCAL;
This enforces CLOCAL for both the callout devices and consoles. This
shouldn't be done here for either. Set it in the lock and initial state
device for consoles and don't change anything for callout devices.
%
% ttydevsw_modem(tp, SER_DTR|SER_RTS, 0);
% @@ -282,5 +284,5 @@
%
% /* Wait for Carrier Detect. */
% - if (!TTY_CALLOUT(tp, dev) && (oflags & O_NONBLOCK) == 0 &&
% + if ((oflags & O_NONBLOCK) == 0 &&
% (tp->t_termios.c_cflag & CLOCAL) == 0) {
% while ((ttydevsw_modem(tp, 0, 0) & SER_DCD) == 0) {
This simplification depends on the previous erroneous enforcement.
> About the default flags for serial devices. I really have no idea how to
> sanely address the issue you're raising. Isn't it a good thing that
> right now we initialize all termios structures similarly? It makes the
> system far more predictive than pushing that logic into the driver.
It's good for the initialization to be centralized, but console
initialization must be special. It already is special, and requires
special logic in the driver, since only the driver knows the speed and
line state that are already being used and must be preserved. Presumably
it overwrites the settings of tty_init_termios() soon after that returns.
RELENG_7 had this more or less correctly centralized, with the speed and
an "echo" (really consoleness) flag passed to ttyinitmode(), and another
function ttyconsolemode() for the additional special setting of consoles
(this should have handled the speed and "echo" too). The special settings
are almost exactly the defaulting and locking of CLOCAL:
% void
% ttyconsolemode(struct tty *tp, int speed)
% {
%
% if (speed == 0)
% speed = TTYDEF_SPEED;
% ttyinitmode(tp, 1, speed);
% tp->t_init_in.c_cflag |= CLOCAL;
Default CLOCAL for t_init_in (whole t_init_in will be copied to t_init_out).
% tp->t_lock_out.c_cflag = tp->t_lock_in.c_cflag = CLOCAL;
Lock CLOCAL for both (won't copy whole lock termios).
% tp->t_lock_out.c_ispeed = tp->t_lock_out.c_ospeed =
% tp->t_lock_in.c_ispeed = tp->t_lock_in.c_ospeed = speed;
Lock the speeds too.
% tp->t_init_out = tp->t_init_in;
% tp->t_termios = tp->t_init_in;
Actually silly to copy the whole t_init_in twice after just changing
CLOCAL in it (we have just called ttyinitmode() which copied everything
else).
% ttsetwater(tp);
Must change the watermarks whenever the speed changed, but ttyinitmode()
should have called this since it changed the speed.
Missing: setting and locking of other termios state (especially in cflag)
critical to the continued operation of the console (also the debugger on
a possibly-non-console port).
% }
Having predicitiveness/orthogonality is perhaps the main reason why
CLOCAL was not forced or even defaulted to for callout devices.
> Also, I don't think "breaking historical behaviour" should always be
> considered a bug, which is often something you seem to imply.
It is when the historical behaviour was carefully designed.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100921064714.N31073>