USB: serial: console: fix reported terminal settings

PATCH In review Johan Hovold 3 participants 9 messages Dec 4, 2018 – Dec 5, 2018 1 message not in the archive

Subsystems (4)

Stable branch USB TTY layer and serial drivers USB serial

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20181204155904.GC18087@localhost

Johan Hovold

author maintainer

2018-12-04 16:00 UTC #1 lore raw

PATCH 1/1 USB: serial: console: fix reported terminal settings

The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.

Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.

Instead, use a (renamed) tty helper to save the termios data used at console setup, so that the tty termios reflects the actual terminal settings after a subsequent tty open().

Note that the calls to tty_init_termios() (tty_driver_install()) and tty_save_termios() are serialised using the disconnect mutex.

This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).

Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")

Cc stable <[email protected]> # 4.18

Reported-by Jarkko Nikula <[email protected]>

Cc Florian Zumbiehl <[email protected]>

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/tty/tty_io.c         | 11 +++++++++--
 drivers/usb/serial/console.c |  2 +-
 include/linux/tty.h          |  1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

drivers/tty/tty_io.c +9 -2

index ee80dfbd5442..687250ec8032 100644
@@ -1373,7 +1373,13 @@ struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx)
 	return ERR_PTR(retval);
 }
 
-static void tty_free_termios(struct tty_struct *tty)
+/**
+ * tty_save_termios() - save tty termios data in driver table
+ * @tty: tty whose termios data to save
+ *
+ * Locking: Caller guarantees serialisation with tty_init_termios().
+ */
+void tty_save_termios(struct tty_struct *tty)
 {
 	struct ktermios *tp;
 	int idx = tty->index;

@@ -1392,6 +1398,7 @@ static void tty_free_termios(struct tty_struct *tty)
 	}
 	*tp = tty->termios;
 }
