[PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

Kernel console is sensitive to any kind of complex work needed to print
out anything on it. One such case is emergency print during Oops.

This series proposes to disable runtime PM and DMA operations on 8250
serial console.

More detailed explanation why is provided in patch 2.

The series has been in our internal trees for years already with no
problems observed.

Andy Shevchenko (3):
  console: introduce ->exit() callback
  serial: 8250_port: Don't use power management for kernel console
  serial: 8250_port: Disable DMA operations for kernel console

 drivers/tty/serial/8250/8250_core.c |  9 +++++++++
 drivers/tty/serial/8250/8250_port.c | 27 ++++++++++++++++++++++-----
 include/linux/console.h             |  1 +
 include/linux/serial_8250.h         |  1 +
 kernel/printk/printk.c              |  3 +++
 5 files changed, 36 insertions(+), 5 deletions(-)

-- 
2.17.0

[PATCH v1 1/3] console: introduce ->exit() callback

[Andy Shevchenko]

Some consoles might require special operations on unregistering. For example,
serial console, when registered in the kernel, keeps power on for entire time,
until it gets unregistered. For such cases to have a balance we would provide
->exit() callback.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/console.h | 1 +
 kernel/printk/printk.c  | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/console.h b/include/linux/console.h
index dfd6b0e97855..62c5b958b548 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -147,6 +147,7 @@ struct console {
 	struct tty_driver *(*device)(struct console *, int *);
 	void	(*unblank)(void);
 	int	(*setup)(struct console *, char *);
+	void	(*exit)(struct console *);
 	int	(*match)(struct console *, char *name, int idx, char *options);
 	short	flags;
 	short	index;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 247808333ba4..a3170886f481 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2756,6 +2756,9 @@ int unregister_console(struct console *console)
 	if (console_drivers != NULL && console->flags & CON_CONSDEV)
 		console_drivers->flags |= CON_CONSDEV;
 
+	if (console->exit)
+		console->exit(console);
+
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
 	console_sysfs_notify();
-- 
2.17.0

[PATCH v1 2/3] serial: 8250_port: Don't use power management for kernel console

[Andy Shevchenko]

Doing any kind of power management for kernel console is really bad idea.

First of all, it runs in poll and atomic mode. This fact attaches a limitation
on the functions that might be called. For example, pm_runtime_get_sync() might
sleep and thus can't be used. On the other hand pm_runtime_get() doesn't
guarantee that device becomes powered on (fast enough). On ACPI enabled
platforms it might even call firmware for a job.

Besides that, imagine the case when console is about to print a kernel oops and
it's powered off. In such an emergency case calling the complex functions is
not the best what we can do, taking into consideration that user wants to see
at least something of the last kernel word before it passes away.

Here we modify the 8250 console code to prevent runtime power management.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_core.c |  9 +++++++++
 drivers/tty/serial/8250/8250_port.c | 22 ++++++++++++++++++----
 include/linux/serial_8250.h         |  1 +
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..cdafee307482 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -612,6 +612,14 @@ static int univ8250_console_setup(struct console *co, char *options)
 	return retval;
 }
 
+static void univ8250_console_exit(struct console *co)
+{
+	struct uart_port *port;
+
+	port = &serial8250_ports[co->index].port;
+	serial8250_console_exit(port);
+}
+
 /**
  *	univ8250_console_match - non-standard console matching
  *	@co:	  registering console
@@ -670,6 +678,7 @@ static struct console univ8250_console = {
 	.write		= univ8250_console_write,
 	.device		= uart_console_device,
 	.setup		= univ8250_console_setup,
+	.exit		= univ8250_console_exit,
 	.match		= univ8250_console_match,
 	.flags		= CON_PRINTBUFFER | CON_ANYTIME,
 	.index		= -1,
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 2ca7df29dc11..db97222a1bf4 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3207,6 +3207,9 @@ static void serial8250_console_restore(struct uart_8250_port *up)
  *	any possible real use of the port...
  *
  *	The console_lock must be held when we get here.
+ *
+ *	Doing runtime PM is a really bad idea for the kernel console.
+ *	Thus we assume that the function called when device is powered on.
  */
 void serial8250_console_write(struct uart_8250_port *up, const char *s,
 			      unsigned int count)
@@ -3218,8 +3221,6 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	touch_nmi_watchdog();
 
-	serial8250_rpm_get(up);
-
 	if (port->sysrq)
 		locked = 0;
 	else if (oops_in_progress)
@@ -3264,7 +3265,6 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
 
 	if (locked)
 		spin_unlock_irqrestore(&port->lock, flags);
-	serial8250_rpm_put(up);
 }
 
 static unsigned int probe_baud(struct uart_port *port)
