Date: Tue, 11 Mar 2014 18:22:14 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marcel Moolenaar <marcel@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r262996 - head/sys/dev/uart Message-ID: <20140311173639.K2716@besplex.bde.org> In-Reply-To: <201403110320.s2B3KBqu023414@svn.freebsd.org> References: <201403110320.s2B3KBqu023414@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 11 Mar 2014, Marcel Moolenaar wrote:
> Log:
> Pass the actual baudrate to tty_init_console(). This defines the initial
> baudrate of the device special file, and makes sure that on open(2) the
> UART is programmed with the correct baudrate. This then eliminates the
> need in uart_tty_param() to override the speed setting.
Here are my old patches (except the one for uart, and larger updates for sio)
to do this for more console drivers. Actually, I mainly added
tty_init_console() calls that are completely missing (these are more needed
when the non-console case is fixed so that the default isn't for consoles).
Most of the drivers are virtual ones that don't have a speed. Only sio and
uart were tested at runtime. bvm seems to be the only new console driver
which is missing a tty_init_console() call after these patches.
% diff -c2 ./dev/cfe/cfe_console.c~ ./dev/cfe/cfe_console.c
% *** ./dev/cfe/cfe_console.c~ Sat Dec 24 05:21:14 2011
% --- ./dev/cfe/cfe_console.c Sat Dec 24 05:21:16 2011
% ***************
% *** 90,93 ****
% --- 90,94 ----
% cfe_consdev.cn_name[0] != '\0') {
% tp = tty_alloc(&cfe_ttydevsw, NULL);
% + tty_init_console(tp, 0);
% tty_makedev(tp, NULL, "cfecons");
% }
% diff -c2 ./dev/ofw/ofw_console.c~ ./dev/ofw/ofw_console.c
% *** ./dev/ofw/ofw_console.c~ Sat Dec 24 05:21:27 2011
% --- ./dev/ofw/ofw_console.c Sat Dec 24 05:21:29 2011
% ***************
% *** 98,101 ****
% --- 98,102 ----
% */
% tp = tty_alloc(&ofw_ttydevsw, NULL);
% + tty_init_console(tp, 0);
% tty_makedev(tp, NULL, "%s", output);
% tty_makealias(tp, "ofwcons");
% diff -c2 ./dev/syscons/syscons.c~ ./dev/syscons/syscons.c
% *** ./dev/syscons/syscons.c~ Tue Apr 10 01:48:10 2012
% --- ./dev/syscons/syscons.c Tue Apr 10 01:48:11 2012
% ***************
% *** 560,564 ****
% */
% }
% !
% dev = make_dev(&consolectl_devsw, 0, UID_ROOT, GID_WHEEL, 0600,
% "consolectl");
% --- 562,567 ----
% */
% }
% ! if (sc_console_unit == unit)
% ! tty_init_console(sc->dev[0], 0);
% dev = make_dev(&consolectl_devsw, 0, UID_ROOT, GID_WHEEL, 0600,
% "consolectl");
% diff -c2 ./dev/usb/serial/usb_serial.c~ ./dev/usb/serial/usb_serial.c
% *** ./dev/usb/serial/usb_serial.c~ Tue Apr 10 01:48:34 2012
% --- ./dev/usb/serial/usb_serial.c Tue Apr 10 01:48:57 2012
% ***************
% *** 331,336 ****
%
% tp = tty_alloc_mutex(&ucom_class, sc, sc->sc_mtx);
% - if (tp == NULL)
% - return (ENOMEM);
%
% /* Check if the client has a custom TTY name */
-current already has my patches for usb only, except for this hunk.
% --- 331,334 ----
% ***************
% *** 369,376 ****
% ucom_cons_softc = sc;
%
% ! memset(&t, 0, sizeof(t));
% ! t.c_ispeed = ucom_cons_baud;
% ! t.c_ospeed = t.c_ispeed;
% t.c_cflag = CS8;
%
% mtx_lock(ucom_cons_softc->sc_mtx);
% --- 367,375 ----
% ucom_cons_softc = sc;
%
% ! tty_init_console(tp, ucom_cons_baud);
% ! t = tp->t_termios_init_in;
% ! #ifdef GARBAGE /* maybe usb can't do all modes, but CS8 by itself is invalid */
% t.c_cflag = CS8;
% + #endif
%
% mtx_lock(ucom_cons_softc->sc_mtx);
% ***************
% *** 1080,1084 ****
%
% /* XXX the TTY layer should call "open()" first! */
% !
% error = ucom_open(tp);
% if (error) {
% --- 1079,1087 ----
%
% /* XXX the TTY layer should call "open()" first! */
% ! /*
% ! * Not quite; its ordering is partly backwards, but some
% ! * parameters must be set early in ttydev_open(), possibly
% ! * before calling ttydevsw_open().
% ! */
% error = ucom_open(tp);
% if (error) {
% ***************
% *** 1091,1094 ****
% --- 1094,1098 ----
% /* Check requested parameters. */
% if (t->c_ispeed && (t->c_ispeed != t->c_ospeed)) {
% + /* XXX c_ospeed == 0 is perfectly valid. */
% DPRINTF("mismatch ispeed and ospeed\n");
% error = EINVAL;
-current has the rest of these usb patches to a fault. It has this XXX
comment about the broken ospeed (ospeed == 0 means hang up), but not the fix.
Testing ispeed is also strange -- ispeed == 0 has no special meaning.
uart gets this wrong in a lesser way: when ospeed == 0, it returns
after hanging up without setting any other parameters. tcsetattr()
is specified to attempt set _all_ the supported attributes requested.
Then if it succeeds in setting a single attribute, it must return
success, and if it failed to set a single attribute then it must return
failure. When it succeeds in setting ospeed == 0, that is success for
the call. The error handling for this is onerous for applications and
most don't do it on success. On success, non-broken error handling
has to do a tcgetattr() and check which attributes are actually set
and decide whether it cares about others. For the ospeed == 0 case,
it would see that no attributes were changed and would notice the uart
bug if it requested a change of another attribute that it cares about
(it has to discard the change to ospeed == 0 before comparing with the
old attributes, since ospeed == 0 is physically impossible so it would
be another bug for this setting to be sticky at the software level).
If it doesn't want to change anything but just wants to hang up, then
it should use tcgetattr() before tcsetattr and only request changing
ospeed.
% diff -c2 ./dev/xen/console/console.c~ ./dev/xen/console/console.c
% *** ./dev/xen/console/console.c~ Sat Dec 24 05:20:43 2011
% --- ./dev/xen/console/console.c Sat Dec 24 05:20:45 2011
% ***************
% *** 234,237 ****
% --- 234,238 ----
%
% xccons = tty_alloc(&xc_ttydevsw, NULL);
% + tty_init_console(xccons, 0);
% tty_makedev(xccons, NULL, "xc%r", 0);
%
% diff -c2 ./ia64/ia64/ssc.c~ ./ia64/ia64/ssc.c
% *** ./ia64/ia64/ssc.c~ Sat Dec 24 05:24:12 2011
% --- ./ia64/ia64/ssc.c Sat Dec 24 05:24:13 2011
% ***************
% *** 118,121 ****
% --- 118,122 ----
%
% tp = tty_alloc(&ssc_class, NULL);
% + tty_init_console(tp, 0);
% tty_makedev(tp, NULL, "ssccons");
% }
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140311173639.K2716>