[PATCH RESEND 0/2] two serial_core suspend/resume fixes

[PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Jason Wang]

Sorry for resending this thread. Last thread is forgot to CC
Greg Kroah-Hartman.

The [1/2] fix this situation:
we set no_console_supend and console=ttyS0 to bootargs, then bootup
the kernel, the boot logs will print out from ttyS0. When we execute
echo mem > /sys/power/state, the system will suspend, we press a
key(or other wakeup trigger) to resume the system, but the ttyS0 can't
work anymore.

The [2/2] fix this situation:
we set console=ttyS0 to bootargs, then bootup the kernel, the boot
logs will print out from ttyS0, this time we set ttyS1 as tty and
login for shell. When we execute echo mem > /sys/power/state, the
system will suspend, we press a key(or other wakeup trigger) to
resume the system, but the ttyS0 can't work anymore.

[PATCH RESEND 1/2] serial-core: skip call set_termios/console_start when no_console_suspend

[Jason Wang]

The commit 4547be7 rewrites suspend and resume functions, this
introduces a problem on the OMAP3EVM platoform. when the kernel boots
with no_console_suspend and we suspend the kernel, then resume it,
the serial console will be not usable. This problem should be common
for all platforms.
The cause for this problem is that when enter suspend, if we choose
no_console_suspend, the console_stop will be skiped. But in resume
function, the console port will be set to uninitialized state by
calling set_termios function and the console_start is called without
checking whether the no_console_suspend is set, Now fix it.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/serial/serial_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index cd85112..ff21200 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2065,7 +2065,7 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 	/*
 	 * Re-enable the console device after suspending.
 	 */
