console, serial8250: Disable PM and DMA ops

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 <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/tty/serial/8250/8250_port.c +0 -0

index db97222a1bf4..9a4696ea728b 100644
@@ -2371,7 +2371,10 @@ int serial8250_do_startup(struct uart_port *port)
 	 * Request DMA channels for both RX and TX.

Signature · 1 line

2.17.0

On 2018-05-15 21:34:09 [+0300], Andy Shevchenko wrote:

Quoted 3 lines

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.

Quoted 22 lines

Disable any kind of DMA operations in such case.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/tty/serial/8250/8250_port.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/tty/serial/8250/8250_port.c +4 -1

index db97222a1bf4..9a4696ea728b 100644
@@ -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);

Quoted 4 lines

drivers/tty/serial/8250/8250_port.c +0 -0

 		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 :)

Quoted 3 lines

Signature · 2 lines

-- 
2.17.0

Sebastian

Quoted 5 lines

Quoted 3 lines

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.

Quoted 7 lines

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_.

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(-)

Quoted 7 lines · 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.

Quoted 4 lines

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.

Quoted 13 lines · Andy Shevchenko

Quoted 11 lines · 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.

Quoted 3 lines

+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.

Quoted 9 lines

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.

From reading the description of #2, my understanding is that you are

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

Quoted 2 lines

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.

Quoted 14 lines · Andy Shevchenko

Quoted 5 lines

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

Quoted 8 lines

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.

Quoted 5 lines

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?

Quoted 6 lines

Quoted 4 lines

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

Quoted 3 lines · Sebastian Andrzej Siewior

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.

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Sebastian Andrzej Siewior

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?

Quoted 2 lines

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.

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Sebastian Andrzej Siewior

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?

Quoted 6 lines

Quoted 2 lines

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.

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Sebastian Andrzej Siewior

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 4 lines · Tony Lindgren

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines · Andy Shevchenko

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.

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Tony Lindgren

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines

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? :)

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines · Tony Lindgren

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

I'll see what we can do here.

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Sebastian Andrzej Siewior

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?

Quoted 2 lines

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.

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 15 lines · Sebastian Andrzej Siewior

Quoted 13 lines · Andy Shevchenko

Quoted 9 lines

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.

Quoted 5 lines

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.

Quoted 22 lines · Tony Lindgren

Quoted 20 lines · Sebastian Andrzej Siewior

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines · Andy Shevchenko

Quoted 4 lines · Tony Lindgren

* Sebastian Andrzej Siewior <[email protected]> [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.

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.

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Andy Shevchenko

Quoted 4 lines · Tony Lindgren

* Sebastian Andrzej Siewior <[email protected]> [180516 10:49]:

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 18 lines · Andy Shevchenko

Quoted 15 lines · Tony Lindgren

Quoted 10 lines · Andy Shevchenko

On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <[email protected]

Quoted 6 lines

wrote:
* Sebastian Andrzej Siewior <[email protected]> [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.

Quoted 2 lines

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?

Quoted 2 lines · Tony Lindgren

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?).