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 [email protected]
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);
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;
--- 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;
}
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
On Tue, Sep 8, 2009 at 10:06, Greg KH<[email protected]> 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