@@ -3288,6 +3288,7 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
+	int ret;
 
 	if (!port->iobase && !port->membase)
 		return -ENODEV;
@@ -3297,7 +3298,20 @@ int serial8250_console_setup(struct uart_port *port, char *options, bool probe)
 	else if (probe)
 		baud = probe_baud(port);
 
-	return uart_set_options(port, port->cons, baud, parity, bits, flow);
+	ret = uart_set_options(port, port->cons, baud, parity, bits, flow);
+	if (ret)
+		return ret;
+
+	if (port->dev)
+		pm_runtime_get_noresume(port->dev);
+
+	return 0;
+}
+
+void serial8250_console_exit(struct uart_port *port)
+{
+	if (port->dev)
+		pm_runtime_put_noidle(port->dev);
 }
 
 #endif /* CONFIG_SERIAL_8250_CONSOLE */
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 76b9db71e489..f83309328b66 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -171,6 +171,7 @@ void serial8250_set_defaults(struct uart_8250_port *up);
 void serial8250_console_write(struct uart_8250_port *up, const char *s,
 			      unsigned int count);
 int serial8250_console_setup(struct uart_port *port, char *options, bool probe);
+void serial8250_console_exit(struct uart_port *port);
 
 extern void serial8250_set_isa_configurator(void (*v)
 					(int port, struct uart_port *up,
-- 
2.17.0

[PATCH v1 3/3] serial: 8250_port: Disable DMA operations for kernel console

[Andy Shevchenko]

It will be too tricky and error prone to allow DMA operations on kernel
console.

Disable any kind of DMA operations in such case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index db97222a1bf4..9a4696ea728b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2371,7 +2371,10 @@ int serial8250_do_startup(struct uart_port *port)
 	 * Request DMA channels for both RX and TX.
 	 */
 	if (up->dma) {
-		retval = serial8250_request_dma(up);
+		if (uart_console(port))
+			retval = -ENXIO;
+		else
+			retval = serial8250_request_dma(up);
 		if (retval)
 			pr_warn_ratelimited("ttyS%d - failed to request DMA\n",
 					    serial_index(port));
-- 
2.17.0

Re: [PATCH v1 3/3] serial: 8250_port: Disable DMA operations for kernel console

[Sebastian Andrzej Siewior]

On 2018-05-15 21:34:09 [+0300], Andy Shevchenko wrote:
> It will be too tricky and error prone to allow DMA operations on kernel
> console.

Why is it tricky and error prone? I had it working…
But I don't mind dropping the DMA on the kernel console because I doubt
that we lose something here by disabling it. I would even imagine that
it gets "simpler" (maybe what you tried to say by "error prone") to
print something in the NMI case by writing directly to the FIFO register
instead setting up a DMA transfer and so on.

> Disable any kind of DMA operations in such case.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index db97222a1bf4..9a4696ea728b 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2371,7 +2371,10 @@ int serial8250_do_startup(struct uart_port *port)
>  	 * Request DMA channels for both RX and TX.
>  	 */
>  	if (up->dma) {
> -		retval = serial8250_request_dma(up);
> +		if (uart_console(port))
> +			retval = -ENXIO;
> +		else
> +			retval = serial8250_request_dma(up);
>  		if (retval)
>  			pr_warn_ratelimited("ttyS%d - failed to request DMA\n",
>  					    serial_index(port));

I would appreciate if you note that it is disabled on purpose and not
because something failed.
Other than those minor things I an board :)

> -- 
> 2.17.0

Sebastian

Re: [PATCH v1 3/3] serial: 8250_port: Disable DMA operations for kernel console

[Andy Shevchenko]

On Wed, 2018-05-16 at 12:55 +0200, Sebastian Andrzej Siewior wrote:
> On 2018-05-15 21:34:09 [+0300], Andy Shevchenko wrote:

Thanks for review, my answers below.

> > It will be too tricky and error prone to allow DMA operations on
> > kernel
> > console.
> 
> Why is it tricky and error prone? I had it working…

On OMAP only? Had you tested this on let's say Intel Cherrytrail where
DMA controller is a separate PCI device which needs to be handled
separately from UART IP.

> But I don't mind dropping the DMA on the kernel console because I
> doubt
> that we lose something here by disabling it. I would even imagine that
> it gets "simpler" (maybe what you tried to say by "error prone") to
> print something in the NMI case by writing directly to the FIFO
> register
> instead setting up a DMA transfer and so on.

"error prone" mostly refers to patch 2 commit message. Here it seems I
need to put something like above to explain why DMA case tricky _as
well_.

> > Disable any kind of DMA operations in such case.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/tty/serial/8250/8250_port.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_port.c
> > b/drivers/tty/serial/8250/8250_port.c
> > index db97222a1bf4..9a4696ea728b 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -2371,7 +2371,10 @@ int serial8250_do_startup(struct uart_port
> > *port)
> >  	 * Request DMA channels for both RX and TX.
> >  	 */
> >  	if (up->dma) {
> > -		retval = serial8250_request_dma(up);
> > +		if (uart_console(port))
> > +			retval = -ENXIO;
> > +		else
> > +			retval = serial8250_request_dma(up);
> >  		if (retval)
> >  			pr_warn_ratelimited("ttyS%d - failed to
> > request DMA\n",
> >  					    serial_index(port));
> 
> I would appreciate if you note that it is disabled on purpose and not
> because something failed.
> Other than those minor things I an board :)

Got it for v2.

> 
> > -- 
> > 2.17.0
> 
> Sebastian

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 3/3] serial: 8250_port: Disable DMA operations for kernel console

[Sebastian Andrzej Siewior]

On 2018-05-16 15:58:10 [+0300], Andy Shevchenko wrote:
> > > It will be too tricky and error prone to allow DMA operations on
> > > kernel
> > > console.
> > 
> > Why is it tricky and error prone? I had it working…
> 
> On OMAP only? Had you tested this on let's say Intel Cherrytrail where
> DMA controller is a separate PCI device which needs to be handled
> separately from UART IP.

My point was simply to clarify if this entirely broken and requires
backports or if this makes things more complicated and could be avoided.

> > But I don't mind dropping the DMA on the kernel console because I
> > doubt
> > that we lose something here by disabling it. I would even imagine that
> > it gets "simpler" (maybe what you tried to say by "error prone") to
> > print something in the NMI case by writing directly to the FIFO
> > register
> > instead setting up a DMA transfer and so on.
> 
> "error prone" mostly refers to patch 2 commit message. Here it seems I
> need to put something like above to explain why DMA case tricky _as
> well_.

Yes, full context is helpful :)

Sebastian

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Tue, 2018-05-15 at 21:34 +0300, Andy Shevchenko wrote:
> Kernel console is sensitive to any kind of complex work needed to
> print
> out anything on it. One such case is emergency print during Oops.
> 
> This series proposes to disable runtime PM and DMA operations on 8250
> serial console.
> 
> More detailed explanation why is provided in patch 2.
> 
> The series has been in our internal trees for years already with no
> problems observed.

+Cc: Tony.

You might have some thoughts / test means for this.

> 
> Andy Shevchenko (3):
>   console: introduce ->exit() callback
>   serial: 8250_port: Don't use power management for kernel console
>   serial: 8250_port: Disable DMA operations for kernel console
> 
>  drivers/tty/serial/8250/8250_core.c |  9 +++++++++
>  drivers/tty/serial/8250/8250_port.c | 27 ++++++++++++++++++++++-----
>  include/linux/console.h             |  1 +
>  include/linux/serial_8250.h         |  1 +
>  kernel/printk/printk.c              |  3 +++
>  5 files changed, 36 insertions(+), 5 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Sebastian Andrzej Siewior]