-	if (uart_console(uport)) {
+	if (console_suspend_enabled && uart_console(uport)) {
 		uart_change_pm(state, 0);
 		uport->ops->set_termios(uport, &termios, NULL);
 		console_start(uport->cons);
-- 
1.5.6.5

[PATCH RESEND 2/2] serial-core: restore termios settings when resume console ports

[Jason Wang]

The commit 4547be7 rewrites suspend and resume functions. According
to this rewrite, when a serial port is a printk console device and
can suspend(without set no_console_suspend flag), it will definitely
call set_termios function during its resume, but parameter termios
isn't initialized, this will pass an unpredictable config to the
serial port. If this serial port is not a userspace opened tty device
, a suspend and resume action will make this serial port unusable.
I.E. ttyS0 is a printk console device, ttyS1 or keyboard+display is
userspace tty device, a suspend/resume action will make ttyS0
unusable.

If a serial port is both a printk console device and an opened tty
device, this issue can be overcome because it will call set_termios
again with the correct parameter in the uart_change_speed function.

Refer to the deleted content of commit 4547be7, revert parts relate
to restore settings into parameter termios. It is safe because if
a serial port is a printk console only device, the only meaningful
field in termios is c_cflag and its old config is saved in
uport->cons->cflag, if this port is also an opened tty device,
it will clear uport->cons->cflag in the uart_open and the old config
is saved in tty->termios.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/serial/serial_core.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index ff21200..bc6cddd 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2066,6 +2066,18 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 	 * Re-enable the console device after suspending.
 	 */
 	if (console_suspend_enabled && uart_console(uport)) {
+		/*
+		 * First try to use the console cflag setting.
+		 */
+		memset(&termios, 0, sizeof(struct ktermios));
+		termios.c_cflag = uport->cons->cflag;
+
+		/*
+		 * If that's unset, use the tty termios setting.
+		 */
+		if (port->tty && port->tty->termios && termios.c_cflag == 0)
+			termios = *(port->tty->termios);
+
 		uart_change_pm(state, 0);
 		uport->ops->set_termios(uport, &termios, NULL);
 		console_start(uport->cons);
-- 
1.5.6.5

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

Jason Wang wrote:
> Sorry for resending this thread. Last thread is forgot to CC
> Greg Kroah-Hartman.

Well, I experienced no_console_supend breakage on my PXA270 based Zaurus
SL-C3200 (terrier/spitz) as well.

But your patches did not fix the behavior, serial port remains dead
after resume with no_console_supend.

> The [1/2] fix this situation:
> we set no_console_supend and console=ttyS0 to bootargs, then bootup
> the kernel, the boot logs will print out from ttyS0. When we execute
> echo mem > /sys/power/state, the system will suspend, we press a
> key(or other wakeup trigger) to resume the system, but the ttyS0 can't
> work anymore.
> 
> The [2/2] fix this situation:
> we set console=ttyS0 to bootargs, then bootup the kernel, the boot
> logs will print out from ttyS0, this time we set ttyS1 as tty and
> login for shell. When we execute echo mem > /sys/power/state, the
> system will suspend, we press a key(or other wakeup trigger) to
> resume the system, but the ttyS0 can't work anymore.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +420 284 028 966
190 00 Praha 9                                fax: +420 284 028 951
Czech Republic                                http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

jason wang wrote:

>         Well, I experienced no_console_supend breakage on my PXA270
>         based Zaurus
>         SL-C3200 (terrier/spitz) as well.
>         
>         But your patches did not fix the behavior, serial port remains
>         dead
>         after resume with no_console_supend.
>  
> Very strange, I have validated these patches on ti_omap3530evm,
> fsl_imx31pdk and fsl_imx51pdk.
> They work fine.

Well, on spitz it worked in past early after 4547be7 applying, but does
not work in current vanilla. I don't yet see why.

Well, 4547be7 was created by disabling and enabling chunks using trial
and error method comparing result of two boots (with and without
no_console_suspend) and thinking what happened, maybe I did something
bad on omap. Just one resume chunk was removed: "short resume with
no_console_suspend" not going through the whole resume process.

> When we set no_console_suspend to bootargs,  the suspend process will
> skip most sub-callings in the
> serial_core.c->uart_suspend_port(), only call ops->tx_empty().  While
> in resume process, if without my
> first patch, it will call uart_change_pm(), ops->set_termios() and
> console_start(), these callings will make
> the console uart unusable, but if apply my first patch,  it will call
> nothing in the resume process. So
> apply my first patch will balance suspend and resume sub-callings.

I tried to boot:
- without no_console_suspend: works both with and without your patches
- with no_console_suspend: does not work both with and without your
  patches

So your patches surely don't mean a regression here.

> So i guess, your issue is not here. Maybe other parts(like gpio/clock)
> of suspend/resume affect your UART.

Yes, it is pretty well possible, but the breakage is triggered by the
no_console_suspend boot parameter as well.

My 4547be7 surely fixed no_console_suspend on Zaurus spitz, and OLPC XO
laptop. But over the time, no_console_suspend stopped to work again, at
least on spitz. I started to debug it independently on you (see below).

So here is the story:

b5b82df6 May 2007 breaks no_console_suspend on some ARM devices
  related ML subject:
  Possible suspend/resume regression in .32-rc?

4547be7  Dec 2009 fixes no_console_suspend on spitz and OLPC.
  related ML subject:
  serial-core: resume serial hardware with no_console_suspend

???????  ??? 2010 breaks no_console_suspend at least on spitz

your patch        fixes no_console_suspend on omap but not on spitz
  related ML subject:
  two serial_core suspend/resume fixes


Attaching my temporary debugging patch (tests done before applying your
patch).

Here is the output without no_console_suspend:
SUSPEND: 1 1 0 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 7
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
SUSPEND: 1 1 0 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 7
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
SUSPEND: 1 1 1 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 7
SUSPEND: 8
SUSPEND: 9
SUSPEND: 11
SUSPEND: 12
SUSPEND: 13
SUSPEND: 14
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
PM: suspend of devices complete after 311.219 msecs
PM: late suspend of devices complete after 0.615 msecs
PM: early resume of devices complete after 1458.544 msecs
RESUME: 1 1 1 1073741824
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 7
RESUME: 8
RESUME: 9
RESUME: 10
RESUME: 11
RESUME: 13
RESUME: 14
RESUME: 15
RESUME: 1 1 0 0
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 14
RESUME: 15
RESUME: 1 1 0 0
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 14
RESUME: 15

Here is the output with no_console_suspend (serial console was broken
after resume):
SUSPEND: 1 0 0 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 7
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
SUSPEND: 1 0 0 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 7
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
SUSPEND: 1 0 1 0
SUSPEND: 2
SUSPEND: 3
SUSPEND: 6
SUSPEND: 8
SUSPEND: 11
SUSPEND: 14
SUSPEND: 15
SUSPEND: 16
SUSPEND: 17
SUSPEND: 18
PM: suspend of devices complete after 358.314 msecs
PM: late suspend of devices complete after 0.637 msecs
PM: early resume of devices complete after 1453.923 msecs
RESUME: 1 0 1 0
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 7
RESUME: 8
RESUME: 14
RESUME: 15
RESUME: 1 0 0 0
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 14
RESUME: 15
RESUME: 1 0 0 0
RESUME: 2
RESUME: 3
RESUME: 6
RESUME: 14
RESUME: 15

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index cd85112..6146712 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -1983,25 +1983,35 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 	struct uart_match match = {uport, drv};
 	struct tty_struct *tty;
 
+printk(KERN_INFO "SUSPEND: 1 %d %d %ld\n", console_suspend_enabled, uart_console(uport), port->flags & ASYNC_SUSPENDED);
 	mutex_lock(&port->mutex);
 
+printk(KERN_INFO "SUSPEND: 2\n");
 	/* Must be inside the mutex lock until we convert to tty_port */
 	tty = port->tty;
 
 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+printk(KERN_INFO "SUSPEND: 3\n");
 	if (device_may_wakeup(tty_dev)) {
+printk(KERN_INFO "SUSPEND: 4\n");
 		enable_irq_wake(uport->irq);
 		put_device(tty_dev);
 		mutex_unlock(&port->mutex);
+printk(KERN_INFO "SUSPEND: 5\n");
 		return 0;
 	}
+printk(KERN_INFO "SUSPEND: 6\n");
 	if (console_suspend_enabled || !uart_console(uport))
+{
+printk(KERN_INFO "SUSPEND: 7\n");
 		uport->suspended = 1;
+}
 
 	if (port->flags & ASYNC_INITIALIZED) {
 		const struct uart_ops *ops = uport->ops;
 		int tries;
 
+printk(KERN_INFO "SUSPEND: 8\n");
 		if (console_suspend_enabled || !uart_console(uport)) {
 			set_bit(ASYNCB_SUSPENDED, &port->flags);
 			clear_bit(ASYNCB_INITIALIZED, &port->flags);
@@ -2011,13 +2021,17 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 			ops->set_mctrl(uport, 0);
 			ops->stop_rx(uport);
 			spin_unlock_irq(&uport->lock);
+printk(KERN_INFO "SUSPEND: 9\n");
 		}
 
 		/*
 		 * Wait for the transmitter to empty.
 		 */
 		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
+{
+printk(KERN_INFO "SUSPEND: 10\n");
 			msleep(10);
+}
 		if (!tries)
 			printk(KERN_ERR "%s%s%s%d: Unable to drain "
 					"transmitter\n",
@@ -2026,20 +2040,30 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
 			       drv->dev_name,
 			       drv->tty_driver->name_base + uport->line);
 
+printk(KERN_INFO "SUSPEND: 11\n");
 		if (console_suspend_enabled || !uart_console(uport))
+{
+printk(KERN_INFO "SUSPEND: 12\n");
 			ops->shutdown(uport);
+printk(KERN_INFO "SUSPEND: 13\n");
+}
+printk(KERN_INFO "SUSPEND: 14\n");
 	}
 
 	/*
 	 * Disable the console device before suspending.
 	 */
+printk(KERN_INFO "SUSPEND: 15\n");
 	if (console_suspend_enabled && uart_console(uport))
 		console_stop(uport->cons);
 
+printk(KERN_INFO "SUSPEND: 16\n");
 	if (console_suspend_enabled || !uart_console(uport))
 		uart_change_pm(state, 3);
 
+printk(KERN_INFO "SUSPEND: 17\n");
 	mutex_unlock(&port->mutex);
+printk(KERN_INFO "SUSPEND: 18\n");
 
 	return 0;
 }
@@ -2052,26 +2076,35 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 	struct uart_match match = {uport, drv};
 	struct ktermios termios;
 
+printk(KERN_INFO "RESUME: 1 %d %d %ld\n", console_suspend_enabled, uart_console(uport), port->flags & ASYNC_SUSPENDED);
 	mutex_lock(&port->mutex);
+printk(KERN_INFO "RESUME: 2\n");
 
 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+printk(KERN_INFO "RESUME: 3\n");
 	if (!uport->suspended && device_may_wakeup(tty_dev)) {
+printk(KERN_INFO "RESUME: 4\n");
 		disable_irq_wake(uport->irq);
 		mutex_unlock(&port->mutex);
+printk(KERN_INFO "RESUME: 5\n");
 		return 0;
 	}
 	uport->suspended = 0;
 
+printk(KERN_INFO "RESUME: 6\n");
 	/*
 	 * Re-enable the console device after suspending.
 	 */
 	if (uart_console(uport)) {
+printk(KERN_INFO "RESUME: 7\n");
 		uart_change_pm(state, 0);
 		uport->ops->set_termios(uport, &termios, NULL);
 		console_start(uport->cons);
+printk(KERN_INFO "RESUME: 8\n");
 	}
 
 	if (port->flags & ASYNC_SUSPENDED) {
+printk(KERN_INFO "RESUME: 9\n");
 		const struct uart_ops *ops = uport->ops;
 		int ret;
 
@@ -2079,7 +2112,9 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 		spin_lock_irq(&uport->lock);
 		ops->set_mctrl(uport, 0);
 		spin_unlock_irq(&uport->lock);
+printk(KERN_INFO "RESUME: 10\n");
 		if (console_suspend_enabled || !uart_console(uport)) {
+printk(KERN_INFO "RESUME: 11\n");
 			/* Protected by port mutex for now */
 			struct tty_struct *tty = port->tty;
 			ret = ops->startup(uport);
@@ -2097,14 +2132,18 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
 				 * Clear the "initialized" flag so we won't try
 				 * to call the low level drivers shutdown method.
 				 */
+printk(KERN_INFO "RESUME: 12\n");
 				uart_shutdown(tty, state);
 			}
+printk(KERN_INFO "RESUME: 13\n");
 		}
 
 		clear_bit(ASYNCB_SUSPENDED, &port->flags);
 	}
 
+printk(KERN_INFO "RESUME: 14\n");
 	mutex_unlock(&port->mutex);
+printk(KERN_INFO "RESUME: 15\n");
 
 	return 0;
 }


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +420 284 028 966
190 00 Praha 9                                fax: +420 284 028 951
Czech Republic                                http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[wanghui]

