linux-next: manual merge of the tty tree with the tree

linux-next: manual merge of the tty tree with the tree

[Stephen Rothwell]

Hi Greg,

Today's linux-next merge of the tty tree got a conflict in
drivers/char/n_tty.c between commit
37f81fa1f63ad38e16125526bb2769ae0ea8d332 ("n_tty: do O_ONLCR translation
as a single write") from Linus' tree and commit
bb2d17d83926bf9d70b922031aeb49ca896e0b3d ("tty: n_tty: honor opost flag
for echoes") from the tty tree.

I fixed it up (see below) and can carry the fix for a while.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc drivers/char/n_tty.c
index 4e28b35,71308b6..0000000
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@@ -292,53 -292,56 +292,55 @@@ static int do_output_char(unsigned cha
  	if (!space)
  		return -1;
  
- 	switch (c) {
- 	case '\n':
- 		if (O_ONLRET(tty))
- 			tty->column = 0;
- 		if (O_ONLCR(tty)) {
- 			if (space < 2)
- 				return -1;
- 			tty->canon_column = tty->column = 0;
- 			tty->ops->write(tty, "\r\n", 2);
- 			return 2;
- 		}
- 		tty->canon_column = tty->column;
- 		break;
- 	case '\r':
- 		if (O_ONOCR(tty) && tty->column == 0)
- 			return 0;
- 		if (O_OCRNL(tty)) {
- 			c = '\n';
+ 	if (O_OPOST(tty)) {
+ 		switch (c) {
+ 		case '\n':
  			if (O_ONLRET(tty))
+ 				tty->column = 0;
+ 			if (O_ONLCR(tty)) {
+ 				if (space < 2)
+ 					return -1;
  				tty->canon_column = tty->column = 0;
 -				tty_put_char(tty, '\r');
 -				tty_put_char(tty, c);
++				tty->ops->write(tty, "\r\n", 2);
+ 				return 2;
+ 			}
+ 			tty->canon_column = tty->column;
  			break;
- 		}
- 		tty->canon_column = tty->column = 0;
- 		break;
- 	case '\t':
- 		spaces = 8 - (tty->column & 7);
- 		if (O_TABDLY(tty) == XTABS) {
- 			if (space < spaces)
- 				return -1;
+ 		case '\r':
+ 			if (O_ONOCR(tty) && tty->column == 0)
+ 				return 0;
+ 			if (O_OCRNL(tty)) {
+ 				c = '\n';
+ 				if (O_ONLRET(tty))
+ 					tty->canon_column = tty->column = 0;
+ 				break;
+ 			}
+ 			tty->canon_column = tty->column = 0;
+ 			break;
+ 		case '\t':
+ 			spaces = 8 - (tty->column & 7);
+ 			if (O_TABDLY(tty) == XTABS) {
+ 				if (space < spaces)
+ 					return -1;
+ 				tty->column += spaces;
+ 				tty->ops->write(tty, "        ", spaces);
+ 				return spaces;
+ 			}
  			tty->column += spaces;
- 			tty->ops->write(tty, "        ", spaces);
- 			return spaces;
- 		}
- 		tty->column += spaces;
- 		break;
- 	case '\b':
- 		if (tty->column > 0)
- 			tty->column--;
- 		break;
- 	default:
- 		if (!iscntrl(c)) {
- 			if (O_OLCUC(tty))
- 				c = toupper(c);
- 			if (!is_continuation(c, tty))
- 				tty->column++;
+ 			break;
+ 		case '\b':
+ 			if (tty->column > 0)
+ 				tty->column--;
+ 			break;
+ 		default:
+ 			if (!iscntrl(c)) {
+ 				if (O_OLCUC(tty))
+ 					c = toupper(c);
+ 				if (!is_continuation(c, tty))
+ 					tty->column++;
+ 			}
+ 			break;
  		}
- 		break;
  	}
  
  	tty_put_char(tty, c);

Re: linux-next: manual merge of the tty tree with the tree

[Linus Torvalds]

On Mon, 7 Sep 2009, Stephen Rothwell wrote:
> 
> Today's linux-next merge of the tty tree got a conflict in
> drivers/char/n_tty.c between commit
> 37f81fa1f63ad38e16125526bb2769ae0ea8d332 ("n_tty: do O_ONLCR translation
> as a single write") from Linus' tree and commit
> bb2d17d83926bf9d70b922031aeb49ca896e0b3d ("tty: n_tty: honor opost flag
> for echoes") from the tty tree.
> 
> I fixed it up (see below) and can carry the fix for a while.

Hmm. I think that the "honor opost flag for echoes" patch is actually 
wrong.

We check O_OPOST() in the _caller_ for the regular write case, and that 
test actually looks like this:

	if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {

so at a minimum, if we add it to process_output() we should likely add it 
in the same format. But if we need that test, I'd rather do it in the 
caller anyway, like we already do for regular writes.

So maybe the patch could be changed to something (UNTESTD!) like the 
following instead? And thus avoid the conflict at the same time.

		Linus

---
 drivers/char/n_tty.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
index 4e28b35..9c04bb4 100644
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@ -345,6 +345,18 @@ static int do_output_char(unsigned char c, struct tty_struct *tty, int space)
 	return 1;
 }
 
+static int output_echo(unsigned char c, struct tty_struct *tty, int space)
+{
+	if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags)))
+		return do_output_char(c, tty, space);
+
+	if (!space)
+		return -1;
+
+	tty_put_char(tty, c);
+	return 1;
+}
+
 /**
  *	process_output			-	output post processor
  *	@c: character (or partial unicode symbol)
@@ -607,7 +619,7 @@ static void process_echoes(struct tty_struct *tty)
 		} else {
 			int retval;
 
-			retval = do_output_char(c, tty, space);
+			retval = output_echo(c, tty, space);
 			if (retval < 0)
 				break;
 			space -= retval;

Re: linux-next: manual merge of the tty tree with the tree

[Joe Peterson]

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

Linus Torvalds wrote:
> Hmm. I think that the "honor opost flag for echoes" patch is actually 
> wrong.
> 
> We check O_OPOST() in the _caller_ for the regular write case, and that
> test actually looks like this:
> 
> 	if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {
> 
> so at a minimum, if we add it to process_output() we should likely add it 
> in the same format. But if we need that test, I'd rather do it in the 
> caller anyway, like we already do for regular writes.

Yes, very true.  The old opost() function also contained the O_OPOST
check (i.e. causing a double check for normal writes), and you are right
that we should not reintroduce it (and it makes sense for the caller to
check it).

There is only the one case in which the O_OPOST check is needed before
calling do_output_char() (in process_echoes()), so we could just inline
the test there.  Take a look at my new attached patch (untested also).
I'll test and resubmit, assuming there are no objections.

					-Thanks, Joe

[-- Attachment #2: n_tty-honor-opost-flag-for-echoes.patch --]
[-- Type: text/plain, Size: 2102 bytes --]

--- n_tty.c.orig	2009-09-07 13:54:32.460678337 -0600
+++ n_tty.c	2009-09-07 15:48:51.191033405 -0600
@@ -272,7 +272,8 @@ static inline int is_continuation(unsign
  *
  *	This is a helper function that handles one output character
  *	(including special characters like TAB, CR, LF, etc.),
- *	putting the results in the tty driver's write buffer.
+ *	doing OPOST processing and putting the results in the
+ *	tty driver's write buffer.
  *
  *	Note that Linux currently ignores TABDLY, CRDLY, VTDLY, FFDLY
  *	and NLDLY.  They simply aren't relevant in the world today.
@@ -351,8 +352,9 @@ static int do_output_char(unsigned char 
  *	@c: character (or partial unicode symbol)
  *	@tty: terminal device
  *
- *	Perform OPOST processing.  Returns -1 when the output device is
- *	full and the character must be retried.
+ *	Output one character with OPOST processing.
+ *	Returns -1 when the output device is full and the character
+ *	must be retried.
  *
  *	Locking: output_lock to protect column state and space left
  *		 (also, this is called from n_tty_write under the
@@ -378,8 +380,11 @@ static int process_output(unsigned char 
 /**
  *	process_output_block		-	block post processor
  *	@tty: terminal device
- *	@inbuf: user buffer
- *	@nr: number of bytes
+ *	@buf: character buffer
+ *	@nr: number of bytes to output
+ *
+ *	Output a block of characters with OPOST processing.
+ *	Returns the number of characters output.
  *
  *	This path is used to speed up block console writes, among other
  *	things when processing blocks of output data. It handles only
@@ -606,12 +611,18 @@ static void process_echoes(struct tty_st
 			if (no_space_left)
 				break;
 		} else {
-			int retval;
-
-			retval = do_output_char(c, tty, space);
-			if (retval < 0)
-				break;
-			space -= retval;
+			if (O_OPOST(tty) &&
+			    !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {
+				int retval = do_output_char(c, tty, space);
+				if (retval < 0)
+					break;
+				space -= retval;
+			} else {
+				if (!space)
+					break;
+				tty_put_char(tty, c);
+				space -= 1;
+			}
 			cp += 1;
 			nr -= 1;
 		}

Re: linux-next: manual merge of the tty tree with the tree

[Greg KH]

On Mon, Sep 07, 2009 at 03:55:44PM -0600, Joe Peterson wrote:
> Linus Torvalds wrote:
> > Hmm. I think that the "honor opost flag for echoes" patch is actually 
> > wrong.
> > 
> > We check O_OPOST() in the _caller_ for the regular write case, and that
> > test actually looks like this:
> > 
> > 	if (O_OPOST(tty) && !(test_bit(TTY_HW_COOK_OUT, &tty->flags))) {
> > 
> > so at a minimum, if we add it to process_output() we should likely add it 
> > in the same format. But if we need that test, I'd rather do it in the 
> > caller anyway, like we already do for regular writes.
> 
> Yes, very true.  The old opost() function also contained the O_OPOST
> check (i.e. causing a double check for normal writes), and you are right
> that we should not reintroduce it (and it makes sense for the caller to
> check it).
> 
> There is only the one case in which the O_OPOST check is needed before
> calling do_output_char() (in process_echoes()), so we could just inline
> the test there.  Take a look at my new attached patch (untested also).
> I'll test and resubmit, assuming there are no objections.

Thanks for doing this, I'll drop the patch from my tree and wait for you
to test and resubmit this.

thanks,

greg k-h

Re: linux-next: manual merge of the tty tree with the tree

[Joe Peterson]

On Tue, Sep 8, 2009 at 10:06, Greg KH<greg@kroah.com> wrote:
> Thanks for doing this, I'll drop the patch from my tree and wait for you
> to test and resubmit this.

No problem, Greg - see the two resubmitted patch parts I just sent
out; and please excuse the double send (munged the addresses on the
lists).

-Joe