On 2018-05-15 21:37:18 [+0300], Andy Shevchenko wrote:
> On Tue, 2018-05-15 at 21:34 +0300, Andy Shevchenko wrote:
> > Kernel console is sensitive to any kind of complex work needed to
> > print
> > out anything on it. One such case is emergency print during Oops.
> > 
> > This series proposes to disable runtime PM and DMA operations on 8250
> > serial console.
> > 
> > More detailed explanation why is provided in patch 2.
> > 
> > The series has been in our internal trees for years already with no
> > problems observed.
> 
> +Cc: Tony.
> 
> You might have some thoughts / test means for this.

I haven't look at the patches, just your cover letter. Disabling DMA on
kernel-console should be fine. The output is usually short so there
shouldn't be much benefit from using it.
I remember Tony wanted runtime-pm on the kernel console, too. And he
told me explicit how to test it so that it works. Once the UART goes
into PM (down), the whole IP block can go into power save mode. The
board can be woken up by sending a character via the UART. The first few
(incoming / read) characters are lost until the IP block is up again the
frequency stable. This is known / expected.
In order to achieve the same thing you would have to disable the kernel
console on that UART. I leave this to Tony.

Sebastian

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Wed, May 16, 2018 at 1:08 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-05-15 21:37:18 [+0300], Andy Shevchenko wrote:
>> On Tue, 2018-05-15 at 21:34 +0300, Andy Shevchenko wrote:
>> > Kernel console is sensitive to any kind of complex work needed to
>> > print
>> > out anything on it. One such case is emergency print during Oops.
>> >
>> > This series proposes to disable runtime PM and DMA operations on 8250
>> > serial console.
>> >
>> > More detailed explanation why is provided in patch 2.
>> >
>> > The series has been in our internal trees for years already with no
>> > problems observed.
>>
>> +Cc: Tony.
>>
>> You might have some thoughts / test means for this.
>
> I haven't look at the patches, just your cover letter. Disabling DMA on
> kernel-console should be fine.