Stanislav Brabec wrote:
> jason wang wrote:
>
>   
>>         Well, I experienced no_console_supend breakage on my PXA270
>>         based Zaurus
>>         SL-C3200 (terrier/spitz) as well.
>>         
>>         But your patches did not fix the behavior, serial port remains
>>         dead
>>         after resume with no_console_supend.
>>  
>> Very strange, I have validated these patches on ti_omap3530evm,
>> fsl_imx31pdk and fsl_imx51pdk.
>> They work fine.
>>     
>
> Well, on spitz it worked in past early after 4547be7 applying, but does
> not work in current vanilla. I don't yet see why.
>
> Well, 4547be7 was created by disabling and enabling chunks using trial
> and error method comparing result of two boots (with and without
> no_console_suspend) and thinking what happened, maybe I did something
> bad on omap. Just one resume chunk was removed: "short resume with
> no_console_suspend" not going through the whole resume process.
>
>   
Thank for your explanation.
>> When we set no_console_suspend to bootargs,  the suspend process will
>> skip most sub-callings in the
>> serial_core.c->uart_suspend_port(), only call ops->tx_empty().  While
>> in resume process, if without my
>> first patch, it will call uart_change_pm(), ops->set_termios() and
>> console_start(), these callings will make
>> the console uart unusable, but if apply my first patch,  it will call
>> nothing in the resume process. So
>> apply my first patch will balance suspend and resume sub-callings.
>>     
>
> I tried to boot:
> - without no_console_suspend: works both with and without your patches
>   
For this situation, i guess you use a single serial port both as a
printk console and as a login tty. If you use that serial port only
as a printk console, and use another serial port or (keyboard + lcd)
as login tty, you will see the the printk console serial port can't
work after resume.

