Re: [PATCH v2 1/3] tty: Add functions for handling flow control chars

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Fri, 8 Apr 2022, Andy Shevchenko wrote:

> On Fri, Apr 08, 2022 at 02:39:52PM +0300, Ilpo Järvinen wrote:
> > Move receive path flow control character handling to own function
> > and a helper.
> > 
> > This seems cleanest approach especially once skipping due to lookahead
> > is added. Its downside is the duplicated START_CHAR and STOP_CHAR
> > checks.
> > 
> > No functional changes.
> 
> But it seems the change. See below.
> 
> ...
> 
> > +static bool n_tty_is_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> > +{
> > +	return c == START_CHAR(tty) || c == STOP_CHAR(tty);
> > +}
> > +
> > +/* Returns true if c is consumed as flow-control character */
> > +static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> > +{
> > +	if (!n_tty_is_char_flow_ctrl(tty, c))
> > +		return false;
> > +
> > +	if (c == START_CHAR(tty)) {
> > +		start_tty(tty);
> > +		process_echoes(tty);
> 
> > +	} else if (c == STOP_CHAR(tty)) {
> 
> In the original code no 'else' was present.
>
> Perhaps it's not a functional change, but this detail has to be explained.

Correct that the previous code didn't have else, however, there was return 
with the same effect. Adding this else here was no accident from my part 
but it is intentionally there to have no functional change for the 
START_CHAR == STOP_CHAR case!

> > +		stop_tty(tty);
> > +	}
> > +
> > +	return true;
> > +}
> 
> ...
> 
> > -	if (I_IXON(tty)) {
> > -		if (c == START_CHAR(tty)) {
> > -			start_tty(tty);
> > -			process_echoes(tty);
> > -			return;
> > -		}
> > -		if (c == STOP_CHAR(tty)) {
> > -			stop_tty(tty);
> > -			return;
> > -		}
> > -	}
> > +	if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c))
> > +		return;
> 
> 

-- 
 i.