There are 3 regressions in the 2.6.32 development stream for the usb
serial console.
1) A hang in the usb serial core due to a unfreed mutex
2) Crash in the usb serial core if you use a serial console
because there is a double open of the low level device
3) It is not possible to pass the console baud rate on to
the first tty open for any usb serial driver other
than the pl2303 (which has device specific baud
management).
The first open of the usb serial HW has the termios initialized to
9600 baud, and this will override what ever was setup via the original
console initialization. The solution is to save the console baud rate
and re-use it later on the first open.
This patch series only applies to the end of the Greg KH usb tree. It
was tested with a pl2303, mct_u232 and ftdi_sio usb serial devices.
Jason.
Jason Wessel (3):
usb console: fix mutex lock regression
usb console,usb-serial: fix regression use of serial->console
usb console,usb-serial: pass initial console baud on to first tty open
drivers/usb/serial/console.c | 2 ++
drivers/usb/serial/usb-serial.c | 32 +++++++++++++++++---------------
include/linux/usb/serial.h | 1 +
3 files changed, 20 insertions(+), 15 deletions(-)
During the 2.6.32 development the use of serial->disc_mutex was
introduced. In usb_console_setup() the call to
usb_serial_get_by_index() will obtain this mutex. The
usb_console_setup() must release the mutex before it completes else
later calls to the usb serial core will hang.
Signed-off-by: Jason Wessel <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Alan Stern <[email protected]>
---
drivers/usb/serial/console.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index be086e4..166ba4e 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -176,6 +176,7 @@ static int usb_console_setup(struct console *co, char *options)
* indicate this port is now acting as a system console. */
port->console = 1;
retval = 0;
+ mutex_unlock(&serial->disc_mutex);
out:
return retval;
--
1.6.4.rc1
During the 2.6.32 development usb serial tty interface was changed.
This change caused the usb serial driver to crash when used as a
system console.
The open and close of the raw hardware has to be protected by the
serial->console variable so either the console code does the open and
close or the higher level tty based driver executes the open and
close.
* In serial_open() the raw open must be protected by serial->console
* In serial_down() the test_and_clear_bit must be executed before
leaving the function if the device is a serial console
* serial_release() should no longer protect for the console condition
due to the 2.6.32 interface changes. The actual low level close
protection is accounted for in serial_down()
Signed-off-by: Jason Wessel <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Alan Stern <[email protected]>
---
drivers/usb/serial/usb-serial.c | 28 +++++++++++++---------------
1 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index ff75a35..3d35ed2 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -267,10 +267,14 @@ static int serial_open(struct tty_struct *tty, struct file *filp)
if (mutex_lock_interruptible(&port->mutex))
return -ERESTARTSYS;
mutex_lock(&serial->disc_mutex);
- if (serial->disconnected)
+ if (serial->disconnected) {
retval = -ENODEV;
- else
- retval = port->serial->type->open(tty, port);
+ } else {
+ if (port->console)
+ retval = 0;
+ else
+ retval = port->serial->type->open(tty, port);
+ }
mutex_unlock(&serial->disc_mutex);
mutex_unlock(&port->mutex);
if (retval)
@@ -294,6 +298,12 @@ static void serial_down(struct usb_serial_port *port)
{
struct usb_serial_driver *drv = port->serial->type;
+ /* Don't call the close method if the hardware hasn't been
+ * initialized.
+ */
+ if (!test_and_clear_bit(ASYNCB_INITIALIZED, &port->port.flags))
+ return;
+
/*
* The console is magical. Do not hang up the console hardware
* or there will be tears.
@@ -301,12 +311,6 @@ static void serial_down(struct usb_serial_port *port)
if (port->console)
return;
- /* Don't call the close method if the hardware hasn't been
- * initialized.
- */
- if (!test_and_clear_bit(ASYNCB_INITIALIZED, &port->port.flags))
- return;
-
mutex_lock(&port->mutex);
if (drv->close)
drv->close(port);
@@ -353,12 +357,6 @@ static void serial_release(struct tty_struct *tty)
struct usb_serial *serial;
struct module *owner;
- /* The console is magical. Do not hang up the console hardware
- * or there will be tears.
- */
- if (port->console)
- return;
-
dbg("%s - port %d", __func__, port->number);
/* Standard shutdown processing */
--
1.6.4.rc1
The first open of the usb serial HW has the termios initialized to the
default of 9600 baud, and this will override what ever was setup via
the original console initialization.
The solution is to save the console baud rate and re-use it later on
the first tty open, so as to allow the use of baud rates other than
the default 9600 for the usb serial console.
Signed-off-by: Jason Wessel <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Alan Stern <[email protected]>
---
drivers/usb/serial/console.c | 1 +
drivers/usb/serial/usb-serial.c | 8 ++++++--
include/linux/usb/serial.h | 1 +
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 166ba4e..4fd5b52 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -175,6 +175,7 @@ static int usb_console_setup(struct console *co, char *options)
/* The console is special in terms of closing the device so
* indicate this port is now acting as a system console. */
port->console = 1;
+ port->console_init_baud = baud;
retval = 0;
mutex_unlock(&serial->disc_mutex);
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 3d35ed2..f931738 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -270,10 +270,14 @@ static int serial_open(struct tty_struct *tty, struct file *filp)
if (serial->disconnected) {
retval = -ENODEV;
} else {
- if (port->console)
+ if (port->console) {
+ tty_encode_baud_rate(tty,
+ port->console_init_baud,
+ port->console_init_baud);
retval = 0;
- else
+ } else {
retval = port->serial->type->open(tty, port);
+ }
}
mutex_unlock(&serial->disc_mutex);
mutex_unlock(&port->mutex);
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index c17eb64..a7c8725 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -109,6 +109,7 @@ struct usb_serial_port {
char throttled;
char throttle_req;
char console;
+ int console_init_baud;
unsigned long sysrq; /* sysrq timeout */
struct device dev;
enum port_dev_state dev_state;
--
1.6.4.rc1
On Wed, 16 Sep 2009, Jason Wessel wrote:
> During the 2.6.32 development the use of serial->disc_mutex was
> introduced. In usb_console_setup() the call to
> usb_serial_get_by_index() will obtain this mutex. The
> usb_console_setup() must release the mutex before it completes else
> later calls to the usb serial core will hang.
Apparently you didn't see my patch:
http://marc.info/?l=linux-usb&m=125209260714221&w=2
Did this not get into linus-next? Greg?
Alan Stern
On Wed, 16 Sep 2009, Jason Wessel wrote:
> During the 2.6.32 development usb serial tty interface was changed.
> This change caused the usb serial driver to crash when used as a
> system console.
>
> The open and close of the raw hardware has to be protected by the
> serial->console variable so either the console code does the open and
> close or the higher level tty based driver executes the open and
> close.
>
> * In serial_open() the raw open must be protected by serial->console
> * In serial_down() the test_and_clear_bit must be executed before
> leaving the function if the device is a serial console
> * serial_release() should no longer protect for the console condition
> due to the 2.6.32 interface changes. The actual low level close
> protection is accounted for in serial_down()
>
> Signed-off-by: Jason Wessel <[email protected]>
> Cc: Greg KH <[email protected]>
> Cc: Alan Stern <[email protected]>
>
> ---
> drivers/usb/serial/usb-serial.c | 28 +++++++++++++---------------
> 1 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index ff75a35..3d35ed2 100644
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -267,10 +267,14 @@ static int serial_open(struct tty_struct *tty, struct file *filp)
> if (mutex_lock_interruptible(&port->mutex))
> return -ERESTARTSYS;
> mutex_lock(&serial->disc_mutex);
> - if (serial->disconnected)
> + if (serial->disconnected) {
> retval = -ENODEV;
> - else
> - retval = port->serial->type->open(tty, port);
> + } else {
> + if (port->console)
> + retval = 0;
> + else
> + retval = port->serial->type->open(tty, port);
> + }
> mutex_unlock(&serial->disc_mutex);
> mutex_unlock(&port->mutex);
> if (retval)
This part of the patch shouldn't be necessary. The console device
should always be open.
> @@ -294,6 +298,12 @@ static void serial_down(struct usb_serial_port *port)
> {
> struct usb_serial_driver *drv = port->serial->type;
>
> + /* Don't call the close method if the hardware hasn't been
> + * initialized.
> + */
> + if (!test_and_clear_bit(ASYNCB_INITIALIZED, &port->port.flags))
> + return;
> +
> /*
> * The console is magical. Do not hang up the console hardware
> * or there will be tears.
I don't understand this. Why would you want to clear the
ASYNCB_INITIALIZED flag for the console device? Since you're not going
to deinitialize the hardware, the flag should remain set.
> @@ -301,12 +311,6 @@ static void serial_down(struct usb_serial_port *port)
> if (port->console)
> return;
>
> - /* Don't call the close method if the hardware hasn't been
> - * initialized.
> - */
> - if (!test_and_clear_bit(ASYNCB_INITIALIZED, &port->port.flags))
> - return;
> -
> mutex_lock(&port->mutex);
> if (drv->close)
> drv->close(port);
And of course, without the previous hunk this hunk is unnecessary.
> @@ -353,12 +357,6 @@ static void serial_release(struct tty_struct *tty)
> struct usb_serial *serial;
> struct module *owner;
>
> - /* The console is magical. Do not hang up the console hardware
> - * or there will be tears.
> - */
> - if (port->console)
> - return;
> -
> dbg("%s - port %d", __func__, port->number);
>
> /* Standard shutdown processing */
This also doesn't make sense. The console never should get closed or
released.
Alan Stern
Alan Stern wrote:
> On Wed, 16 Sep 2009, Jason Wessel wrote:
>
>
>> During the 2.6.32 development usb serial tty interface was changed.
>> This change caused the usb serial driver to crash when used as a
>> system console.
>>
>> The open and close of the raw hardware has to be protected by the
>> serial->console variable so either the console code does the open and
>> close or the higher level tty based driver executes the open and
>> close.
>>
>> * In serial_open() the raw open must be protected by serial->console
>> * In serial_down() the test_and_clear_bit must be executed before
>> leaving the function if the device is a serial console
>> * serial_release() should no longer protect for the console condition
>> due to the 2.6.32 interface changes. The actual low level close
>> protection is accounted for in serial_down()
>>
>> Signed-off-by: Jason Wessel <[email protected]>
>> Cc: Greg KH <[email protected]>
>> Cc: Alan Stern <[email protected]>
>>
>> ---
>> drivers/usb/serial/usb-serial.c | 28 +++++++++++++---------------
>> 1 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
>> index ff75a35..3d35ed2 100644
>> --- a/drivers/usb/serial/usb-serial.c
>> +++ b/drivers/usb/serial/usb-serial.c
>> @@ -267,10 +267,14 @@ static int serial_open(struct tty_struct *tty, struct file *filp)
>> if (mutex_lock_interruptible(&port->mutex))
>> return -ERESTARTSYS;
>> mutex_lock(&serial->disc_mutex);
>> - if (serial->disconnected)
>> + if (serial->disconnected) {
>> retval = -ENODEV;
>> - else
>> - retval = port->serial->type->open(tty, port);
>> + } else {
>> + if (port->console)
>> + retval = 0;
>> + else
>> + retval = port->serial->type->open(tty, port);
>> + }
>> mutex_unlock(&serial->disc_mutex);
>> mutex_unlock(&port->mutex);
>> if (retval)
>>
>
> This part of the patch shouldn't be necessary. The console device
> should always be open.
>
>
With your other patch you just mentioned, I absolutely agree. This does
not need to be tracked in this manner and I don't believe this patch is
needed either.
Alan Stern wrote:
> On Wed, 16 Sep 2009, Jason Wessel wrote:
>
>
>> During the 2.6.32 development the use of serial->disc_mutex was
>> introduced. In usb_console_setup() the call to
>> usb_serial_get_by_index() will obtain this mutex. The
>> usb_console_setup() must release the mutex before it completes else
>> later calls to the usb serial core will hang.
>>
>
> Apparently you didn't see my patch:
>
> http://marc.info/?l=linux-usb&m=125209260714221&w=2
>
> Did this not get into linus-next? Greg?
>
>
Maybe it was not merged at the time. We'll wait to hear from Greg.
Certainly I would opt to use your more complete implementation.
Jason.
On Wed, 16 Sep 2009, Jason Wessel wrote:
> The first open of the usb serial HW has the termios initialized to the
> default of 9600 baud, and this will override what ever was setup via
> the original console initialization.
I don't understand. The first open of the console hardware occurs in
console.c as part of usb_console_setup(), and it uses the baud rate
passed in via the console options. Why does anything need to get
saved?
Alan Stern
Alan Stern wrote:
> On Wed, 16 Sep 2009, Jason Wessel wrote:
>
>
>> The first open of the usb serial HW has the termios initialized to the
>> default of 9600 baud, and this will override what ever was setup via
>> the original console initialization.
>>
>
> I don't understand. The first open of the console hardware occurs in
> console.c as part of usb_console_setup(), and it uses the baud rate
> passed in via the console options. Why does anything need to get
> saved?
>
The initialization in console.c is done with a dummy tty structure that
is thrown away. It is thrown away because it was not connected to any
device handle.
Any new tty that is opened defaults to 9600 baud, for the mct_u232
driver as an example, if you boot the system with
console=ttyUSB0,115200, when mingetty starts it will inherit 9600 from
the first tty that is opened, vs reading the value from the HW. Even if
you have a pl2303 and boot with the 115200 baud, you can run "stty -a <
/dev/ttyUSB0" and you will see it reports it is running at 9600. The
pl2303 driver just happens to work because it handles the baud differently.
I elected to save the baud because:
1) There was no uniform interface to read the current baud from the usb
hardware
2) There was no documented API for getting tty to attach when you create
the initial console.
If there is a better way to fix this, it would be good to continue the
discussion.
Thanks,
Jason.
On Wed, 16 Sep 2009, Jason Wessel wrote:
> Alan Stern wrote:
> > On Wed, 16 Sep 2009, Jason Wessel wrote:
> >
> >
> >> The first open of the usb serial HW has the termios initialized to the
> >> default of 9600 baud, and this will override what ever was setup via
> >> the original console initialization.
> >>
> >
> > I don't understand. The first open of the console hardware occurs in
> > console.c as part of usb_console_setup(), and it uses the baud rate
> > passed in via the console options. Why does anything need to get
> > saved?
> >
>
> The initialization in console.c is done with a dummy tty structure that
> is thrown away. It is thrown away because it was not connected to any
> device handle.
>
> Any new tty that is opened defaults to 9600 baud, for the mct_u232
> driver as an example, if you boot the system with
> console=ttyUSB0,115200, when mingetty starts it will inherit 9600 from
> the first tty that is opened, vs reading the value from the HW. Even if
> you have a pl2303 and boot with the 115200 baud, you can run "stty -a <
> /dev/ttyUSB0" and you will see it reports it is running at 9600. The
> pl2303 driver just happens to work because it handles the baud differently.
>
> I elected to save the baud because:
>
> 1) There was no uniform interface to read the current baud from the usb
> hardware
>
> 2) There was no documented API for getting tty to attach when you create
> the initial console.
>
> If there is a better way to fix this, it would be good to continue the
> discussion.
I guess a better approach would be not to throw away the "dummy" tty
structure, thereby keeping the termios structure as well. But I don't
know if this is practical.
How do other serial console drivers handle this?
Alan Stern
Alan Stern wrote:
> On Wed, 16 Sep 2009, Jason Wessel wrote:
>
>
>> Alan Stern wrote:
>>
>>> On Wed, 16 Sep 2009, Jason Wessel wrote:
>>>
>>>
>>>
>>>> The first open of the usb serial HW has the termios initialized to the
>>>> default of 9600 baud, and this will override what ever was setup via
>>>> the original console initialization.
>>>>
>>>>
>>> I don't understand. The first open of the console hardware occurs in
>>> console.c as part of usb_console_setup(), and it uses the baud rate
>>> passed in via the console options. Why does anything need to get
>>> saved?
>>>
>>>
>> The initialization in console.c is done with a dummy tty structure that
>> is thrown away. It is thrown away because it was not connected to any
>> device handle.
>>
>> Any new tty that is opened defaults to 9600 baud, for the mct_u232
>> driver as an example, if you boot the system with
>> console=ttyUSB0,115200, when mingetty starts it will inherit 9600 from
>> the first tty that is opened, vs reading the value from the HW. Even if
>> you have a pl2303 and boot with the 115200 baud, you can run "stty -a <
>> /dev/ttyUSB0" and you will see it reports it is running at 9600. The
>> pl2303 driver just happens to work because it handles the baud differently.
>>
>> I elected to save the baud because:
>>
>> 1) There was no uniform interface to read the current baud from the usb
>> hardware
>>
>> 2) There was no documented API for getting tty to attach when you create
>> the initial console.
>>
>> If there is a better way to fix this, it would be good to continue the
>> discussion.
>>
>
> I guess a better approach would be not to throw away the "dummy" tty
> structure, thereby keeping the termios structure as well. But I don't
> know if this is practical.
>
> How do other serial console drivers handle this?
>
>
My answer goes back to the point #1.
If we take the 8250 driver for example, it reads the HW and force sets
the baud into the termios structure of the real tty. There are several
layers but in the example case in 8250.c, the serial8250_set_termios()
calls uart_get_baud_rate() to establish "the baud rate to use".
As far as I could tell even the uart based serial drivers use a dummy
termios structure for the initial setup of the console, but do so with
stack memory instead of heap memory.
It looked like the API for the tty subsystem was gradually changing such
that the ldisc structure could ultimately get shared for use with the
console, but this work is not complete, so we are somewhere in between.
The question is what is an acceptable solution?
Depending on the route you go you could modify part of the tty
subsystem, the high level usb serial API to work similarly to the uart
based serial api.
Jason.
> It looked like the API for the tty subsystem was gradually changing such
> that the ldisc structure could ultimately get shared for use with the
> console, but this work is not complete, so we are somewhere in between.
The mechanics of this are already there. A tty drive can manage its own
termios structs, and the termios structs are pointers in the tty structure
> The question is what is an acceptable solution?
Take a look at usb_serial.c:serial_install. You can do the console state
preservation there and cleanly.
Alan Cox wrote:
>> It looked like the API for the tty subsystem was gradually changing such
>> that the ldisc structure could ultimately get shared for use with the
>> console, but this work is not complete, so we are somewhere in between.
>
> The mechanics of this are already there. A tty drive can manage its own
> termios structs, and the termios structs are pointers in the tty structure
>
>> The question is what is an acceptable solution?
>
> Take a look at usb_serial.c:serial_install. You can do the console state
> preservation there and cleanly.
>
Thanks for the pointer Alan. That solves the termios initialization
piece nicely.
In the usb git tree the serial_install() got removed, but I can see
the init_termios structure that gets setup in usb_serial_init().
The patch will look something like the code below, but we have to sort
out the patch tree with Alan Stern's changes to usb/serial/console.c.
Thanks,
Jason.
Signed-off-by: Jason Wessel <[email protected]>
---
drivers/usb/serial/console.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -65,7 +65,7 @@ static int usb_console_setup(struct cons
struct usb_serial_port *port;
int retval = 0;
struct tty_struct *tty = NULL;
- struct ktermios *termios = NULL, dummy;
+ struct ktermios dummy;
dbg("%s", __func__);
@@ -136,14 +136,7 @@ static int usb_console_setup(struct cons
goto reset_open_count;
}
kref_init(&tty->kref);
- termios = kzalloc(sizeof(*termios), GFP_KERNEL);
- if (!termios) {
- retval = -ENOMEM;
- err("no more memory");
- goto free_tty;
- }
- memset(&dummy, 0, sizeof(struct ktermios));
- tty->termios = termios;
+ tty->termios = &usb_serial_tty_driver->init_termios;
tty_port_tty_set(&port->port, tty);
}
@@ -156,16 +149,15 @@ static int usb_console_setup(struct cons
if (retval) {
err("could not open USB console port");
- goto free_termios;
+ goto clear_port_tty_set;
}
if (serial->type->set_termios) {
- termios->c_cflag = cflag;
- tty_termios_encode_baud_rate(termios, baud, baud);
+ tty->termios->c_cflag = cflag;
+ tty_termios_encode_baud_rate(tty->termios, baud, baud);
serial->type->set_termios(tty, port, &dummy);
tty_port_tty_set(&port->port, NULL);
- kfree(termios);
kfree(tty);
}
}
@@ -180,13 +172,11 @@ static int usb_console_setup(struct cons
out:
return retval;
-free_termios:
- kfree(termios);
+clear_port_tty_set:
tty_port_tty_set(&port->port, NULL);
-free_tty:
kfree(tty);
reset_open_count:
- port->port.count = 0;
+ --port->port.count;
goto out;
}
> - memset(&dummy, 0, sizeof(struct ktermios));
> - tty->termios = termios;
> + tty->termios = &usb_serial_tty_driver->init_termios;
That will corrupt the master one which is a reference used for many ports.
You need the serial_install stuff or something similar resurrecting to do
this properly.
On Thu, 17 Sep 2009 14:08:28 +0100
Alan Cox <[email protected]> wrote:
>
> > - memset(&dummy, 0, sizeof(struct ktermios));
> > - tty->termios = termios;
> > + tty->termios = &usb_serial_tty_driver->init_termios;
>
> That will corrupt the master one which is a reference used for many ports.
>
>
> You need the serial_install stuff or something similar resurrecting to do
> this properly.
Here's a suggestion on how to do it.
Add a tty->ops->init_termios()
Make tty_init_termios() read
if (tp == NULL) {
tp = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
if (tp == NULL)
return -ENOMEM;
memcpy(tp, &tty->driver->init_termios,
sizeof(struct ktermios));
if (tty->ops->init_termios)
tty->ops->init_termios(tty);
tty->driver->termios[idx] = tp;
}
Thats a good deal cleaner than the ->install override in the USB patches
Greg inherited and will be usable for cleaning up USB stuff too.
Alan Cox wrote:
> On Thu, 17 Sep 2009 14:08:28 +0100
> Alan Cox <[email protected]> wrote:
>
>
>>> - memset(&dummy, 0, sizeof(struct ktermios));
>>> - tty->termios = termios;
>>> + tty->termios = &usb_serial_tty_driver->init_termios;
>>>
>> That will corrupt the master one which is a reference used for many ports.
>>
>>
I agree.
>> You need the serial_install stuff or something similar resurrecting to do
>> this properly.
>>
>
> Here's a suggestion on how to do it.
>
> Add a tty->ops->init_termios()
>
> Make tty_init_termios() read
>
> if (tp == NULL) {
> tp = kzalloc(sizeof(struct ktermios[2]), GFP_KERNEL);
> if (tp == NULL)
> return -ENOMEM;
> memcpy(tp, &tty->driver->init_termios,
> sizeof(struct ktermios));
> if (tty->ops->init_termios)
> tty->ops->init_termios(tty);
> tty->driver->termios[idx] = tp;
> }
>
> Thats a good deal cleaner than the ->install override in the USB patches
> Greg inherited and will be usable for cleaning up USB stuff too.
>
>
Sure we can add another layer of init_termios callbacks, but where is
the right place to store the termios per console?
Should the "struct console co*" get a new member for the baud, or should
it get a termios? The source of the problem is how to inherit the
termios setting from the first user (the console code in this case).
Before making several more implementations it is probably worth
discussing few more of the details. In the current code the ->install
override has the opportunity to apply the fixup by copying something
from a "struct console co* termios" if one existed.
Certainly you could do the same thing with the proposed call back you
mentioned, but then the serial_install() needs to be removed from the
usb code and the power management setup would need to find a new home...
or a copy of the new call back would need to go into the
serial_install() as well.
Jason.
> Should the "struct console co*" get a new member for the baud, or should
> it get a termios? The source of the problem is how to inherit the
> termios setting from the first user (the console code in this case).
I think the console should probably have a ktermios structure and that is
something which in future could become more generic (arguably in a saner
world the termios would belong to the tty_port in the end as and when
everything is a tty port - which would make a whole ton of things a lot
lot more logical)
On Wed, Sep 16, 2009 at 11:25:40PM -0400, Alan Stern wrote:
> On Wed, 16 Sep 2009, Jason Wessel wrote:
>
> > During the 2.6.32 development the use of serial->disc_mutex was
> > introduced. In usb_console_setup() the call to
> > usb_serial_get_by_index() will obtain this mutex. The
> > usb_console_setup() must release the mutex before it completes else
> > later calls to the usb serial core will hang.
>
> Apparently you didn't see my patch:
>
> http://marc.info/?l=linux-usb&m=125209260714221&w=2
>
> Did this not get into linus-next? Greg?
Bah, my fault, I forgot this after I fixed up my tree and took your
other patches.
I've now added this one, so all should be good.
My appologies.
greg k-h
Alan Cox wrote:
>
> I think the console should probably have a ktermios structure and that is
> something which in future could become more generic (arguably in a saner
> world the termios would belong to the tty_port in the end as and when
> everything is a tty port - which would make a whole ton of things a lot
> lot more logical)
What about something like the patch shown below?
I did come up with a question. I was curious where you envisioned the kfree() for the ktermios allocated in the usb serial?
Perhaps we don't care because it will never go away but I don't want to add a memory leak. Another option would to make the termios in the struct console not be a pointer, but that obviously makes the structure a bit bigger. Did you have a suggestion?
Thanks,
Jason.
---
drivers/char/tty_io.c | 2 ++
drivers/usb/serial/console.c | 25 +++++++++++++------------
drivers/usb/serial/usb-serial.c | 15 +++++++++++++--
include/linux/console.h | 1 +
include/linux/tty_driver.h | 1 +
include/linux/usb/serial.h | 2 +-
6 files changed, 31 insertions(+), 15 deletions(-)
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -66,7 +66,7 @@ static int usb_console_setup(struct cons
struct usb_serial_port *port;
int retval;
struct tty_struct *tty = NULL;
- struct ktermios *termios = NULL, dummy;
+ struct ktermios dummy;
dbg("%s", __func__);
@@ -141,14 +141,17 @@ static int usb_console_setup(struct cons
goto reset_open_count;
}
kref_init(&tty->kref);
- termios = kzalloc(sizeof(*termios), GFP_KERNEL);
- if (!termios) {
+
+ if (!co->termios)
+ co->termios = kzalloc(sizeof(struct ktermios),
+ GFP_KERNEL);
+ if (!co->termios) {
retval = -ENOMEM;
err("no more memory");
goto free_tty;
}
memset(&dummy, 0, sizeof(struct ktermios));
- tty->termios = termios;
+ tty->termios = co->termios;
tty_port_tty_set(&port->port, tty);
}
@@ -161,16 +164,15 @@ static int usb_console_setup(struct cons
if (retval) {
err("could not open USB console port");
- goto free_termios;
+ goto free_port;
}
if (serial->type->set_termios) {
- termios->c_cflag = cflag;
- tty_termios_encode_baud_rate(termios, baud, baud);
+ co->termios->c_cflag = cflag;
+ tty_termios_encode_baud_rate(co->termios, baud, baud);
serial->type->set_termios(tty, port, &dummy);
tty_port_tty_set(&port->port, NULL);
- kfree(termios);
kfree(tty);
}
set_bit(ASYNCB_INITIALIZED, &port->port.flags);
@@ -180,13 +182,12 @@ static int usb_console_setup(struct cons
--port->port.count;
/* The console is special in terms of closing the device so
* indicate this port is now acting as a system console. */
- port->console = 1;
+ port->console = co;
mutex_unlock(&serial->disc_mutex);
return retval;
- free_termios:
- kfree(termios);
+ free_port:
tty_port_tty_set(&port->port, NULL);
free_tty:
kfree(tty);
@@ -312,7 +313,7 @@ void usb_serial_console_exit(void)
{
if (usbcons_info.port) {
unregister_console(&usbcons);
- usbcons_info.port->console = 0;
+ usbcons_info.port->console = NULL;
usbcons_info.port = NULL;
}
}
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -104,6 +104,7 @@ struct console {
short flags;
short index;
int cflag;
+ struct ktermios *termios;
void *data;
struct console *next;
};
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -1175,6 +1175,8 @@ int tty_init_termios(struct tty_struct *
memcpy(tp, &tty->driver->init_termios,
sizeof(struct ktermios));
tty->driver->termios[idx] = tp;
+ if (tty->ops->init_termios)
+ tty->ops->init_termios(tty);
}
tty->termios = tp;
tty->termios_locked = tp + 1;
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -259,6 +259,7 @@ struct tty_operations {
unsigned int set, unsigned int clear);
int (*resize)(struct tty_struct *tty, struct winsize *ws);
int (*set_termiox)(struct tty_struct *tty, struct termiox *tnew);
+ void (*init_termios)(struct tty_struct *tty);
#ifdef CONFIG_CONSOLE_POLL
int (*poll_init)(struct tty_driver *driver, int line, char *options);
int (*poll_get_char)(struct tty_driver *driver, int line);
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -35,6 +35,7 @@
#include <linux/serial.h>
#include <linux/usb.h>
#include <linux/usb/serial.h>
+#include <linux/console.h>
#include "pl2303.h"
/*
@@ -212,6 +213,7 @@ static int serial_install(struct tty_dri
if (!try_module_get(serial->type->driver.owner))
goto error_module_get;
+ tty->driver_data = port;
/* perform the standard setup */
retval = tty_init_termios(tty);
if (retval)
@@ -227,8 +229,6 @@ static int serial_install(struct tty_dri
if (serial->type->init_termios)
serial->type->init_termios(tty);
- tty->driver_data = port;
-
/* Final install (we use the default method) */
tty_driver_kref_get(driver);
tty->count++;
@@ -237,6 +237,7 @@ static int serial_install(struct tty_dri
error_get_interface:
error_init_termios:
+ tty->driver_data = NULL;
module_put(serial->type->driver.owner);
error_module_get:
error_no_port:
@@ -245,6 +246,15 @@ static int serial_install(struct tty_dri
return retval;
}
+static void serial_init_termios(struct tty_struct *tty)
+{
+ struct usb_serial_port *port = tty->driver_data;
+
+ if (port->console && port->console->termios)
+ memcpy(tty->driver->termios[tty->index],
+ port->console->termios, sizeof(struct ktermios));
+}
+
static int serial_open(struct tty_struct *tty, struct file *filp)
{
struct usb_serial_port *port = tty->driver_data;
@@ -1202,6 +1212,7 @@ static const struct tty_operations seria
.shutdown = serial_release,
.install = serial_install,
.proc_fops = &serial_proc_fops,
+ .init_termios = serial_init_termios,
};
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -106,7 +106,7 @@ struct usb_serial_port {
struct work_struct work;
char throttled;
char throttle_req;
- char console;
+ struct console *console;
unsigned long sysrq; /* sysrq timeout */
struct device dev;
enum port_dev_state dev_state;
Alan:
Jason and I discussed how console devices (both USB-serial and others)
should be handled. My feeling is that these devices should persist
throughout the entire life of the kernel, not disappearing until the
system goes down. So for example, if ttyUSB0 were to be a console and
the user were to unplug the corresponding USB serial device, ttyUSB0
would remain in existence -- unusable but still there -- and any new
USB serial devices would show up as ttyUSB1 or higher.
In addition, console output shouldn't be sent through these devices
before the driver has properly initialized the hardware. And once
initialized, the hardware should never be shut down.
In the end, this amounts to making the kernel open each console device
when it is registered as a console. The open call would create a tty
and termios to go with the device. The open "file" reference would
never be closed, preventing the tty and termios from being released and
preventing the hardware from being shut down.
I don't know what the best way is to accomplish this. Create dummy
inode and file structs and pass them to the usual tty_open() routine?
(But then what about hangup event handling?) What do you think?
Alan Stern
> I don't know what the best way is to accomplish this. Create dummy
> inode and file structs and pass them to the usual tty_open() routine?
> (But then what about hangup event handling?) What do you think?
What I long term envisioned was that
- Every tty (or at least every interesting tty) would have a tty_port
object for the hardware [done now for all consoles but vt]
- Every tty port object would have an "output" method
- The tty->termios would move to the tty_port
- The tty receive buffers would move to the tty_port (only needed for a
console that supports input)
At that point
- The tty lock/refcount isn't needed all the time for the receive data
paths which speeds it up a fair bit
- A console can be implemented without a tty_struct anywhere in sight
- Flow control and speed setting can be done on hardware generically on
resume paths
That fixes the lifetime and magic object invention issues that plague the
current console.
I still think that is the right way to fix it.
Alan