The cause of this issue is:
in the uart_suspend_port(), we check ASYNC_INITIALIZED,  if
it is set, we will stop this serial port, but ASYNC_INITIALIZED
is only set in the open(), so if this serial port is only used as
printk console, this flag will not set for this serial port.
So in the uart_suspend_port()

    if (port->flags & ASYNC_INITIALIZED) {
        ....
    }
will not be executed.

as a result, in the uart_resume_port()
    if (port->flags & ASYNC_SUSPENDED) {
  ...
    }
will not be executed.

In the resume process, the only serial driver relating sub-callings is
uport->ops->set_termios(), but the parameter passed in is not initialized,
we can imagine this serial port will not work anymore after this resume.

My 2rd patch is for this issue.
> - with no_console_suspend: does not work both with and without your
>   patches
>
>   
In this situation, the uart_suspend_port() will not call any serial driver
relating sub-callings except ops->tx_empty().

In the resume process, if apply my 1st patch, the uart_resume_port()
will not call any serial driver relating sub-callings just like the suspend
process does, i don't know why your serial can't work.

If without my 1st patch, the uart_resume_port() will call 
uport->ops->set_termios(),
but the parameter passed in is not initialized, moreover, it will call 
console_start(),
this calling is not balance with suspend process because we don't call 
console_stop()
in the suspend process.

Thanks,
Jason.
> So your patches surely don't mean a regression here.
>
>   
Not a 100% regression.