Would you mind to give an Ack for it?

> The output is usually short so there
> shouldn't be much benefit from using it.

> I remember Tony wanted runtime-pm on the kernel console, too. And he
> told me explicit how to test it so that it works. Once the UART goes
> into PM (down), the whole IP block can go into power save mode. The
> board can be woken up by sending a character via the UART. The first few
> (incoming / read) characters are lost until the IP block is up again the
> frequency stable. This is known / expected.

Don't consider world the OMAP only. The things more complicated if we
go out of it. Which I tried to explain in the commit message of patch
2.

> In order to achieve the same thing you would have to disable the kernel
> console on that UART. I leave this to Tony.

Precisely the point of the series.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Sebastian Andrzej Siewior]

On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> > The output is usually short so there
> > shouldn't be much benefit from using it.
> 
> > I remember Tony wanted runtime-pm on the kernel console, too. And he
> > told me explicit how to test it so that it works. Once the UART goes
> > into PM (down), the whole IP block can go into power save mode. The
> > board can be woken up by sending a character via the UART. The first few
> > (incoming / read) characters are lost until the IP block is up again the
> > frequency stable. This is known / expected.
> 
> Don't consider world the OMAP only. The things more complicated if we
> go out of it. Which I tried to explain in the commit message of patch
> 2.

I am not saying the world is OMAP only. I just tried to explain how any
why it got there and its purpose. I haven't NACKed it. I would believe
that with that information and Tony not defending his use case or
possible change it somehow so it is not standing in your way, Greg would
have enough information to go your way.
But since I am on it. You have to enable runtime-PM for the UART. So
what is the problem if you simply don't enable it for the UART which
used as the kernel console?
>From reading the description of #2, my understanding is that you are
afraid that enabling the UART (bringing it from power-down mode) might
take too long because it requires (or might require) an ACPI function
call. This brings me again to: why bother and enable it in the first
place? Because this might not work from NMI context.

>From looking at #2. You remove put/get in console_write() and avoid
runtime PM by pm_runtime_get_noresume() and need the extra hook on
console exit. I wasn't aware that you can remove the console at runtime
(and assumed you have to stick to what said in console= at bootime).
Because if that is the case, you could simply strip UART_CAP_RPM and
then there is no more runtime-PM for you. Or is it too simple and I
miss something here?

> > In order to achieve the same thing you would have to disable the kernel
> > console on that UART. I leave this to Tony.
> 
> Precisely the point of the series.

Sebastian

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> > > The output is usually short so there
> > > shouldn't be much benefit from using it.
> > > I remember Tony wanted runtime-pm on the kernel console, too. And
> > > he
> > > told me explicit how to test it so that it works. Once the UART
> > > goes
> > > into PM (down), the whole IP block can go into power save mode.
> > > The
> > > board can be woken up by sending a character via the UART. The
> > > first few
> > > (incoming / read) characters are lost until the IP block is up
> > > again the
> > > frequency stable. This is known / expected.
> > 
> > Don't consider world the OMAP only. The things more complicated if
> > we
> > go out of it. Which I tried to explain in the commit message of
> > patch
> > 2.
> 
> I am not saying the world is OMAP only. I just tried to explain how
> any
> why it got there and its purpose. I haven't NACKed it. I would believe
> that with that information and Tony not defending his use case or
> possible change it somehow so it is not standing in your way, Greg
> would
> have enough information to go your way.

Yes, I got it.

> But since I am on it. You have to enable runtime-PM for the UART. So
> what is the problem if you simply don't enable it for the UART which
> used as the kernel console?

How do I know at the ->probe() time that device in question is going to
be kernel console? Maybe I missed simple way of it.

