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: <f8f94397-14d8-60d0-f977-8bc982a00b7a@suse.cz>
Date:   Thu, 13 May 2021 09:24:03 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     Johan Hovold <johan@...nel.org>
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 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. 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.

> 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.

thanks,
-- 
js
suse labs