>> So i guess, your issue is not here. Maybe other parts(like gpio/clock)
>> of suspend/resume affect your UART.
>>     
>
> Yes, it is pretty well possible, but the breakage is triggered by the
> no_console_suspend boot parameter as well.
>
> My 4547be7 surely fixed no_console_suspend on Zaurus spitz, and OLPC XO
> laptop. But over the time, no_console_suspend stopped to work again, at
> least on spitz. I started to debug it independently on you (see below).
>
> So here is the story:
>
> b5b82df6 May 2007 breaks no_console_suspend on some ARM devices
>   related ML subject:
>   Possible suspend/resume regression in .32-rc?
>
> 4547be7  Dec 2009 fixes no_console_suspend on spitz and OLPC.
>   related ML subject:
>   serial-core: resume serial hardware with no_console_suspend
>
> ???????  ??? 2010 breaks no_console_suspend at least on spitz
>
> your patch        fixes no_console_suspend on omap but not on spitz
>   related ML subject:
>   two serial_core suspend/resume fixes
>
>
> Attaching my temporary debugging patch (tests done before applying your
> patch).
>
> Here is the output without no_console_suspend:
> SUSPEND: 1 1 0 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 7
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> SUSPEND: 1 1 0 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 7
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> SUSPEND: 1 1 1 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 7
> SUSPEND: 8
> SUSPEND: 9
> SUSPEND: 11
> SUSPEND: 12
> SUSPEND: 13
> SUSPEND: 14
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> PM: suspend of devices complete after 311.219 msecs
> PM: late suspend of devices complete after 0.615 msecs
> PM: early resume of devices complete after 1458.544 msecs
> RESUME: 1 1 1 1073741824
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 7
> RESUME: 8
> RESUME: 9
> RESUME: 10
> RESUME: 11
> RESUME: 13
> RESUME: 14
> RESUME: 15
> RESUME: 1 1 0 0
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 14
> RESUME: 15
> RESUME: 1 1 0 0
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 14
> RESUME: 15
>
> Here is the output with no_console_suspend (serial console was broken
> after resume):
> SUSPEND: 1 0 0 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 7
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> SUSPEND: 1 0 0 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 7
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> SUSPEND: 1 0 1 0
> SUSPEND: 2
> SUSPEND: 3
> SUSPEND: 6
> SUSPEND: 8
> SUSPEND: 11
> SUSPEND: 14
> SUSPEND: 15
> SUSPEND: 16
> SUSPEND: 17
> SUSPEND: 18
> PM: suspend of devices complete after 358.314 msecs
> PM: late suspend of devices complete after 0.637 msecs
> PM: early resume of devices complete after 1453.923 msecs
> RESUME: 1 0 1 0
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 7
> RESUME: 8
> RESUME: 14
> RESUME: 15
> RESUME: 1 0 0 0
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 14
> RESUME: 15
> RESUME: 1 0 0 0
> RESUME: 2
> RESUME: 3
> RESUME: 6
> RESUME: 14
> RESUME: 15
>
> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> index cd85112..6146712 100644
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
> @@ -1983,25 +1983,35 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>  	struct uart_match match = {uport, drv};
>  	struct tty_struct *tty;
>  
> +printk(KERN_INFO "SUSPEND: 1 %d %d %ld\n", console_suspend_enabled, uart_console(uport), port->flags & ASYNC_SUSPENDED);
>  	mutex_lock(&port->mutex);
>  
> +printk(KERN_INFO "SUSPEND: 2\n");
>  	/* Must be inside the mutex lock until we convert to tty_port */
>  	tty = port->tty;
>  
>  	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> +printk(KERN_INFO "SUSPEND: 3\n");
>  	if (device_may_wakeup(tty_dev)) {
> +printk(KERN_INFO "SUSPEND: 4\n");
>  		enable_irq_wake(uport->irq);
>  		put_device(tty_dev);
>  		mutex_unlock(&port->mutex);
> +printk(KERN_INFO "SUSPEND: 5\n");
>  		return 0;
>  	}
> +printk(KERN_INFO "SUSPEND: 6\n");
>  	if (console_suspend_enabled || !uart_console(uport))
> +{
> +printk(KERN_INFO "SUSPEND: 7\n");
>  		uport->suspended = 1;
> +}
>  
>  	if (port->flags & ASYNC_INITIALIZED) {
>  		const struct uart_ops *ops = uport->ops;
>  		int tries;
>  
> +printk(KERN_INFO "SUSPEND: 8\n");
>  		if (console_suspend_enabled || !uart_console(uport)) {
>  			set_bit(ASYNCB_SUSPENDED, &port->flags);
>  			clear_bit(ASYNCB_INITIALIZED, &port->flags);
> @@ -2011,13 +2021,17 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>  			ops->set_mctrl(uport, 0);
>  			ops->stop_rx(uport);
>  			spin_unlock_irq(&uport->lock);
> +printk(KERN_INFO "SUSPEND: 9\n");
>  		}
>  
>  		/*
>  		 * Wait for the transmitter to empty.
>  		 */
>  		for (tries = 3; !ops->tx_empty(uport) && tries; tries--)
> +{
> +printk(KERN_INFO "SUSPEND: 10\n");
>  			msleep(10);
> +}
>  		if (!tries)
>  			printk(KERN_ERR "%s%s%s%d: Unable to drain "
>  					"transmitter\n",
> @@ -2026,20 +2040,30 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>  			       drv->dev_name,
>  			       drv->tty_driver->name_base + uport->line);
>  
> +printk(KERN_INFO "SUSPEND: 11\n");
>  		if (console_suspend_enabled || !uart_console(uport))
> +{
> +printk(KERN_INFO "SUSPEND: 12\n");
>  			ops->shutdown(uport);
> +printk(KERN_INFO "SUSPEND: 13\n");
> +}
> +printk(KERN_INFO "SUSPEND: 14\n");
>  	}
>  
>  	/*
>  	 * Disable the console device before suspending.
>  	 */
> +printk(KERN_INFO "SUSPEND: 15\n");
>  	if (console_suspend_enabled && uart_console(uport))
>  		console_stop(uport->cons);
>  
> +printk(KERN_INFO "SUSPEND: 16\n");
>  	if (console_suspend_enabled || !uart_console(uport))
>  		uart_change_pm(state, 3);
>  
> +printk(KERN_INFO "SUSPEND: 17\n");
>  	mutex_unlock(&port->mutex);
> +printk(KERN_INFO "SUSPEND: 18\n");
>  
>  	return 0;
>  }
> @@ -2052,26 +2076,35 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  	struct uart_match match = {uport, drv};
>  	struct ktermios termios;
>  
> +printk(KERN_INFO "RESUME: 1 %d %d %ld\n", console_suspend_enabled, uart_console(uport), port->flags & ASYNC_SUSPENDED);
>  	mutex_lock(&port->mutex);
> +printk(KERN_INFO "RESUME: 2\n");
>  
>  	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> +printk(KERN_INFO "RESUME: 3\n");
>  	if (!uport->suspended && device_may_wakeup(tty_dev)) {
> +printk(KERN_INFO "RESUME: 4\n");
>  		disable_irq_wake(uport->irq);
>  		mutex_unlock(&port->mutex);
> +printk(KERN_INFO "RESUME: 5\n");
>  		return 0;
>  	}
>  	uport->suspended = 0;
>  
> +printk(KERN_INFO "RESUME: 6\n");
>  	/*
>  	 * Re-enable the console device after suspending.
>  	 */
>  	if (uart_console(uport)) {
> +printk(KERN_INFO "RESUME: 7\n");
>  		uart_change_pm(state, 0);
>  		uport->ops->set_termios(uport, &termios, NULL);
>  		console_start(uport->cons);
> +printk(KERN_INFO "RESUME: 8\n");
>  	}
>  
>  	if (port->flags & ASYNC_SUSPENDED) {
> +printk(KERN_INFO "RESUME: 9\n");
>  		const struct uart_ops *ops = uport->ops;
>  		int ret;
>  
> @@ -2079,7 +2112,9 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  		spin_lock_irq(&uport->lock);
>  		ops->set_mctrl(uport, 0);
>  		spin_unlock_irq(&uport->lock);
> +printk(KERN_INFO "RESUME: 10\n");
>  		if (console_suspend_enabled || !uart_console(uport)) {
> +printk(KERN_INFO "RESUME: 11\n");
>  			/* Protected by port mutex for now */
>  			struct tty_struct *tty = port->tty;
>  			ret = ops->startup(uport);
> @@ -2097,14 +2132,18 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>  				 * Clear the "initialized" flag so we won't try
>  				 * to call the low level drivers shutdown method.
>  				 */
> +printk(KERN_INFO "RESUME: 12\n");
>  				uart_shutdown(tty, state);
>  			}
> +printk(KERN_INFO "RESUME: 13\n");
>  		}
>  
>  		clear_bit(ASYNCB_SUSPENDED, &port->flags);
>  	}
>  
> +printk(KERN_INFO "RESUME: 14\n");
>  	mutex_unlock(&port->mutex);
> +printk(KERN_INFO "RESUME: 15\n");
>  
>  	return 0;
>  }
>
>
>   

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