> From reading the description of #2, my understanding is that you are
> afraid that enabling the UART (bringing it from power-down mode) might
> take too long because it requires (or might require) an ACPI function
> call. This brings me again to: why bother and enable it in the first
> place? Because this might not work from NMI context.

> From looking at #2. You remove put/get in console_write()

...which must be removed in any case...

>  and avoid
> runtime PM by pm_runtime_get_noresume() and need the extra hook on
> console exit. I wasn't aware that you can remove the console at
> runtime
> (and assumed you have to stick to what said in console= at bootime).

> Because if that is the case, you could simply strip UART_CAP_RPM and
> then there is no more runtime-PM for you. Or is it too simple and I
> miss something here?

See above question. IIRC it's either too early or too late to decide on
per port basis.

> 
> > > In order to achieve the same thing you would have to disable the
> > > kernel
> > > console on that UART. I leave this to Tony.
> > 
> > Precisely the point of the series.

Besides above this would be needed _at least_ for several 8250 glue
drivers, so, I decide to go once for all.

> 
> Sebastian

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> > But since I am on it. You have to enable runtime-PM for the UART. So
> > what is the problem if you simply don't enable it for the UART which
> > used as the kernel console?
> 
> How do I know at the ->probe() time that device in question is going to
> be kernel console? Maybe I missed simple way of it.

Hmm parse the kernel cmdline maybe? :)

BTW, kernel already has earlycon doing exactly what you're trying to do.

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
>> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
>> > But since I am on it. You have to enable runtime-PM for the UART. So
>> > what is the problem if you simply don't enable it for the UART which
>> > used as the kernel console?
>>
>> How do I know at the ->probe() time that device in question is going to
>> be kernel console? Maybe I missed simple way of it.
>
> Hmm parse the kernel cmdline maybe? :)
>
> BTW, kernel already has earlycon doing exactly what you're trying to do.

I'm sorry, I didn't follow. What exactly earlycon does?

The problem is in 8250 driver. The issue with runtime PM used in atomic context.
So, I can, of course just remove callbacks from the console ->write().
Though it will prevent to use kernel console anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:41]:
> On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> >> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> >> > But since I am on it. You have to enable runtime-PM for the UART. So
> >> > what is the problem if you simply don't enable it for the UART which
> >> > used as the kernel console?
> >>
> >> How do I know at the ->probe() time that device in question is going to
> >> be kernel console? Maybe I missed simple way of it.
> >
> > Hmm parse the kernel cmdline maybe? :)
> >
> > BTW, kernel already has earlycon doing exactly what you're trying to do.
> 
> I'm sorry, I didn't follow. What exactly earlycon does?

It provides a console very early on, see earlycon in
Documentation/admin-guide/kernel-parameters.txt

> The problem is in 8250 driver. The issue with runtime PM used in atomic context.

So how about add some "noidle" kernel command line parameter for console
that calls pm_runtime_forbid() and then you have the UART permanently
on. Hmm I guess you could make also serial8250_rpm_get() do nothing
based on that.

I do agree the serial runtime PM has an issue if it depends on
pm_runtime_irq_safe() being set.

> So, I can, of course just remove callbacks from the console ->write().
> Though it will prevent to use kernel console anyway.

Please et's not start breaking things, we already see a constant
flow of regressions on weekly basis.

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:41]:
>> On Thu, May 17, 2018 at 4:56 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
>> >> On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
>> >> > But since I am on it. You have to enable runtime-PM for the UART. So
>> >> > what is the problem if you simply don't enable it for the UART which
>> >> > used as the kernel console?
>> >>
>> >> How do I know at the ->probe() time that device in question is going to
>> >> be kernel console? Maybe I missed simple way of it.
>> >
>> > Hmm parse the kernel cmdline maybe? :)
>> >
>> > BTW, kernel already has earlycon doing exactly what you're trying to do.
>>
>> I'm sorry, I didn't follow. What exactly earlycon does?
>
> It provides a console very early on, see earlycon in
> Documentation/admin-guide/kernel-parameters.txt

Yes, but what we are talking is to put the kernel console out of power
management due to _real_ issues with it.

>> The problem is in 8250 driver. The issue with runtime PM used in atomic context.
>
> So how about add some "noidle" kernel command line parameter for console
> that calls
> pm_runtime_forbid() and then you have the UART permanently
> on.

IIUC _forbid() can be overwritten via sysfs.
And I would prefer to do other way around, something like console.idle
and put default for OMAP to yes and no for everything else.

> Hmm I guess you could make also serial8250_rpm_get() do nothing
> based on that.

Have you seen entire series which I keep here:
https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
Among other things it gets rid of those specific callbacks entirely.

