linux-kernel - Re: [PATCH v2 34/35] tty: make tty_get_byte

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YJz0MqOOzrIBjDBL@hovoldconsulting.com>
Date:   Thu, 13 May 2021 11:41:06 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Jiri Slaby <jslaby@...e.cz>
Cc:     gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v2 34/35] tty: make tty_get_byte_size available

On Thu, May 13, 2021 at 09:24:03AM +0200, Jiri Slaby wrote:
> On 10. 05. 21, 11:47, Johan Hovold wrote:
> >> --- a/drivers/tty/tty_ioctl.c
> >> +++ b/drivers/tty/tty_ioctl.c
> >> @@ -300,6 +300,48 @@ int tty_termios_hw_change(const struct ktermios *a, const struct ktermios *b)
> ...
> >> +unsigned char tty_get_byte_size(unsigned int cflag, bool account_flags)
> >> +{
> >> +	unsigned char bits;
> >> +
> >> +	switch (cflag & CSIZE) {
> >> +	case CS5:
> >> +		bits = 5;
> >> +		break;
> >> +	case CS6:
> >> +		bits = 6;
> >> +		break;
> >> +	case CS7:
> >> +		bits = 7;
> >> +		break;
> >> +	case CS8:
> >> +	default:
> >> +		bits = 8;
> >> +		break;
> >> +	}
> >> +
> >> +	if (!account_flags)
> >> +		return bits;
> >> +
> >> +	if (cflag & CSTOPB)
> >> +		bits++;
> >> +	if (cflag & PARENB)
> >> +		bits++;
> >> +
> >> +	return bits + 2;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tty_get_byte_size);
> > 
> > This should really be two functions rather than passing a bool argument.
> > 
> > I think naming them
> > 
> > 	tty_get_word_size()
> > 
> > and
> > 
> > 	tty_get_frame_size()
> > 
> > would be much more clear than than "byte size" + flag.
> 
> Maybe I am screwed, but word means exactly 2B here.

No, not in this context.

Some UART datasheets use "word" and "word length", while others use
"character length" or "data bits" (and variations thereof).

> So instead, I would 
> go for:
> s/word/char/ -- might be confused with C's char, 1B, or maybe not -- or
> s/word/data/ -- more generic and generally used in serial terminology.

But "data size" it not very precise.

I'd go for either

	tty_get_word_size() or tty_get_char_size(), and
	tty_get_frame_size()

or possibly

	tty_get_data_bits(), and
	tty_get_frame_bits()

Since posix and the termios interface use "CSIZE" (even if that "C" is
ambiguous) and our man pages use "character size" perhaps we should
stick with that and use:

	tty_get_char_size(), and
	tty_get_frame_size()

That should be clear enough for everyone.

> > I realise that the serial-driver interface only uses a cflag argument,
> > but I think we should consider passing a pointer to the termios
> > structure instead.
> 
> That's impossible as termios is not always at hand. Examples are:
> pch_uart_startup -> uart_update_timeout
> sunsab_console_setup -> sunsab_convert_to_sab -> uart_update_timeout
> sunsu_kbd_ms_init -> sunsu_change_speed -> uart_update_timeout
> 
> Let me document that in the commit.

Sounds good.

Johan