wanghui wrote:   
> For this situation, i guess you use a single serial port both as a
> printk console and as a login tty.

Yes, I did exactly that.

>  If you use that serial port only
> as a printk console, and use another serial port or (keyboard + lcd)
> as login tty, you will see the the printk console serial port can't
> work after resume.

Well, I never tried this with my old patch, and I even forgot to think
about this situation. It complicates the suspend logic even more and I
understand that 4547be7 breaks it.

> as a result, in the uart_resume_port()
>     if (port->flags & ASYNC_SUSPENDED) {
>   ...
>     }
> will not be executed.
> 
> In the resume process, the only serial driver relating sub-callings is
> uport->ops->set_termios(), but the parameter passed in is not initialized,
> we can imagine this serial port will not work anymore after this resume.

Well, if you look at 4547be7 deleted chunk "/* no need to resume serial
console, it wasn't suspended */", then it may contain relevant code.
It was called with no_console_suspend before and returned. But "it
wasn't suspended" is not true: The hardware went to sleep and now is in
undefined state and this chunk failed to re-initialize it. That is why I
decided to never enter here and carefully pick parts of the whole resume
process that need to be executed. I forgot to think about "just debug
console, not a tty".

> My 2rd patch is for this issue.
> > - with no_console_suspend: does not work both with and without your
> >   patches