> I do agree the serial runtime PM has an issue if it depends on
> pm_runtime_irq_safe() being set.

It's more than an issue. The  so called "support" of RPM for UART is
_based on the hack_.
I would love to NAK that in the first place if I would have known of it in time.

>> So, I can, of course just remove callbacks from the console ->write().
>> Though it will prevent to use kernel console anyway.
>
> Please et's not start breaking things, we already see a constant
> flow of regressions on weekly basis.

Now we are stick with a hack and the case based on that is against
fixing things.
This is how it looks from my side.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com> wrote:
> > So how about add some "noidle" kernel command line parameter for console
> > that calls
> > pm_runtime_forbid() and then you have the UART permanently
> > on.
> 
> IIUC _forbid() can be overwritten via sysfs.
> And I would prefer to do other way around, something like console.idle
> and put default for OMAP to yes and no for everything else.

OK yeah console.idle sounds good to me. We should default to a
safe option.

> > Hmm I guess you could make also serial8250_rpm_get() do nothing
> > based on that.
> 
> Have you seen entire series which I keep here:
> https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> Among other things it gets rid of those specific callbacks entirely.

Well I was not Cc:ed on it, I browsed it in some archive and it
seemed unsafe to me. But if you figured out a way to do it conditionally
based on console.idle without causing regressions.

> > I do agree the serial runtime PM has an issue if it depends on
> > pm_runtime_irq_safe() being set.
> 
> It's more than an issue. The  so called "support" of RPM for UART is
> _based on the hack_.
> I would love to NAK that in the first place if I would have known of it in time.

Hmm well it seems that you too have been patching the 8250_rpm
functions for years and then now what after multiple years you
hit this issue? :)

> >> So, I can, of course just remove callbacks from the console ->write().
> >> Though it will prevent to use kernel console anyway.
> >
> > Please et's not start breaking things, we already see a constant
> > flow of regressions on weekly basis.
> 
> Now we are stick with a hack and the case based on that is against
> fixing things.
> This is how it looks from my side.

Sorry yeah I agree there are issues, but let's fix it properly with
no regressions.

Regards,

TOny

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180523 10:58]:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> > Have you seen entire series which I keep here:
> > https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> > Among other things it gets rid of those specific callbacks entirely.
> 
> Well I was not Cc:ed on it, I browsed it in some archive and it
> seemed unsafe to me. But if you figured out a way to do it conditionally
> based on console.idle without causing regressions.

Sorry incomplete sentence here.. If you have some branch that
based on console.idle I'd be happy to test it.

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Wed, 2018-05-23 at 10:58 -0700, Tony Lindgren wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> > On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com>
> > wrote:
> > > So how about add some "noidle" kernel command line parameter for
> > > console
> > > that calls
> > > pm_runtime_forbid() and then you have the UART permanently
> > > on.
> > 
> > IIUC _forbid() can be overwritten via sysfs.
> > And I would prefer to do other way around, something like
> > console.idle
> > and put default for OMAP to yes and no for everything else.
> 
> OK yeah console.idle sounds good to me. We should default to a
> safe option.

I'll see what we can do here.

> > > Hmm I guess you could make also serial8250_rpm_get() do nothing
> > > based on that.
> > 
> > Have you seen entire series which I keep here:
> > https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> > Among other things it gets rid of those specific callbacks entirely.
> 
> Well I was not Cc:ed on it, I browsed it in some archive and it
> seemed unsafe to me. But if you figured out a way to do it
> conditionally
> based on console.idle
>  without causing regressions.

I restored that branch with some updated patches. It's far from done and
doesn't have any new stuff (yet) regrading to this discussion.

> > > I do agree the serial runtime PM has an issue if it depends on
> > > pm_runtime_irq_safe() being set.
> > 
> > It's more than an issue. The  so called "support" of RPM for UART is
> > _based on the hack_.
> > I would love to NAK that in the first place if I would have known of
> > it in time.
> 
> Hmm well it seems that you too have been patching the 8250_rpm
> functions for years and then now what after multiple years you
> hit this issue? :)

Nope, I hit it as soon as I tried.

I can't find easily the discussion (hmm... yes, I was so pissed off that
time, I put a bit of harsh in that) I had with Sebastian few years back,
but at least I reported about an issue.

> > > > So, I can, of course just remove callbacks from the console
> > > > ->write().
> > > > Though it will prevent to use kernel console anyway.
> > > 
> > > Please et's not start breaking things, we already see a constant
> > > flow of regressions on weekly basis.
> > 
> > Now we are stick with a hack and the case based on that is against
> > fixing things.
> > This is how it looks from my side.
> 
> Sorry yeah I agree there are issues, but let's fix it properly