+EXPORT_SYMBOL_GPL(tty_save_termios);
 
 /**
  *	tty_flush_works		-	flush all works of a tty/pty pair

@@ -1491,7 +1498,7 @@ static void release_tty(struct tty_struct *tty, int idx)
 	WARN_ON(!mutex_is_locked(&tty_mutex));
 	if (tty->ops->shutdown)
 		tty->ops->shutdown(tty);
-	tty_free_termios(tty);
+	tty_save_termios(tty);
 	tty_driver_remove_tty(tty->driver, tty);
 	tty->port->itty = NULL;
 	if (tty->link)

drivers/usb/serial/console.c +1 -1

index 17940589c647..7d289302ff6c 100644
@@ -101,7 +101,6 @@ static int usb_console_setup(struct console *co, char *options)
 		cflag |= PARENB;
 		break;
 	}
-	co->cflag = cflag;
 
 	/*
 	 * no need to check the index here: if the index is wrong, console

@@ -164,6 +163,7 @@ static int usb_console_setup(struct console *co, char *options)
 			serial->type->set_termios(tty, port, &dummy);
 
 			tty_port_tty_set(&port->port, NULL);
+			tty_save_termios(tty);
 			tty_kref_put(tty);
 		}
 		tty_port_set_initialized(&port->port, 1);

include/linux/tty.h +1 -0

index 414db2bce715..392138fe59b6 100644
@@ -556,6 +556,7 @@ extern struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx);
 extern void tty_release_struct(struct tty_struct *tty, int idx);
 extern int tty_release(struct inode *inode, struct file *filp);
 extern void tty_init_termios(struct tty_struct *tty);
+extern void tty_save_termios(struct tty_struct *tty);
 extern int tty_standard_install(struct tty_driver *driver,
 		struct tty_struct *tty);

Greg Kroah-Hartman

maintainer

2018-12-04 16:15 UTC #2 lore raw

Quoted 29 lines · Johan Hovold

The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.
Instead, use a (renamed) tty helper to save the termios data used at console setup, so that the tty termios reflects the actual terminal settings after a subsequent tty open().
Note that the calls to tty_init_termios() (tty_driver_install()) and tty_save_termios() are serialised using the disconnect mutex.
This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).
Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")
Cc stable <[email protected]> # 4.18
Reported-by Jarkko Nikula <[email protected]>
Cc Florian Zumbiehl <[email protected]>
Signed-off-by Johan Hovold <[email protected]>

Quoted 5 lines

---
 drivers/tty/tty_io.c         | 11 +++++++++--
 drivers/usb/serial/console.c |  2 +-
 include/linux/tty.h          |  1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

Ah, messy :)

Want me to take this through my tty tree?

thanks,

greg k-h

Johan Hovold

author maintainer

2018-12-04 16:31 UTC #3 lore raw

Quoted 36 lines · Greg Kroah-Hartman

Quoted 29 lines · Johan Hovold
The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.
Instead, use a (renamed) tty helper to save the termios data used at console setup, so that the tty termios reflects the actual terminal settings after a subsequent tty open().
Note that the calls to tty_init_termios() (tty_driver_install()) and tty_save_termios() are serialised using the disconnect mutex.
This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).
Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")
Cc stable <[email protected]> # 4.18
Reported-by Jarkko Nikula <[email protected]>
Cc Florian Zumbiehl <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
---
 drivers/tty/tty_io.c         | 11 +++++++++--
 drivers/usb/serial/console.c |  2 +-
 include/linux/tty.h          |  1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

Quoted 3 lines

Ah, messy :)
Want me to take this through my tty tree?

If you prefer. I was planning on including this in a USB-serial pull request for -rc6 since it fixes a user-reported regression, but perhaps taking this through your tty-linus branch (which already holds a console fix) is easier/faster.

We should wait for Jarkko to confirm that this fixes the problem he reported first, though.

Thanks,
Johan

Jarkko Nikula

Tested-by

2018-12-05 09:36 UTC #4 lore raw

Quoted 41 lines · Johan Hovold

Quoted 36 lines · Greg Kroah-Hartman
Quoted 29 lines · Johan Hovold
The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.
Instead, use a (renamed) tty helper to save the termios data used at console setup, so that the tty termios reflects the actual terminal settings after a subsequent tty open().
Note that the calls to tty_init_termios() (tty_driver_install()) and tty_save_termios() are serialised using the disconnect mutex.
This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).
Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")
Cc stable <[email protected]> # 4.18
Reported-by Jarkko Nikula <[email protected]>
Cc Florian Zumbiehl <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
---
  drivers/tty/tty_io.c         | 11 +++++++++--
  drivers/usb/serial/console.c |  2 +-
  include/linux/tty.h          |  1 +
  3 files changed, 11 insertions(+), 3 deletions(-)
Quoted 3 lines
Ah, messy :)
Want me to take this through my tty tree?

Quoted 8 lines

If you prefer. I was planning on including this in a USB-serial pull request for -rc6 since it fixes a user-reported regression, but perhaps taking this through your tty-linus branch (which already holds a console fix) is easier/faster.
We should wait for Jarkko to confirm that this fixes the problem he reported first, though.

Great, this fixed the issue for both pl2303 based adapters I reported.

Tested on top of 0072a0c14d5b ("Merge tag 'media/v4.20-4' of
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media")

Tested-by Jarkko Nikula <[email protected]>

Johan Hovold

author maintainer

2018-12-05 09:50 UTC #5 lore raw

Quoted 43 lines · Jarkko Nikula

Quoted 34 lines · Johan Hovold
Quoted 29 lines · Greg Kroah-Hartman
Quoted 22 lines · Johan Hovold
The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.
This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).
Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")
Cc stable <[email protected]> # 4.18
Reported-by Jarkko Nikula <[email protected]>
Cc Florian Zumbiehl <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
---
  drivers/tty/tty_io.c         | 11 +++++++++--
  drivers/usb/serial/console.c |  2 +-
  include/linux/tty.h          |  1 +
  3 files changed, 11 insertions(+), 3 deletions(-)
Quoted 3 lines
Ah, messy :)
Want me to take this through my tty tree?
Quoted 8 lines
If you prefer. I was planning on including this in a USB-serial pull request for -rc6 since it fixes a user-reported regression, but perhaps taking this through your tty-linus branch (which already holds a console fix) is easier/faster.
We should wait for Jarkko to confirm that this fixes the problem he reported first, though.

Quoted 6 lines

Great, this fixed the issue for both pl2303 based adapters I reported.
Tested on top of 0072a0c14d5b ("Merge tag 'media/v4.20-4' of
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media")
Tested-by Jarkko Nikula <[email protected]>

Great, thanks for testing.

Greg, I noticed I left out the part about subdrivers not expecting *write()* to be called before open() so I'll amend the commit message when applying and include this one in a pull-request tomorrow, if that's ok with you?

Note that the changes to tty are trivial; I'm just renaming and exporting an existing helper.

Thanks,
Johan

Greg Kroah-Hartman

maintainer

2018-12-05 10:05 UTC #6 lore raw

Quoted 49 lines · Johan Hovold

Quoted 48 lines · Jarkko Nikula
Quoted 34 lines · Johan Hovold
Quoted 29 lines · Greg Kroah-Hartman
Quoted 22 lines · Johan Hovold
The USB-serial console implementation has never reported the actual terminal settings used. Despite storing the corresponding cflags in its struct console, this was never honoured on later tty open() where the tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls subdriver open() already at console setup. While calling set_termios() before open() looks like it could work for some USB-serial drivers, others definitely do not expect this, so modelling this after serial core is going to be intrusive, if at all possible.
This specifically fixes a regression that was triggered by a recent change adding software flow control to the pl2303 driver: a getty trying to disable flow control while leaving the baud rate unchanged would now also set the baud rate to the driver default (prior to the flow-control change this had been a noop).
Fixes 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow control")
Cc stable <[email protected]> # 4.18
Reported-by Jarkko Nikula <[email protected]>
Cc Florian Zumbiehl <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
---
  drivers/tty/tty_io.c         | 11 +++++++++--
  drivers/usb/serial/console.c |  2 +-
  include/linux/tty.h          |  1 +
  3 files changed, 11 insertions(+), 3 deletions(-)
Quoted 3 lines
Ah, messy :)
Want me to take this through my tty tree?
Quoted 8 lines
If you prefer. I was planning on including this in a USB-serial pull request for -rc6 since it fixes a user-reported regression, but perhaps taking this through your tty-linus branch (which already holds a console fix) is easier/faster.
We should wait for Jarkko to confirm that this fixes the problem he reported first, though.
Great, this fixed the issue for both pl2303 based adapters I reported.
Tested on top of 0072a0c14d5b ("Merge tag 'media/v4.20-4' of
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media")

Quoted 8 lines

Quoted 1 line
Tested-by Jarkko Nikula <[email protected]>
Great, thanks for testing.
Greg, I noticed I left out the part about subdrivers not expecting *write()* to be called before open() so I'll amend the commit message when applying and include this one in a pull-request tomorrow, if that's ok with you?

Sure, that's fine.

Quoted 2 lines

Note that the changes to tty are trivial; I'm just renaming and exporting an existing helper.

Yeah, I have no objections to them. We can take them in my usb tree.

thanks,

greg k-h

Johan Hovold

author maintainer

2018-12-05 10:10 UTC #7 lore raw

Quoted 6 lines · Greg Kroah-Hartman

Quoted 2 lines · Johan Hovold
Note that the changes to tty are trivial; I'm just renaming and exporting an existing helper.
Yeah, I have no objections to them. We can take them in my usb tree.

Thanks. Can I add your ack to the patch as well?

Johan

Greg Kroah-Hartman

maintainer Acked-by

2018-12-05 10:18 UTC #8 lore raw

Quoted 9 lines · Johan Hovold

Quoted 6 lines · Greg Kroah-Hartman
Quoted 2 lines · Johan Hovold
Note that the changes to tty are trivial; I'm just renaming and exporting an existing helper.
Yeah, I have no objections to them. We can take them in my usb tree.
Thanks. Can I add your ack to the patch as well?

Please do:

Acked-by Greg Kroah-Hartman <[email protected]>

Johan Hovold

author maintainer

2018-12-05 10:33 UTC #9 lore raw

Quoted 7 lines · Greg Kroah-Hartman

Quoted 6 lines · Johan Hovold
Quoted 5 lines · Greg Kroah-Hartman
Quoted 2 lines · Johan Hovold
Note that the changes to tty are trivial; I'm just renaming and exporting an existing helper.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Yeah, I have no objections to them. We can take them in my usb tree.
Thanks. Can I add your ack to the patch as well?
Please do:
Acked-by Greg Kroah-Hartman <[email protected]>

Thanks, now applied.

Johan