> In this situation, the uart_suspend_port() will not call any serial driver
> relating sub-callings except ops->tx_empty().

And setting "uport->suspended = 1". It was my intention - the console
should stay alive as long as possible.

> In the resume process, if apply my 1st patch, the uart_resume_port()
> will not call any serial driver relating sub-callings just like the suspend
> process does, i don't know why your serial can't work.

Me too, but the spitz hardware required setup, otherwise it remains in
undefined state after the resume. It worked in time of 4547be7, but not
now. Reviewing patches that affect serial_core, I don't see anything
obvious.

> If without my 1st patch, the uart_resume_port() will call 
> uport->ops->set_termios(),
> but the parameter passed in is not initialized, moreover, it will call 
> console_start(),
> this calling is not balance with suspend process because we don't call 
> console_stop()
> in the suspend process.

Does it mean any problem? If I remember correctly, there were two lines
in suspend that stopped late pre-suspend console messages. I tried to
skip them, but still wanted to call their resume counterpart (without
them, the hardware was not re-initialized correctly). Maybe there is a
better way to do it.

> > So your patches surely don't mean a regression here.
> Not a 100% regression.

No regression at all on spitz and fix on your omap.

Well, thinking about correct implementation, we need:

suspend:
- save hardware state
- but not physically stop it
- tell kernel that the port is suspended
- but prevent postponing of late pre-suspend messages

resume:
- If the port is in any use (console, login or communication) then never
  skip the hardware resume
- tell kernel that the port is in fully working state

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12                            tel: +420 284 028 966
190 00 Praha 9                                fax: +420 284 028 951
Czech Republic                                http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

Stanislav Brabec wrote:
> wanghui wrote:   
> > For this situation, i guess you use a single serial port both as a
> > printk console and as a login tty.
> 
> Yes, I did exactly that.

Well, I just succeeded with experiments.
Changing
if (console_suspend_enabled || !uart_console(uport)) {
in resume process just above the line
/* Protected by port mutex for now */
to
if (1) {
makes resume working with no_console_suspend again. So one of lines
inside does something important.

Even then, the log is incomplete. dmesg shows more, but even it lacks
printk output from inside of if (1).

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12           tel: +420 284 028 966, +49 911 740538747
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Jason Wang]

Stanislav Brabec wrote:
> wanghui wrote:   
>   
>> For this situation, i guess you use a single serial port both as a
>> printk console and as a login tty.
>>     
>
> Yes, I did exactly that.
>
>   
>>  If you use that serial port only
>> as a printk console, and use another serial port or (keyboard + lcd)
>> as login tty, you will see the the printk console serial port can't
>> work after resume.
>>     
>
> Well, I never tried this with my old patch, and I even forgot to think
> about this situation. It complicates the suspend logic even more and I
> understand that 4547be7 breaks it.
>
>   
>> as a result, in the uart_resume_port()
>>     if (port->flags & ASYNC_SUSPENDED) {
>>   ...
>>     }
>> will not be executed.
>>
>> In the resume process, the only serial driver relating sub-callings is
>> uport->ops->set_termios(), but the parameter passed in is not initialized,
>> we can imagine this serial port will not work anymore after this resume.
>>     
>
> Well, if you look at 4547be7 deleted chunk "/* no need to resume serial
> console, it wasn't suspended */", then it may contain relevant code.
> It was called with no_console_suspend before and returned. But "it
> wasn't suspended" is not true: The hardware went to sleep and now is in
> undefined state and this chunk failed to re-initialize it. That is why I
> decided to never enter here and carefully pick parts of the whole resume
> process that need to be executed. I forgot to think about "just debug
> console, not a tty".
>
>   
>> My 2rd patch is for this issue.
>>     
>>> - with no_console_suspend: does not work both with and without your
>>>   patches
>>>       
>
>   
>> In this situation, the uart_suspend_port() will not call any serial driver
>> relating sub-callings except ops->tx_empty().
>>     
>
> And setting "uport->suspended = 1". It was my intention - the console
> should stay alive as long as possible.
>
>   
clear.
>> In the resume process, if apply my 1st patch, the uart_resume_port()
>> will not call any serial driver relating sub-callings just like the suspend
>> process does, i don't know why your serial can't work.
>>     
>
> Me too, but the spitz hardware required setup, otherwise it remains in
> undefined state after the resume. It worked in time of 4547be7, but not
> now. Reviewing patches that affect serial_core, I don't see anything
> obvious.
>
>   
The problem is here,  the uart on my platforms can keep its settings
during the suspend if i don't call ops->stop_r/tx(), but your uart can't.
Maybe your suspend level is deeper than mine. So no matter if calling
ops->stop_r/tx() during suspend, your uart must be re-initialized through
ops->set_termios().

>> If without my 1st patch, the uart_resume_port() will call 
>> uport->ops->set_termios(),
>> but the parameter passed in is not initialized, moreover, it will call 
>> console_start(),
>> this calling is not balance with suspend process because we don't call 
>> console_stop()
>> in the suspend process.
>>     
>
> Does it mean any problem? If I remember correctly, there were two lines
> in suspend that stopped late pre-suspend console messages. I tried to
> skip them, but still wanted to call their resume counterpart (without
> them, the hardware was not re-initialized correctly). Maybe there is a
> better way to do it.
>
>   
>>> So your patches surely don't mean a regression here.
>>>       
>> Not a 100% regression.
>>     
>
> No regression at all on spitz and fix on your omap.
>
> Well, thinking about correct implementation, we need:
>
> suspend:
> - save hardware state
>   
How about move this part to driver specific level(i.e. pxa.c)
> - but not physically stop it
>   
Maybe stop it can save power on some platforms, moreover,
it can prevent some unreadable chars because of the change of
Baud rate clock during suspend.
> - tell kernel that the port is suspended
> - but prevent postponing of late pre-suspend messages
>
> resume:
> - If the port is in any use (console, login or communication) then never
>   skip the hardware resume
> - tell kernel that the port is in fully working state
>
>   

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Jason Wang]