Agree.

>  with
> no regressions.

...though I think a word "regression" is inappropriate here. Regression
is what the support did in the first place. Pity I didn't know about it
at that time.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180718 15:17]:
> On Wed, 2018-05-23 at 10:58 -0700, Tony Lindgren wrote:
> > 
> > OK yeah console.idle sounds good to me. We should default to a
> > safe option.
> 
> I'll see what we can do here.

Like we discussed offline I think if we allow detaching and attaching
kernel console from userspace we can get rid of irqsafe completely :)
And in that case no need for console.idle or anything like that.

Then if kernel console is attached, we can keep console uart enabled.
If detached, it can do runtime PM if hardware supports it.

Not sure if we need some separate new /sys entry for that, maybe
we already have that with the loglevel in /proc/sys/kernel/printk?

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180517 06:56]:
> * Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180516 13:12]:
> > On Wed, 2018-05-16 at 12:47 +0200, Sebastian Andrzej Siewior wrote:
> > > But since I am on it. You have to enable runtime-PM for the UART. So
> > > what is the problem if you simply don't enable it for the UART which
> > > used as the kernel console?
> > 
> > How do I know at the ->probe() time that device in question is going to
> > be kernel console? Maybe I missed simple way of it.
> 
> Hmm parse the kernel cmdline maybe? :)
> 
> BTW, kernel already has earlycon doing exactly what you're trying to do.

And if earlycon does not do the job, maybe just set up some
kernel command line parameter like "noidle" that makes serial
probe call pm_runtime_forbid().

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> > > The output is usually short so there
> > > shouldn't be much benefit from using it.
> > 
> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
> > > told me explicit how to test it so that it works. Once the UART goes
> > > into PM (down), the whole IP block can go into power save mode. The
> > > board can be woken up by sending a character via the UART. The first few
> > > (incoming / read) characters are lost until the IP block is up again the
> > > frequency stable. This is known / expected.
> > 
> > Don't consider world the OMAP only. The things more complicated if we
> > go out of it. Which I tried to explain in the commit message of patch
> > 2.
> 
> I am not saying the world is OMAP only. I just tried to explain how any
> why it got there and its purpose. I haven't NACKed it. I would believe
> that with that information and Tony not defending his use case or
> possible change it somehow so it is not standing in your way, Greg would
> have enough information to go your way.

The idea breaking PM seems silly to me considering that we've had
it working for years already.

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
>> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
>> > > The output is usually short so there
>> > > shouldn't be much benefit from using it.
>> >
>> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
>> > > told me explicit how to test it so that it works. Once the UART goes
>> > > into PM (down), the whole IP block can go into power save mode. The
>> > > board can be woken up by sending a character via the UART. The first few
>> > > (incoming / read) characters are lost until the IP block is up again the
>> > > frequency stable. This is known / expected.
>> >
>> > Don't consider world the OMAP only. The things more complicated if we
>> > go out of it. Which I tried to explain in the commit message of patch
>> > 2.
>>
>> I am not saying the world is OMAP only. I just tried to explain how any
>> why it got there and its purpose. I haven't NACKed it. I would believe
>> that with that information and Tony not defending his use case or
>> possible change it somehow so it is not standing in your way, Greg would
>> have enough information to go your way.
>
> The idea breaking PM seems silly to me considering that we've had
> it working for years already.

Same question  / note. World is much more complex than just being OMAP.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
> >> On 2018-05-16 13:17:36 [+0300], Andy Shevchenko wrote:
> >> > > The output is usually short so there
> >> > > shouldn't be much benefit from using it.
> >> >
> >> > > I remember Tony wanted runtime-pm on the kernel console, too. And he
> >> > > told me explicit how to test it so that it works. Once the UART goes
> >> > > into PM (down), the whole IP block can go into power save mode. The
> >> > > board can be woken up by sending a character via the UART. The first few
> >> > > (incoming / read) characters are lost until the IP block is up again the
> >> > > frequency stable. This is known / expected.
> >> >
> >> > Don't consider world the OMAP only. The things more complicated if we
> >> > go out of it. Which I tried to explain in the commit message of patch
> >> > 2.
> >>
> >> I am not saying the world is OMAP only. I just tried to explain how any
> >> why it got there and its purpose. I haven't NACKed it. I would believe
> >> that with that information and Tony not defending his use case or
> >> possible change it somehow so it is not standing in your way, Greg would
> >> have enough information to go your way.
> >
> > The idea breaking PM seems silly to me considering that we've had
> > it working for years already.
> 
> Same question  / note. World is much more complex than just being OMAP.