Stanislav Brabec wrote:
> Stanislav Brabec wrote:
>   
>> wanghui wrote:   
>>     
>>> For this situation, i guess you use a single serial port both as a
>>> printk console and as a login tty.
>>>       
>> Yes, I did exactly that.
>>     
>
> Well, I just succeeded with experiments.
> Changing
> if (console_suspend_enabled || !uart_console(uport)) {
> in resume process just above the line
> /* Protected by port mutex for now */
> to
> if (1) {
> makes resume working with no_console_suspend again. So one of lines
> inside does something important.
>
> Even then, the log is incomplete. dmesg shows more, but even it lacks
> printk output from inside of if (1).
>
>   
Hah, :), i guess the ops->set_termios() resume your uart in the end.
Why lost some logs, i will try to reproduce your test.

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

Jason Wang wrote:

> > suspend:
> > - save hardware state
> >   
> How about move this part to driver specific level(i.e. pxa.c)
> > - but not physically stop it

Yes, I was thinking about it as well. It would be far the cleanest
solution that would prevent all ugly hacks in serial_core.c.

In addition to the standard suspend and resume there could be save_state
(and keep running) and resume_state (from save_state mode).

> Maybe stop it can save power on some platforms, moreover,
> it can prevent some unreadable chars

No, no_console_suspend is a special mode, required for suspend debugging
and not intended for production kernels. If you are debugging a kernel
that does not resume at all, you have no other way to collect debug
messages than keeping console in running state as long as possible.

This behavior of no_console_suspend should be kept even at cost of
higher power consumption during suspend in this mode, otherwise some
suspend failures may become undebugable.

I see just another "by design" problem: no_console_suspend keeps console
in running state. After the resume, kernel still thinks, that the
console is not suspended and it is possible to write to the console.
That is why early resume messages may be lost.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12           tel: +420 284 028 966, +49 911 740538747
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Stanislav Brabec]

Jason Wang wrote:

> Hah, :), i guess the ops->set_termios() resume your uart in the end.
> Why lost some logs, i will try to reproduce your test.

I'll try and ask you to test verify that my new patch does not break
your platform.

Anyway, now I know, what I did bad. Your patch makes things better on
some platforms and does not break others.

Acked-by: Stanislav Brabec <sbrabec@suse.cz>

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12           tel: +420 284 028 966, +49 911 740538747
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND 0/2] two serial_core suspend/resume fixes

[Jason Wang]

Stanislav Brabec wrote:
> Jason Wang wrote:
>
>   
>> Hah, :), i guess the ops->set_termios() resume your uart in the end.
>> Why lost some logs, i will try to reproduce your test.
>>     
>
> I'll try and ask you to test verify that my new patch does not break
> your platform.
>
>   
Sure, i look forward to your new patch.
> Anyway, now I know, what I did bad. Your patch makes things better on
> some platforms and does not break others.
>
> Acked-by: Stanislav Brabec <sbrabec@suse.cz>
>
>   
Thanks.