Sorry but you are making assumptions about hardware being powered on
all the time.

It may have been a safe assumption up to mid-90's and might still be
valid assumption for hardware providing on MS-DOS boot floppy
compability.

But that is not a safe assumption for Linux kernel at all. Especially
for console TX where there's no need to keep it powered unless there
is actually something to write.

If there are runtime PM issues for consoles, let's just fix them.

Then for really seeing console messages on next reboot from a hung
system, CONFIG_PSTORE_CONSOLE option seems to do a very good job.
With a warm reset after triggered by watchdog the console messages
are readable most of the time even when using DDR as the back end.

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
>> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:

>> > The idea breaking PM seems silly to me considering that we've had
>> > it working for years already.
>>
>> Same question  / note. World is much more complex than just being OMAP.
>
> Sorry but you are making assumptions about hardware being powered on
> all the time.

Nope, other way around. The so called "support" _prevents_ our
hardware to go to sleep.

So it's not breaking, it's fixing. It's pity that OMAP solution is just a hack.

> It may have been a safe assumption up to mid-90's and might still be
> valid assumption for hardware providing on MS-DOS boot floppy
> compability.
>
> But that is not a safe assumption for Linux kernel at all. Especially
> for console TX where there's no need to keep it powered unless there
> is actually something to write.
>
> If there are runtime PM issues for consoles, let's just fix them.

Yes, there is an issue to begin with, i.e. irq_safe hack.
Removal of that hack reveals the real issues with the code.

> Then for really seeing console messages on next reboot from a hung
> system, CONFIG_PSTORE_CONSOLE option seems to do a very good job.

How this helps to prevent a more serious system crashes during an
attempt to power UART on?

> With a warm reset after triggered by watchdog the console messages
> are readable most of the time even when using DDR as the back end.

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:54]:
> On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> >> On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516 10:49]:
> 
> >> > The idea breaking PM seems silly to me considering that we've had
> >> > it working for years already.
> >>
> >> Same question  / note. World is much more complex than just being OMAP.
> >
> > Sorry but you are making assumptions about hardware being powered on
> > all the time.
> 
> Nope, other way around. The so called "support" _prevents_ our
> hardware to go to sleep.

Hmm sorry now I'm all confused what issues you're having.

I thought you said earlier the issue was that you wanted to keep
the console enabled all the time and never idle?

Regards,

Tony

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Andy Shevchenko]

On Wed, 2018-05-23 at 11:00 -0700, Tony Lindgren wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:54]:
> > On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com>
> > wrote:
> > > * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> > > > On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com
> > > > > wrote:
> > > > > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516
> > > > > 10:49]:
> > > > > The idea breaking PM seems silly to me considering that we've
> > > > > had
> > > > > it working for years already.
> > > > 
> > > > Same question  / note. World is much more complex than just
> > > > being OMAP.
> > > 
> > > Sorry but you are making assumptions about hardware being powered
> > > on
> > > all the time.
> > 
> > Nope, other way around. The so called "support" _prevents_ our
> > hardware to go to sleep.
> 
> Hmm sorry now I'm all confused what issues you're having.
> 
> I thought you said earlier the issue was that you wanted to keep
> the console enabled all the time and never idle?

Yes, for kernel console.
To be clear, if user supplies "console=ttySx" it keeps powered on
always. But if there is no such parameter, we are fine with RPM.

Letting kernel console do power management on the systems without
irq_safe hack is dangerous in terms of loosing important data (crash, or
some other stuff which needs atomic context: kgdb?).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops

[Tony Lindgren]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180718 14:54]:
> On Wed, 2018-05-23 at 11:00 -0700, Tony Lindgren wrote:
> > I thought you said earlier the issue was that you wanted to keep
> > the console enabled all the time and never idle?
> 
> Yes, for kernel console.
> To be clear, if user supplies "console=ttySx" it keeps powered on
> always. But if there is no such parameter, we are fine with RPM.

I'm fine with that as long as we have a way to detach or disable
kernel console from userspace after booting so the console uart can
idle.

> Letting kernel console do power management on the systems without
> irq_safe hack is dangerous in terms of loosing important data (crash, or
> some other stuff which needs atomic context: kgdb?).

Yes in the long run we should get rid of all pm_runtime_irq_safe()
use. It increases the parent dev PM runtime usage count permanently
blocking the whole domain from idling.

Regards,

Tony