* [PATCH 2/8] drivers: tty: serial: 8250_dw: use devm_ioremap_resource()
From: Enrico Weigelt, metux IT consult @ 2019-03-12 14:57 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel
In-Reply-To: <1552402660-31730-1-git-send-email-info@metux.net>
Use helper devm_ioremap_resource() to make the code a little bit
shorter and easier to read.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
drivers/tty/serial/8250/8250_dw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d31b975..f0a294d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -526,7 +526,7 @@ static int dw8250_probe(struct platform_device *pdev)
p->set_ldisc = dw8250_set_ldisc;
p->set_termios = dw8250_set_termios;
- p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
+ p->membase = devm_ioremap_resource(dev, regs);
if (!p->membase)
return -ENOMEM;
--
1.9.1
^ permalink raw reply related
* [PATCH 1/8] drivers: tty: serial: 8250_bcm2835aux: use devm_platform_ioremap_resource()
From: Enrico Weigelt, metux IT consult @ 2019-03-12 14:57 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel
In-Reply-To: <1552402660-31730-1-git-send-email-info@metux.net>
---
drivers/tty/serial/8250/8250_bcm2835aux.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index bd53661..0738d14 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -25,7 +25,6 @@ struct bcm2835aux_data {
static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
struct bcm2835aux_data *data;
- struct resource *res;
int ret;
/* allocate the custom structure */
@@ -63,15 +62,12 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
data->uart.port.irq = ret;
/* map the main registers */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev, "memory resource not found");
- return -EINVAL;
- }
- data->uart.port.membase = devm_ioremap_resource(&pdev->dev, res);
+ data->uart.port.membase = devm_platform_ioremap_resource(pdev, 0);
ret = PTR_ERR_OR_ZERO(data->uart.port.membase);
- if (ret)
+ if (ret) {
+ dev_err(&pdev->dev, "could not map memory resource");
return ret;
+ }
/* Check for a fixed line number */
ret = of_alias_get_id(pdev->dev.of_node, "serial");
--
1.9.1
^ permalink raw reply related
* RFC: cleaning up the serial drivers and use struct resource
From: Enrico Weigelt, metux IT consult @ 2019-03-12 14:57 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel
Hello folks,
I'm currently working on some cleanups in drivers/tty/serial.
There're several cases where new helpers, like devm_platform_ioremap_resource
can be used, other places can use devm_ioremap_resource() for a bit
cleaner code.
Another topic here is using struct resource, instead of separate fields
(BTW: struct uart_port->mapsize doesn't seem to be used consequently):
in struct uart_port, I'm adding a struct resource field for holding the
port's iomem range, and helpers for it. Then, I'm step by step patching
the individual drivers to use that, instead of the old fields, directly.
For now, this all is early, untested and not yet meant for mainline.
I'd just like to have your oppions on my approach.
What do you think about it ?
thx
--mtx
^ permalink raw reply
* Re: [PATCH v8 1/2] dt-bindings: serial: Add compatible for Mediatek MT8183
From: Matthias Brugger @ 2019-03-12 14:41 UTC (permalink / raw)
To: Erin Lo, Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, mars.cheng,
eddie.huang, linux-clk, zhiyong.tao, Mengqi.Zhang
In-Reply-To: <1552294472-32929-2-git-send-email-erin.lo@mediatek.com>
On 11/03/2019 09:54, Erin Lo wrote:
> This adds dt-binding documentation of uart for Mediatek MT8183 SoC
> Platform.
>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/serial/mtk-uart.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt
> index 742cb47..bcfb131 100644
> --- a/Documentation/devicetree/bindings/serial/mtk-uart.txt
> +++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt
> @@ -16,6 +16,7 @@ Required properties:
> * "mediatek,mt8127-uart" for MT8127 compatible UARTS
> * "mediatek,mt8135-uart" for MT8135 compatible UARTS
> * "mediatek,mt8173-uart" for MT8173 compatible UARTS
> + * "mediatek,mt8183-uart", "mediatek,mt6577-uart" for MT8183 compatible UARTS
> * "mediatek,mt6577-uart" for MT6577 and all of the above
>
> - reg: The base address of the UART register bank.
>
Acked-by: Matthias Brugger <matthias.bgg@gmail.com>
Greg will you take this through your branch or do you prefer me to queue it?
Regards,
Matthias
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: Petr Mladek @ 2019-03-12 13:44 UTC (permalink / raw)
To: John Ogness
Cc: Nigel Croxon, Theodore Y. Ts'o, Sergey Senozhatsky,
Greg Kroah-Hartman, Steven Rostedt, Sergey Senozhatsky, dm-devel,
Mikulas Patocka, linux-serial
In-Reply-To: <87wol444cj.fsf@linutronix.de>
On Tue 2019-03-12 14:19:56, John Ogness wrote:
> On 2019-03-12, Mikulas Patocka <mpatocka@redhat.com> wrote:
> >>> But it doesn't turn SMP system into UP.
> >>
> >> In this example it turned it into a brick.
> >>
> >> The problems I see are:
> >>
> >> 1. The current loglevels used in the kernel are not sufficient to
> >> distinguish between emergency and informational messages. Addressing
> >> this issue may require things like using a new printk flag and
> >> manually marking the printks that we(?) decide are critical. I was
>
> Perhaps it would be nice if an administrator could additionally "mark"
> what _they_ consider critical, i.e. they are aware of and accept the
> consequences of a synchronous printk for certain messages. When I look
> at things like dynamic_printk and ftrace, I see mechanisms that provide
> excellent runtime tuning without much complexity.
>
> We could extend dynamic_printk to include all printk messages and also
> support the emergency classification. We could have something like
> ftrace-events, where certain _events_ (i.e a group of printk messages)
> could be classified as an emergency (for example, a WARN_ON event, a
> watchdog event, a general stacktrace event, etc.).
>
> These are just ideas that I'm bouncing around my brain. But as you said,
> only the administrator can know what is important. So perhaps we need to
> provide the administrator an interface to specify that.
Interesting idea. But I am very sceptical about it. I can't imagine
that anyone would like to configure 100k messages or so.
It is even worse than kernel configuration. And the trend is
to avoid config options where possible. It is because it is
almost impossible to understand all existing options and
make qualified decisions. And it is hard to expect that
users would be able to make a better decision than a developer
who implemented the code behind the option.
Our discussion is a nice example. Even we have troubles to
understand all consequences of different approaches. Even
if we reach a conclusion and document it. Then nobody
would want to read several pages long tutorial how to
configure printk ;-)
Best Regards,
Petr
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: John Ogness @ 2019-03-12 13:19 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Petr Mladek, Nigel Croxon, Theodore Y. Ts'o,
Sergey Senozhatsky, Greg Kroah-Hartman, Steven Rostedt,
Sergey Senozhatsky, dm-devel, linux-serial
In-Reply-To: <alpine.LRH.2.02.1903120557560.12085@file01.intranet.prod.int.rdu2.redhat.com>
On 2019-03-12, Mikulas Patocka <mpatocka@redhat.com> wrote:
>>> But it doesn't turn SMP system into UP.
>>
>> In this example it turned it into a brick.
>>
>> The problems I see are:
>>
>> 1. The current loglevels used in the kernel are not sufficient to
>> distinguish between emergency and informational messages. Addressing
>> this issue may require things like using a new printk flag and
>> manually marking the printks that we(?) decide are critical. I was
>
> It depends on what the user considers important. The kernel doesn't
> know what's critical and can't know it.
>
> For example, if the user plugs an USB stick and the USB stick reports
> an I/O error, it is not critical condition.
>
> If the user runs the whole system from the USB stick and the USB stick
> reports an I/O error, it is critical condition.
I would argue that it is _not_ a critical condition. I/O errors are
something that happen. This is normal operation.
_My_ definition of critical is when something with the _kernel itself_
has gone wrong. If the kernel is in error paths that should never happen
(for example, when a WARN_ON is hit) or if a BUG/panic is hit. This is
critical information.
> The filesystems and the block device drivers can't know if the data
> stored on them are important.
Perhaps it would be nice if an administrator could additionally "mark"
what _they_ consider critical, i.e. they are aware of and accept the
consequences of a synchronous printk for certain messages. When I look
at things like dynamic_printk and ftrace, I see mechanisms that provide
excellent runtime tuning without much complexity.
We could extend dynamic_printk to include all printk messages and also
support the emergency classification. We could have something like
ftrace-events, where certain _events_ (i.e a group of printk messages)
could be classified as an emergency (for example, a WARN_ON event, a
watchdog event, a general stacktrace event, etc.).
These are just ideas that I'm bouncing around my brain. But as you said,
only the administrator can know what is important. So perhaps we need to
provide the administrator an interface to specify that.
John Ogness
^ permalink raw reply
* Re: [RFC PATCH v1 00/25] printk: new implementation
From: Petr Mladek @ 2019-03-12 12:38 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: John Ogness, linux-kernel, Peter Zijlstra, Steven Rostedt,
Daniel Wang, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20190311105411.GA368@jagdpanzerIV>
On Mon 2019-03-11 19:54:11, Sergey Senozhatsky wrote:
> On (03/07/19 10:53), John Ogness wrote:
> > Since all current console drivers are already irq safe, I'm
> > wondering if using irq_work to handle the emergency printing for console
> > drivers without write_atomic() would help. (If the printk caller is in a
> > context that write() supports, then write() could be called directly.)
> > This would also demand that the irq-safe requirements for write() are
> > not relaxed. The printk-kthread might still be faster than irq_work, but
> > it might increase reliability if an irq_work is triggered as an extra
> > precaution.
>
> Hmm. OK. So one of the things with printk is that it's fully sequential.
> We call console drivers one by one. Slow consoles can affect what appears
> on the fast consoles; fast console have no impact on slow ones.
>
> call_console_drivers()
> for_each_console(c)
> c->write(c, text, text_len);
>
> So a list of (slow_serial serial netcon) console drivers is a camel train;
> fast netcon is not fast anymore, and slow consoles sometimes are the reason
> we have dropped messages. And if we drop messages we drop them for all
> consoles, including fast netcon. Turning that sequential pipline into a
> bunch of per-console kthreads/irq and letting fast consoles to be fast is
> not a completely bad thing. Let's think more about this, I'd like to read
> more opinions.
Per-console kthread sounds interesting but there is the problem with
reliability. I mean that kthread need not get scheduled.
Some of these problems might get solved by the per-console loglevel
patchset.
Sigh, any feature might be useful in some situation. But we always
have to consider the cost and the gain. I wonder how common is
to actively use two consoles at the same time and what would
be the motivation.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v8 2/2] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Matthias Brugger @ 2019-03-12 12:22 UTC (permalink / raw)
To: Erin Lo, Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, mars.cheng,
eddie.huang, linux-clk, zhiyong.tao, Mengqi.Zhang, Ben Ho,
Seiya Wang, Weiyi Lu, Hsin-Hsiung Wang
In-Reply-To: <1552294472-32929-3-git-send-email-erin.lo@mediatek.com>
On 11/03/2019 09:54, Erin Lo wrote:
> From: Ben Ho <Ben.Ho@mediatek.com>
>
> Add basic chip support for Mediatek 8183, include
> uart node with correct uart clocks, pwrap device
>
> Add clock controller nodes, include topckgen, infracfg,
> apmixedsys and subsystem.
>
> Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
> Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
> arch/arm64/boot/dts/mediatek/Makefile | 1 +
> arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 31 +++
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 335 ++++++++++++++++++++++++++++
> 3 files changed, 367 insertions(+)
> create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
>
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index e8f952f..458bbc4 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -7,3 +7,4 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
> dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-evb.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> new file mode 100644
> index 0000000..9b52559
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Ben Ho <ben.ho@mediatek.com>
> + * Erin Lo <erin.lo@mediatek.com>
> + */
> +
> +/dts-v1/;
> +#include "mt8183.dtsi"
> +
> +/ {
> + model = "MediaTek MT8183 evaluation board";
> + compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
> +
> + aliases {
> + serial0 = &uart0;
> + };
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0 0x40000000 0 0x80000000>;
> + };
> +
> + chosen {
> + stdout-path = "serial0:921600n8";
> + };
> +};
> +
> +&uart0 {
> + status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> new file mode 100644
> index 0000000..64f8bd6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -0,0 +1,335 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Ben Ho <ben.ho@mediatek.com>
> + * Erin Lo <erin.lo@mediatek.com>
> + */
> +
> +#include <dt-bindings/clock/mt8183-clk.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> + compatible = "mediatek,mt8183";
> + interrupt-parent = <&sysirq>;
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu-map {
> + cluster0 {
> + core0 {
> + cpu = <&cpu0>;
> + };
> + core1 {
> + cpu = <&cpu1>;
> + };
> + core2 {
> + cpu = <&cpu2>;
> + };
> + core3 {
> + cpu = <&cpu3>;
> + };
> + };
> +
> + cluster1 {
> + core0 {
> + cpu = <&cpu4>;
> + };
> + core1 {
> + cpu = <&cpu5>;
> + };
> + core2 {
> + cpu = <&cpu6>;
> + };
> + core3 {
> + cpu = <&cpu7>;
> + };
> + };
> + };
> +
> + cpu0: cpu@0 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x000>;
> + enable-method = "psci";
> + };
> +
> + cpu1: cpu@1 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x001>;
> + enable-method = "psci";
> + };
> +
> + cpu2: cpu@2 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x002>;
> + enable-method = "psci";
> + };
> +
> + cpu3: cpu@3 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a53";
> + reg = <0x003>;
> + enable-method = "psci";
> + };
> +
> + cpu4: cpu@100 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x100>;
> + enable-method = "psci";
> + };
> +
> + cpu5: cpu@101 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x101>;
> + enable-method = "psci";
> + };
> +
> + cpu6: cpu@102 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x102>;
> + enable-method = "psci";
> + };
> +
> + cpu7: cpu@103 {
> + device_type = "cpu";
> + compatible = "arm,cortex-a73";
> + reg = <0x103>;
> + enable-method = "psci";
> + };
> + };
> +
> + pmu-a53 {
> + compatible = "arm,cortex-a53-pmu";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW &ppi_cluster0>;
> + };
> +
> + pmu-a73 {
> + compatible = "arm,cortex-a73-pmu";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW &ppi_cluster1>;
> + };
> +
> + psci {
> + compatible = "arm,psci-1.0";
> + method = "smc";
> + };
> +
> + clk26m: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <26000000>;
> + clock-output-names = "clk26m";
> + };
> +
> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW 0>,
> + <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW 0>,
> + <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW 0>,
> + <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW 0>;
> + };
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + ranges;
> +
> + gic: interrupt-controller@c000000 {
> + compatible = "arm,gic-v3";
> + #interrupt-cells = <4>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + reg = <0 0x0c000000 0 0x40000>, /* GICD */
> + <0 0x0c100000 0 0x200000>, /* GICR */
> + <0 0x0c400000 0 0x2000>, /* GICC */
> + <0 0x0c410000 0 0x1000>, /* GICH */
> + <0 0x0c420000 0 0x2000>; /* GICV */
> +
> + interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
> + ppi-partitions {
> + ppi_cluster0: interrupt-partition-0 {
> + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
> + };
> + ppi_cluster1: interrupt-partition-1 {
> + affinity = <&cpu4 &cpu5 &cpu6 &cpu7>;
> + };
> + };
> + };
> +
> + mcucfg: syscon@c530000 {
> + compatible = "mediatek,mt8183-mcucfg", "syscon";
Binding is not documented. I found some other bindings not documented as well.
Please update the binding documentation to add this compatible, otherwise I
won't be able to take this patch.
Regards,
Matthias
> + reg = <0 0x0c530000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + sysirq: interrupt-controller@c530a80 {
> + compatible = "mediatek,mt8183-sysirq",
> + "mediatek,mt6577-sysirq";
> + interrupt-controller;
> + #interrupt-cells = <3>;
> + interrupt-parent = <&gic>;
> + reg = <0 0x0c530a80 0 0x50>;
> + };
> +
> + topckgen: syscon@10000000 {
> + compatible = "mediatek,mt8183-topckgen", "syscon";
> + reg = <0 0x10000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + infracfg: syscon@10001000 {
> + compatible = "mediatek,mt8183-infracfg", "syscon";
> + reg = <0 0x10001000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + pio: pinctrl@10005000 {
> + compatible = "mediatek,mt8183-pinctrl";
> + reg = <0 0x10005000 0 0x1000>,
> + <0 0x11f20000 0 0x1000>,
> + <0 0x11e80000 0 0x1000>,
> + <0 0x11e70000 0 0x1000>,
> + <0 0x11e90000 0 0x1000>,
> + <0 0x11d30000 0 0x1000>,
> + <0 0x11d20000 0 0x1000>,
> + <0 0x11c50000 0 0x1000>,
> + <0 0x11f30000 0 0x1000>,
> + <0 0x1000b000 0 0x1000>;
> + reg-names = "iocfg0", "iocfg1", "iocfg2",
> + "iocfg3", "iocfg4", "iocfg5",
> + "iocfg6", "iocfg7", "iocfg8",
> + "eint";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio-ranges = <&pio 0 0 192>;
> + interrupt-controller;
> + interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> + #interrupt-cells = <3>;
> + };
> +
> + apmixedsys: syscon@1000c000 {
> + compatible = "mediatek,mt8183-apmixedsys", "syscon";
> + reg = <0 0x1000c000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + pwrap: pwrap@1000d000 {
> + compatible = "mediatek,mt8183-pwrap";
> + reg = <0 0x1000d000 0 0x1000>;
> + reg-names = "pwrap";
> + interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&topckgen CLK_TOP_MUX_PMICSPI>,
> + <&infracfg CLK_INFRA_PMIC_AP>;
> + clock-names = "spi", "wrap";
> + };
> +
> + uart0: serial@11002000 {
> + compatible = "mediatek,mt8183-uart",
> + "mediatek,mt6577-uart";
> + reg = <0 0x11002000 0 0x1000>;
> + interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&clk26m>, <&infracfg CLK_INFRA_UART0>;
> + clock-names = "baud", "bus";
> + status = "disabled";
> + };
> +
> + uart1: serial@11003000 {
> + compatible = "mediatek,mt8183-uart",
> + "mediatek,mt6577-uart";
> + reg = <0 0x11003000 0 0x1000>;
> + interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&clk26m>, <&infracfg CLK_INFRA_UART1>;
> + clock-names = "baud", "bus";
> + status = "disabled";
> + };
> +
> + uart2: serial@11004000 {
> + compatible = "mediatek,mt8183-uart",
> + "mediatek,mt6577-uart";
> + reg = <0 0x11004000 0 0x1000>;
> + interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&clk26m>, <&infracfg CLK_INFRA_UART2>;
> + clock-names = "baud", "bus";
> + status = "disabled";
> + };
> +
> + audiosys: syscon@11220000 {
> + compatible = "mediatek,mt8183-audiosys", "syscon";
> + reg = <0 0x11220000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + mfgcfg: syscon@13000000 {
> + compatible = "mediatek,mt8183-mfgcfg", "syscon";
> + reg = <0 0x13000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + mmsys: syscon@14000000 {
> + compatible = "mediatek,mt8183-mmsys", "syscon";
> + reg = <0 0x14000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + imgsys: syscon@15020000 {
> + compatible = "mediatek,mt8183-imgsys", "syscon";
> + reg = <0 0x15020000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + vdecsys: syscon@16000000 {
> + compatible = "mediatek,mt8183-vdecsys", "syscon";
> + reg = <0 0x16000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + vencsys: syscon@17000000 {
> + compatible = "mediatek,mt8183-vencsys", "syscon";
> + reg = <0 0x17000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + ipu_conn: syscon@19000000 {
> + compatible = "mediatek,mt8183-ipu_conn", "syscon";
> + reg = <0 0x19000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + ipu_adl: syscon@19010000 {
> + compatible = "mediatek,mt8183-ipu_adl", "syscon";
> + reg = <0 0x19010000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + ipu_core0: syscon@19180000 {
> + compatible = "mediatek,mt8183-ipu_core0", "syscon";
> + reg = <0 0x19180000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + ipu_core1: syscon@19280000 {
> + compatible = "mediatek,mt8183-ipu_core1", "syscon";
> + reg = <0 0x19280000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> + camsys: syscon@1a000000 {
> + compatible = "mediatek,mt8183-camsys", "syscon";
> + reg = <0 0x1a000000 0 0x1000>;
> + #clock-cells = <1>;
> + };
> + };
> +};
>
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: Petr Mladek @ 2019-03-12 12:08 UTC (permalink / raw)
To: John Ogness
Cc: Nigel Croxon, Theodore Y. Ts'o, Sergey Senozhatsky,
Greg Kroah-Hartman, Steven Rostedt, Sergey Senozhatsky, dm-devel,
Mikulas Patocka, linux-serial
In-Reply-To: <87a7i05wwi.fsf@linutronix.de>
On Tue 2019-03-12 09:17:49, John Ogness wrote:
> On 2019-03-12, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> The problems I see are:
>
> 1. The current loglevels used in the kernel are not sufficient to
> distinguish between emergency and informational messages. Addressing
> this issue may require things like using a new printk flag and
> manually marking the printks that we(?) decide are critical. I was
> hoping we could use existing loglevels, but this appears to be such a
> mess that it is probably not practically/politically fixable
> [0]. Maybe it could be a combination of flag and loglevel, where
> certain messages have been flagged by the kernel developers as
> emergency (for example BUG output) and the user still has the
> flexibility of setting a loglevel. I need more input here.
No, please! No extra flag could safe us if people are not able
to set loglevel correctly. Also the importance depends on the
situation. Any message is as important as it helps to resolve
the problem.
> 2. You seem unwilling to acknowledge the difference between emergency
> and informational messages. A message is either critical or it is
> not. If it is, it should be handled as such, regardless of
> interference, regardless if it means turning an SMP machine into a UP
> machine. If it is not critical, it should be sent along a
> non-interfering path so the the system is _not_ affected.
This means that any critical message is always more important than any
workload. It opens doors for iteresting DOS attacks.
> The current printk implementation is handling all console printing as
> best effort. Trying hard enough to dramatically affect the system, but
> not trying hard enough to guarantee success.
I agree that direct output is more reliable. It might be very useful
for debugging some types of problems. The question is
if it is worth the cost (code complexity, serializing CPUs
== slowing down the entire system).
But it is is possible that a reasonable offloading (in the direction
of last Sergey's approach) might be a better deal.
I suggest the following way forward (separate patchsets):
1. Replace log buffer (least controversial thing)
2. Reliable offload to kthread (would be useful anyway)
3. Atomic consoles (a lot of tricky code, might not be
worth the effort)
Could we agree on this?
Best Regards,
Petr
^ permalink raw reply
* Re: [RFC PATCH v1 08/25] printk: add ring buffer and kthread
From: Petr Mladek @ 2019-03-12 10:30 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Sergey Senozhatsky, linux-kernel,
Peter Zijlstra, Steven Rostedt, Daniel Wang, Andrew Morton,
Linus Torvalds, Greg Kroah-Hartman, Alan Cox, Jiri Slaby,
Peter Feiner, linux-serial
In-Reply-To: <87va0pznsq.fsf@linutronix.de>
On Mon 2019-03-11 11:51:49, John Ogness wrote:
> On 2019-03-07, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> > I don't really understand the role of loglevel anymore.
>
> "what the kernel considers" is a configuration option of the
> administrator. The administrator can increase the verbocity of the
> console (loglevel) without having negative effects on the system
> itself.
Where do you get the confidence that atomic console will not
slow down the system? Have you tried it on real life workload
when debugging a real life bug?
Some benchmarks might help. Well, it would be needed to
trigger some messages from them and see how the different
approaches affect the overall system performance.
> Also, if the system were to suddenly crash, those crash messages
> shouldn't be in jeopardy just because the verbocity of the console was
> turned up.
This expects that the error messages will be enough to discover
and fix the problem.
> You (and Petr) talk about that _all_ console printing is for
> emergencies. That if an administrator sets the loglevel to 7 it is
> because the pr_info messages are just as important as the pr_emerg.
It might be true when the messages with higher level (more critical)
are not enough to understand the situation.
> And if that is indeed the intention of console printing and loglevel, then
> why is asynchronous printk calls for console messages even allowed
> today? IMO that isn't taking the importance of the message very
> seriously.
Because it was working pretty well in the past. The amount of messages
is still growing (code complexity, more CPUs, more devices, ...).
Our customers have started reporting softlockups "only" 7 years ago
or so.
We currently have two level handling of messages:
+ all messages can be seen from userspace
+ messages below console_loglevel can be seen
on the console
You are introducing one more level of handling:
+ critical messages are printed on the console directly
even before the queued less critical ones
The third level would be acceptable when:
+ atomic consoles are reliable enough
+ the code complexity is worth the gain
IMHO, we mix too many things here:
+ log buffer implementation
+ console offload
+ direct console handling using atomic consoles
I see the potential in all areas:
+ lock less ring buffer helps to avoid deadlocks,
and extra log buffers
+ console offload prevents too long stalls (softlockups)
+ direct console handling might help to avoid deadlocks
and might make the output more reliable.
I think that we are on the same page here.
But we must use an incremental approach. It is not acceptable
to replace everything by a single patch. And it is not acceptable
to break important functionality and implement alternative
solution several patches later.
Also no solution is as ideal as it is sometimes presented
in this thread.
Best Regards,
Petr
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: Mikulas Patocka @ 2019-03-12 10:05 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Nigel Croxon, Theodore Y. Ts'o,
Sergey Senozhatsky, Greg Kroah-Hartman, Steven Rostedt,
Sergey Senozhatsky, dm-devel, linux-serial
In-Reply-To: <87a7i05wwi.fsf@linutronix.de>
On Tue, 12 Mar 2019, John Ogness wrote:
> > But it doesn't turn SMP system into UP.
>
> In this example it turned it into a brick.
>
> The problems I see are:
>
> 1. The current loglevels used in the kernel are not sufficient to
> distinguish between emergency and informational messages. Addressing
> this issue may require things like using a new printk flag and
> manually marking the printks that we(?) decide are critical. I was
It depends on what the user considers important. The kernel doesn't know
what's critical and can't know it.
For example, if the user plugs an USB stick and the USB stick reports an
I/O error, it is not critical condition.
If the user runs the whole system from the USB stick and the USB stick
reports an I/O error, it is critical condition.
The filesystems and the block device drivers can't know if the data stored
on them are important.
Mikulas
^ permalink raw reply
* Re: [RFC PATCH v1 08/25] printk: add ring buffer and kthread
From: Sergey Senozhatsky @ 2019-03-12 9:58 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Sergey Senozhatsky, linux-kernel,
Peter Zijlstra, Petr Mladek, Steven Rostedt, Daniel Wang,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial
In-Reply-To: <87va0pznsq.fsf@linutronix.de>
On (03/11/19 11:51), John Ogness wrote:
> > In new printk design the tasks are still affected by printing floods.
> > Tasks have to line up and (busy) wait for each other, regardless of
> > contexts.
>
> They only line up and busy wait is to add the informational message to
> the ring buffer. The current printk implementation is the same in this
> respect. And as you've noted, the logbuf spinlock is not a source of
> latencies.
I was talking about prb_lock().
> > When I do ./a.out --loglevel=X I have a clear understanding that
> > all messages which fall into [critical, X] range will be in the logs,
> > because I told that application that those messages are important to
> > me right now. And it used to be the same with the kernel loglevel.
>
> The loglevel is not related to logging. It specifies the amount of
> console printing. But I will assume you are referring to creating log
> files by having an external device store the console printing.
Right. E.g. screenlog.0
> > But now the kernel will do its own thing:
> >
> > - what the kernel considers important will go into the logs
> > - what the kernel doesn't consider important _maybe_ will end up
> > in the logs (preemptible printk kthread). And this is where
> > loglevel now. After the _maybe_ part.
>
> "what the kernel considers" is a configuration option of the
> administrator. The administrator can increase the verbocity of the
> console (loglevel) without having negative effects on the system
> itself. Also, if the system were to suddenly crash, those crash messages
> shouldn't be in jeopardy just because the verbocity of the console was
> turned up.
Right. I'm not very sure about yet another knob which everyone
should figure out. I guess I won't be surprised to find out that
people set it to loglevel value.
-ss
^ permalink raw reply
* Re: [RFC PATCH v1 25/25] printk: remove unused code
From: Petr Mladek @ 2019-03-12 9:38 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Sergey Senozhatsky, John Ogness, linux-kernel, Peter Zijlstra,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky
In-Reply-To: <20190311081826.k7ybxniveuqeaxzp@flow>
On Mon 2019-03-11 09:18:26, Sebastian Andrzej Siewior wrote:
> On 2019-03-11 11:46:00 [+0900], Sergey Senozhatsky wrote:
> > On (03/08/19 15:02), Sebastian Andrzej Siewior wrote:
> > > On 2019-02-12 15:30:03 [+0100], John Ogness wrote:
> > >
> > > you removed the whole `irq_work' thing. You can also remove the include
> > > for linux/irq_work.h.
> >
> > It may be too early to remove the whole `irq_work' thing.
> > printk()->call_console_driver() should take console_sem
> > lock.
>
> I would be _very_ glad to see that irq_work thingy gone. I just stumbled
> upon this irq_work and cursed a little while doing other things.
Have you seen stalls causes by the irq work? Or was it just
glancing over the printk code?
> Checking John's series and seeing that it was gone, was a relief.
> Printing the whole thing in irq context does not look sane. printing the
> import things right away and printing the remaining things later in
> kthread looks good to me.
The irq_work was originally added to handle messages from the
scheduler by printk_deferred(). It was later used also to
handle messages from NMI and printk recursion.
It means that the use is pretty limited. It is more reliable
than a kthread, especially when the scheduler is reporting
problems. IMHO, it is a reasonable solution as long
as the amount of messages is low.
The real time kernel is another story but it has special
handling in many other situations.
That said, the kthread might still make sense as a fallback.
It would be nice to have a hard limit for handling messages
in irq context.
Best Regards,
Petr
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: Sergey Senozhatsky @ 2019-03-12 8:59 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Nigel Croxon, Theodore Y. Ts'o,
Sergey Senozhatsky, Greg Kroah-Hartman, Steven Rostedt,
Sergey Senozhatsky, dm-devel, Mikulas Patocka, linux-serial
In-Reply-To: <87a7i05wwi.fsf@linutronix.de>
On (03/12/19 09:17), John Ogness wrote:
> > wait M times (N - 1). Sounds quadratic.
>
> If these are critical messages, then we are _not allowed to drop any_!
> For critical messages printk must be synchronous. Thus for critical
> messages the situation you illustrated is appropriate.
>
> > 40) goto 10
> >
> > So I have some doubts regarding some of assumptions behind new printk
> > design. And the problem is not in prb_lock() unfairness. Current
> > printk design does look to me SMP-friendly; yes, it has unbound
> > printing loop; that can be addressed.
>
> Let us not forget, it deadlocked the machine. That's the reason this
> thread exists.
It didn't deadlock the machine. It was a typical soft lockup. Printing
CPU loop-ed in console_unlock() with preemption disabled; soft lockup
hrtimer was running on that CPU, but due to disabled preemption around
console_unlock() soft lockup's per-CPU kthread could not get scheduled
and could not update per-CPU touch_ts. Soft lockup hrtimer detected it:
[ 5128.552442] watchdog: BUG: soft lockup - CPU#9 stuck for 23s! [kworker/9:53:4131]
Along with that RCU was not able to get scheduled. Which was detected
by RCU stall detector:
[ 4891.199009] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
[ 4891.221308] device-mapper: integrity: Checksum failed at sector 0x118d4f
[ 4891.251366] rcu: 9-....: (1923 ticks this GP) idle=7fa/1/0x4000000000000002 softirq=2190/2190 fqs=15013
[ 4891.251367] rcu: (detected by 16, t=60054 jiffies, g=24641, q=351)
[ 4891.311941] Sending NMI from CPU 16 to CPUs 9:
[..]
> 2. You seem unwilling to acknowledge the difference between emergency
> and informational messages. A message is either critical or it is
> not. If it is, it should be handled as such, regardless of
> interference, regardless if it means turning an SMP machine into a UP
> machine. If it is not critical, it should be sent along a
> non-interfering path so the the system is _not_ affected.
OK.
Let's move on then.
-ss
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: John Ogness @ 2019-03-12 8:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Nigel Croxon, Theodore Y. Ts'o,
Greg Kroah-Hartman, Steven Rostedt, Sergey Senozhatsky, dm-devel,
Mikulas Patocka, linux-serial
In-Reply-To: <20190312023231.GA4146@jagdpanzerIV>
On 2019-03-12, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>>> John, sorry to ask this, does new printk() design always provide
>>> latency guarantees good enough for PREEMPT_RT?
>>
>> Yes, because it is assumed that emergency messages will never occur
>> for a correctly running system.
>>
> [..]
>> Obviously as soon as any emergency message appears, an _unacceptable_
>> latency occurs. But that is considered OK because the system is no
>> longer running correctly and it is worth the price to pay to get
>> those messages with such high reliability.
>
> OK, so what *I'm learning* from this bug report:
>
> 10) WARN/ERR messages do not necessarily tell us that the stability of
> the system was jeopardized. The system can "run correctly" and be
> "perfectly healthy".
If the messages from this report are not critical, they should not be
classified as emergency messages. It is a bug if they are.
> 20) We can have N CPUs reporting issues simultaneously. Even in
> production. Such patterns exist in the kernel.
Sure. But it is important to distinguish if these messages are critical
or just informational.
> 30) The "reporting part" - printk()->call_console_drivers() - can be
> the slowest one.
>
> In this particular case, given that Mikulas saw dropped messages,
> checksum calculation was significantly faster than
> call_console_drivers(). Now, suppose we have new printk, and
> suppose we have CPUs A B C D, each of them reports a checksum error:
>
> A prb_lock owner B prb_lock C prb_lock D prb_lock
>
> A calls call_console_drivers, unlocks prb_lock
> B grabs prb_lock
> B calls call_console_drivers
> A calculates new checksum mismatch
> A calls printk and spins on prb_lock, behind D
>
> So now we have:
>
> B prb_lock owner C prb_lock D prb_lock A prb_lock
>
> And so on
>
> B C D A -> C D A B -> D A B C -> A B C D -> ...
>
> After M rounds of error reporting (M > N), each CPU, had have to busy
> wait M times (N - 1). Sounds quadratic.
If these are critical messages, then we are _not allowed to drop any_!
For critical messages printk must be synchronous. Thus for critical
messages the situation you illustrated is appropriate.
> 40) goto 10
>
> So I have some doubts regarding some of assumptions behind new printk
> design. And the problem is not in prb_lock() unfairness. Current
> printk design does look to me SMP-friendly; yes, it has unbound
> printing loop; that can be addressed.
Let us not forget, it deadlocked the machine. That's the reason this
thread exists.
> But it doesn't turn SMP system into UP.
In this example it turned it into a brick.
The problems I see are:
1. The current loglevels used in the kernel are not sufficient to
distinguish between emergency and informational messages. Addressing
this issue may require things like using a new printk flag and
manually marking the printks that we(?) decide are critical. I was
hoping we could use existing loglevels, but this appears to be such a
mess that it is probably not practically/politically fixable
[0]. Maybe it could be a combination of flag and loglevel, where
certain messages have been flagged by the kernel developers as
emergency (for example BUG output) and the user still has the
flexibility of setting a loglevel. I need more input here.
2. You seem unwilling to acknowledge the difference between emergency
and informational messages. A message is either critical or it is
not. If it is, it should be handled as such, regardless of
interference, regardless if it means turning an SMP machine into a UP
machine. If it is not critical, it should be sent along a
non-interfering path so the the system is _not_ affected.
The current printk implementation is handling all console printing as
best effort. Trying hard enough to dramatically affect the system, but
not trying hard enough to guarantee success.
John Ogness
[0] https://lkml.kernel.org/r/f60d844d-9d3b-3154-4eec-982432c8e502@redhat.com
^ permalink raw reply
* Re: [RFC PATCH v1 19/25] printk: introduce emergency messages
From: Sergey Senozhatsky @ 2019-03-12 2:58 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, John Ogness, linux-kernel, Peter Zijlstra,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky
In-Reply-To: <20190308103127.txsgv3d6lqlf6pad@pathway.suse.cz>
On (03/08/19 11:31), Petr Mladek wrote:
> Great catch!
>
> I think that it is doable to guard the list using RCU.
I think console_sem is more than just list lock.
E.g. fb_flashcursor() - console_sem protects framebuffer from concurrent
modifications. And many other examples.
I think the last time we talked about it (two+ years ago) we counted
5 or 7 (don't remember exactly) different things which console_sem
does.
-ss
^ permalink raw reply
* Re: [RFC PATCH v1 19/25] printk: introduce emergency messages
From: Sergey Senozhatsky @ 2019-03-12 2:51 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Sergey Senozhatsky, linux-kernel, Peter Zijlstra,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky
In-Reply-To: <87k1h5zkfn.fsf@linutronix.de>
On (03/11/19 13:04), John Ogness wrote:
> > Great catch!
>
> Yes, thanks!
>
> > I think that it is doable to guard the list using RCU.
>
> I think it would be enough to take the prb_cpulock when modifying the
> console linked list. That will keep printk_emergency() out until the
> list has been updated. (registering/unregistering consoles is not
> something that happens often.)
console_sem can be a bit more than just registering/unregistering.
Especially when it comes to VT, fbcon and ioctls.
$ git grep console_lock drivers/tty/ | wc -l
82
$ git grep console_lock drivers/video/fbdev/ | wc -l
80
-ss
^ permalink raw reply
* Re: Serial console is causing system lock-up
From: Sergey Senozhatsky @ 2019-03-12 2:32 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Nigel Croxon, Theodore Y. Ts'o,
Sergey Senozhatsky, Greg Kroah-Hartman, Steven Rostedt,
Sergey Senozhatsky, dm-devel, Mikulas Patocka, linux-serial
In-Reply-To: <87r2biojcx.fsf@linutronix.de>
On (03/07/19 15:21), John Ogness wrote:
> > John, sorry to ask this, does new printk() design always provide
> > latency guarantees good enough for PREEMPT_RT?
>
> Yes, because it is assumed that emergency messages will never occur for
> a correctly running system.
>
[..]
> Obviously as soon as any emergency message appears, an _unacceptable_
> latency occurs. But that is considered OK because the system is no
> longer running correctly and it is worth the price to pay to get those
> messages with such high reliability.
OK, so what *I'm learning* from this bug report:
10) WARN/ERR messages do not necessarily tell us that the stability of the
system was jeopardized. The system can "run correctly" and be
"perfectly healthy".
20) We can have N CPUs reporting issues simultaneously. Even in production.
Such patterns exist in the kernel.
30) The "reporting part" - printk()->call_console_drivers() - can be the
slowest one.
In this particular case, given that Mikulas saw dropped messages,
checksum calculation was significantly faster than call_console_drivers().
Now, suppose we have new printk, and suppose we have CPUs A B C D, each of
them reports a checksum error:
A prb_lock owner B prb_lock C prb_lock D prb_lock
A calls call_console_drivers, unlocks prb_lock
B grabs prb_lock
B calls call_console_drivers
A calculates new checksum mismatch
A calls printk and spins on prb_lock, behind D
So now we have:
B prb_lock owner C prb_lock D prb_lock A prb_lock
And so on
B C D A -> C D A B -> D A B C -> A B C D -> ...
After M rounds of error reporting (M > N), each CPU, had have to busy
wait M times (N - 1). Sounds quadratic.
40) goto 10
So I have some doubts regarding some of assumptions behind new printk
design. And the problem is not in prb_lock() unfairness. Current printk
design does look to me SMP-friendly; yes, it has unbound printing loop;
that can be addressed. But it doesn't turn SMP system into UP.
-ss
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: maowenan @ 2019-03-11 14:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
In-Reply-To: <20190311124616.GE2412@kadam>
On 2019/3/11 20:46, Dan Carpenter wrote:
> On Mon, Mar 11, 2019 at 05:51:15PM +0800, Mao Wenan wrote:
>> Add the missing uart_unregister_driver() before return
>> from sci_probe_single() in the error handling case.
>>
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>
> Sorry, I didn't really look at the code when I saw the v1 patch.
>
> There are other error paths, but actually the whole approach is wrong.
> Please, read my google plus post about error handling:
>
> https://plus.google.com/u/0/106378716002406849458/posts/1Ud9JbaYnPr
>
OK.
> But then the other rule I didn't mention in that post which applies
> here is that the error handling should "mirror" the allocation code
> so if you have:
>
> if (foo) {
> ret = allocate_one();
> if (ret)
> return ret;
> }
> ret = allocate_two();
> if (ret)
> goto free_one;
>
> The error handling should mirror the "if (foo) " condition. Like this:
>
> free_one:
> if (foo)
> free_one();
>
> Even if you can do extra analysis and find that the "if (foo) " can
> be removed, you should leave there, because the mirroring helps human
> readers.
>
> In this case, the code is doing:
>
> drivers/tty/serial/sh-sci.c
> 3259 return -EBUSY;
> 3260
> 3261 mutex_lock(&sci_uart_registration_lock);
> 3262 if (!sci_uart_driver.state) {
> ^^^^^^^^^^^^^^^^^^^^^^
> 3263 ret = uart_register_driver(&sci_uart_driver);
> 3264 if (ret) {
> 3265 mutex_unlock(&sci_uart_registration_lock);
> 3266 return ret;
> 3267 }
> 3268 }
> 3269 mutex_unlock(&sci_uart_registration_lock);
> 3270
>
> We would have to mirror the "if (!sci_uart_driver.state) {" code.
>
> But actually, we can't.
>
> The first driver to hit this code is supposed to load the
> sci_uart_driver. We can't know if we are the last driver to stop using
> the sci_uart_driver so we can't know if we can free it. This looks like
> a very ugly hack to me. It should probably be using ref counters.
It seems something should be considered deeply.
>
> regards,
> an carpenter
>
> .
>
^ permalink raw reply
* Re: [PATCH 3/4] printk: Add consoles to a virtual "console" bus
From: Petr Mladek @ 2019-03-11 13:33 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <087b13f7812b32cc7c3f9efea71c9bcf324dd031.1551486732.git.calvinowens@fb.com>
On Fri 2019-03-01 16:48:19, Calvin Owens wrote:
> This patch embeds a device struct in the console struct, and registers
> them on a "console" bus so we can expose attributes in sysfs.
>
> Currently, most drivers declare static console structs, and that is
> incompatible with the dev refcount model. So we end up needing to patch
> all of the console drivers to:
>
> 1. Dynamically allocate the console struct using a new helper
> 2. Handle the allocation in (1) possibly failing
> 3. Dispose of (1) with put_device()
>
> Early console structures must still be static, since they're required
> before we're able to allocate memory. The least ugly way I can come up
> with to handle this is an "is_static" flag in the structure which makes
> the gets and puts NOPs, and is checked in ->release() to catch mistakes.
>
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 5c8d780637bd..e09cb192a469 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -857,12 +857,12 @@ static void lp_console_write(struct console *co, const char *s,
> parport_release(dev);
> }
>
> -static struct console lpcons = {
> - .name = "lp",
> +static const struct console_operations lp_cons_ops = {
> .write = lp_console_write,
> - .flags = CON_PRINTBUFFER,
> };
>
> +static struct console *lpcons;
I have got the following compilation error (see below):
CC drivers/char/lp.o
drivers/char/lp.c: In function ‘lp_register’:
drivers/char/lp.c:925:2: error: ‘lpcons’ undeclared (first use in this function)
lpcons = allocate_console_dfl(&lp_cons_ops, "lp", NULL);
^
drivers/char/lp.c:925:2: note: each undeclared identifier is reported only once for each function it appears in
In file included from drivers/char/lp.c:125:0:
drivers/char/lp.c:925:33: error: ‘lp_cons_ops’ undeclared (first use in this function)
> #endif /* console on line printer */
>
> /* --- initialisation code ------------------------------------- */
> @@ -921,6 +921,11 @@ static int lp_register(int nr, struct parport *port)
> &ppdev_cb, nr);
> if (lp_table[nr].dev == NULL)
> return 1;
> +
> + lpcons = allocate_console_dfl(&lp_cons_ops, "lp", NULL);
> + if (!lpcons)
> + return -ENOMEM;
This should be done inside #ifdef CONFIG_LP_CONSOLE
to avoid the above compilation error.
> +
> lp_table[nr].flags |= LP_EXIST;
>
> if (reset)
[...]
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 3c27a4a29b8c..382591683033 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -142,20 +143,28 @@ static inline int con_debug_leave(void)
> #define CON_BRL (32) /* Used for a braille device */
> #define CON_EXTENDED (64) /* Use the extended output format a la /dev/kmsg */
>
> -struct console {
> - char name[16];
> +struct console;
> +
> +struct console_operations {
> void (*write)(struct console *, const char *, unsigned);
> int (*read)(struct console *, char *, unsigned);
> struct tty_driver *(*device)(struct console *, int *);
> void (*unblank)(void);
> int (*setup)(struct console *, char *);
> int (*match)(struct console *, char *name, int idx, char *options);
> +};
> +
> +struct console {
> + char name[16];
> short flags;
> short index;
> int cflag;
> void *data;
> struct console *next;
> int level;
> + const struct console_operations *ops;
> + struct device dev;
> + int is_static;
> };
>
> /*
> @@ -167,6 +176,29 @@ struct console {
> extern int console_set_on_cmdline;
> extern struct console *early_console;
>
> +extern struct console *allocate_console(const struct console_operations *ops,
> + const char *name, short flags,
> + short index, void *data);
> +
> +#define allocate_console_dfl(ops, name, data) \
> + allocate_console(ops, name, CON_PRINTBUFFER, -1, data)
> +
> +/*
> + * Helpers for get/put that do the right thing for static early consoles.
> + */
> +
> +#define get_console(con) \
> +do { \
> + if (!con->is_static) \
> + get_device(&(con)->dev); \
> +} while (0)
> +
> +#define put_console(con) \
> +do { \
> + if (con && !con->is_static) \
> + put_device(&((struct console *)con)->dev); \
> +} while (0)
> +
> extern int add_preferred_console(char *name, int idx, char *options);
> extern void register_console(struct console *);
> extern int unregister_console(struct console *);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 5fe2b037e833..29b43c4df3d6 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -426,6 +426,25 @@ int uart_add_one_port(struct uart_driver *reg, struct uart_port *port);
> int uart_remove_one_port(struct uart_driver *reg, struct uart_port *port);
> int uart_match_port(struct uart_port *port1, struct uart_port *port2);
>
> +/*
> + * This ugliness removes the need for #ifdef boilerplate in UART drivers which
> + * allow their console functionality to be disabled via Kconfig.
> + */
> +#define uart_allocate_console(drv, ops, name, flags, idx, kcfg) \
> +({ \
> + int __retval = 0; \
> + if (IS_ENABLED(CONFIG_##kcfg)) { \
I wonder if it is worth it. I do not see much existing #ifdef's
removed by this patch. I would expect that the code is never
called when the driver is disabled in Kconfig.
IMHO, such a trick hidden in a macro could cause more confusion
than good.
> + (drv)->cons = allocate_console(ops, name, flags, idx, drv); \
> + __retval = (drv)->cons ? 0 : -ENOMEM; \
> + } \
> + __retval; \
> +})
> +
> +#define uart_allocate_console_dfl(drv, ops, name, kcfg) \
> + uart_allocate_console(drv, ops, name, CON_PRINTBUFFER, -1, kcfg)
> +
> +#define uart_put_console(drv) put_console((drv)->cons)
> +
> /*
> * Power Management
> */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 2e0eb89f046c..67e1e993ab80 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -108,6 +108,8 @@ enum devkmsg_log_masks {
>
> static unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
>
> +static int printk_late_done;
> +
> static int __control_devkmsg(char *str)
> {
> if (!str)
> @@ -1731,7 +1733,7 @@ static void call_console_drivers(const char *ext_text, size_t ext_len,
> continue;
> if (!(con->flags & CON_ENABLED))
> continue;
> - if (!con->write)
> + if (!con->ops->write)
> continue;
> if (!cpu_online(smp_processor_id()) &&
> !(con->flags & CON_ANYTIME))
> @@ -1739,9 +1741,9 @@ static void call_console_drivers(const char *ext_text, size_t ext_len,
> if (suppress_message_printing(level, con))
> continue;
> if (con->flags & CON_EXTENDED)
> - con->write(con, ext_text, ext_len);
> + con->ops->write(con, ext_text, ext_len);
> else
> - con->write(con, text, len);
> + con->ops->write(con, text, len);
> }
> }
>
> @@ -2052,7 +2054,7 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
> n = vscnprintf(buf, sizeof(buf), fmt, ap);
> va_end(ap);
>
> - early_console->write(early_console, buf, n);
> + early_console->ops->write(early_console, buf, n);
> }
> #endif
>
> @@ -2481,8 +2483,8 @@ void console_unblank(void)
> console_locked = 1;
> console_may_schedule = 0;
> for_each_console(c)
> - if ((c->flags & CON_ENABLED) && c->unblank)
> - c->unblank();
> + if ((c->flags & CON_ENABLED) && c->ops->unblank)
> + c->ops->unblank();
> console_unlock();
> }
>
> @@ -2515,9 +2517,9 @@ struct tty_driver *console_device(int *index)
>
> console_lock();
> for_each_console(c) {
> - if (!c->device)
> + if (!c->ops->device)
> continue;
> - driver = c->device(c, index);
> + driver = c->ops->device(c, index);
> if (driver)
> break;
> }
> @@ -2558,6 +2560,68 @@ static int __init keep_bootcon_setup(char *str)
>
> early_param("keep_bootcon", keep_bootcon_setup);
>
> +static struct bus_type console_subsys = {
> + .name = "console",
> +};
> +
> +static void console_release(struct device *dev)
> +{
> + struct console *con = container_of(dev, struct console, dev);
> +
> + if (WARN(con->is_static, "Freeing static early console!\n"))
> + return;
> +
> + if (WARN(con->flags & CON_ENABLED, "Freeing running console!\n"))
> + return;
> +
> + pr_info("Freeing console %s\n", con->name);
> + kfree(con);
> +}
> +
> +static void console_init_device(struct console *con)
> +{
> + device_initialize(&con->dev);
> + dev_set_name(&con->dev, "%s", con->name);
> + con->dev.release = console_release;
> +}
> +
> +static void console_register_device(struct console *new)
> +{
> + /*
> + * We might be called very early from register_console(): in that case,
> + * printk_late_init() will take care of this later.
> + */
> + if (!printk_late_done)
> + return;
> +
> + if (new->is_static)
> + console_init_device(new);
> +
> + new->dev.bus = &console_subsys;
> + WARN_ON(device_add(&new->dev));
> +}
> +
> +struct console *allocate_console(const struct console_operations *ops,
> + const char *name, short flags, short index,
> + void *data)
> +{
> + struct console *new;
> +
> + new = kzalloc(sizeof(*new), GFP_KERNEL);
I have just realized that page_alloc_init() is called before
parse_early_param(). Therefore we should be able to use
memblock_alloc() for early consoles and reduce problems
with static structures.
> + if (!new)
> + return NULL;
> +
> + new->ops = ops;
> + strscpy(new->name, name, sizeof(new->name));
> + new->flags = flags;
> + new->index = index;
> + new->data = data;
> +
> + console_init_device(new);
This is a side effect that I would not expect from a function called
alloc*(). I would rename:
s/allocate_console/create_console/ or
s/allocate_console/init_console/
> + return new;
> +}
> +EXPORT_SYMBOL_GPL(allocate_console);
> +
> /*
> * The console driver calls this routine during kernel initialization
> * to register the console printing procedure with printk() and to
> @@ -2622,10 +2686,10 @@ void register_console(struct console *newcon)
> if (!has_preferred) {
> if (newcon->index < 0)
> newcon->index = 0;
> - if (newcon->setup == NULL ||
> - newcon->setup(newcon, NULL) == 0) {
> + if (newcon->ops->setup == NULL ||
> + newcon->ops->setup(newcon, NULL) == 0) {
> newcon->flags |= CON_ENABLED;
> - if (newcon->device) {
> + if (newcon->ops->device) {
There might be confusion why we need two devices (con->dev
and con->ops->device).
I would rename con->ops->device to con->ops->tty_dev.
> newcon->flags |= CON_CONSDEV;
> has_preferred = true;
> }
> @@ -2639,8 +2703,8 @@ void register_console(struct console *newcon)
> for (i = 0, c = console_cmdline;
> i < MAX_CMDLINECONSOLES && c->name[0];
> i++, c++) {
> - if (!newcon->match ||
> - newcon->match(newcon, c->name, c->index, c->options) != 0) {
> + if (!newcon->ops->match ||
> + newcon->ops->match(newcon, c->name, c->index, c->options) != 0) {
> /* default matching */
> BUILD_BUG_ON(sizeof(c->name) != sizeof(newcon->name));
> if (strcmp(c->name, newcon->name) != 0)
> @@ -2660,8 +2724,8 @@ void register_console(struct console *newcon)
> if (_braille_register_console(newcon, c))
> return;
>
> - if (newcon->setup &&
> - newcon->setup(newcon, c->options) != 0)
> + if (newcon->ops->setup &&
> + newcon->ops->setup(newcon, c->options) != 0)
> break;
> }
>
> @@ -2706,6 +2770,8 @@ void register_console(struct console *newcon)
> console_drivers->next = newcon;
> }
>
> + get_console(newcon);
> +
> if (newcon->flags & CON_EXTENDED)
> nr_ext_console_drivers++;
>
> @@ -2730,6 +2796,7 @@ void register_console(struct console *newcon)
> exclusive_console_stop_seq = console_seq;
> logbuf_unlock_irqrestore(flags);
> }
> + console_register_device(newcon);
This calls console_init_device() for the statically defined
early consoles. I guess that it would invalidate the above
get_console().
Also it is not symmetric with unregister_console(). We add it
to the console_subsys here and did not remove it when
the console is unregistered.
IMHO, this might be done already in allocate_console().
And eventually in printk_late_init() for consoles that
are allocated earlier.
I am not familiar with the device-related sysfs API.
But I see that device_initialize() and device_add()
can be done by device_register(). The separate
calls are needed only when a special reference
handling is needed. Are we special?
Anyway, please, try to describe the expected workflow
in the commit description:
+ where the structure should get allocated
+ when it is added to the sysfs hierarchy
+ what happens when the console gets registered
and unregistered
+ when it is removed from the sysfs hierarchy
+ when the structure is released/freed
+ What is needed to get the console registered
and unregistered repeatedly on a running system
Finally, we might want to show state of the CON_ENABLED flag
in sysfs to distinguish the currently used consoles.
> console_unlock();
> console_sysfs_notify();
Thanks a lot for working on this. I know that it is not easy.
But it is really appreciated.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: Dan Carpenter @ 2019-03-11 12:46 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
In-Reply-To: <20190311095115.156774-1-maowenan@huawei.com>
On Mon, Mar 11, 2019 at 05:51:15PM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() before return
> from sci_probe_single() in the error handling case.
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
Sorry, I didn't really look at the code when I saw the v1 patch.
There are other error paths, but actually the whole approach is wrong.
Please, read my google plus post about error handling:
https://plus.google.com/u/0/106378716002406849458/posts/1Ud9JbaYnPr
But then the other rule I didn't mention in that post which applies
here is that the error handling should "mirror" the allocation code
so if you have:
if (foo) {
ret = allocate_one();
if (ret)
return ret;
}
ret = allocate_two();
if (ret)
goto free_one;
The error handling should mirror the "if (foo) " condition. Like this:
free_one:
if (foo)
free_one();
Even if you can do extra analysis and find that the "if (foo) " can
be removed, you should leave there, because the mirroring helps human
readers.
In this case, the code is doing:
drivers/tty/serial/sh-sci.c
3259 return -EBUSY;
3260
3261 mutex_lock(&sci_uart_registration_lock);
3262 if (!sci_uart_driver.state) {
^^^^^^^^^^^^^^^^^^^^^^
3263 ret = uart_register_driver(&sci_uart_driver);
3264 if (ret) {
3265 mutex_unlock(&sci_uart_registration_lock);
3266 return ret;
3267 }
3268 }
3269 mutex_unlock(&sci_uart_registration_lock);
3270
We would have to mirror the "if (!sci_uart_driver.state) {" code.
But actually, we can't.
The first driver to hit this code is supposed to load the
sci_uart_driver. We can't know if we are the last driver to stop using
the sci_uart_driver so we can't know if we can free it. This looks like
a very ugly hack to me. It should probably be using ref counters.
regards,
an carpenter
^ permalink raw reply
* Re: [PATCH serial v3] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Dan Carpenter @ 2019-03-11 12:19 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, jslaby, linux-serial, linux-kernel, kernel-janitors, vz
In-Reply-To: <20190311093959.86344-1-maowenan@huawei.com>
On Mon, Mar 11, 2019 at 05:39:59PM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() and i2c_del_driver() before
> return from sc16is7xx_init() in the error handling case.
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
This looks nice. Thanks!
Reviewed-by: Dan Carpeter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply
* Re: [RFC PATCH v1 19/25] printk: introduce emergency messages
From: John Ogness @ 2019-03-11 12:04 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, linux-kernel, Peter Zijlstra, Steven Rostedt,
Daniel Wang, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20190308103127.txsgv3d6lqlf6pad@pathway.suse.cz>
On 2019-03-08, Petr Mladek <pmladek@suse.com> wrote:
>>> +static bool console_can_emergency(int level)
>>> +{
>>> + struct console *con;
>>> +
>>> + for_each_console(con) {
>>> + if (!(con->flags & CON_ENABLED))
>>> + continue;
>>> + if (con->write_atomic && level < emergency_console_loglevel)
>>> + return true;
>>> + if (con->write && (con->flags & CON_BOOT))
>>> + return true;
>>> + }
>>> + return false;
>>> +}
>>> +
>>> +static void call_emergency_console_drivers(int level, const char *text,
>>> + size_t text_len)
>>> +{
>>> + struct console *con;
>>> +
>>> + for_each_console(con) {
>>> + if (!(con->flags & CON_ENABLED))
>>> + continue;
>>> + if (con->write_atomic && level < emergency_console_loglevel) {
>>> + con->write_atomic(con, text, text_len);
>>> + continue;
>>> + }
>>> + if (con->write && (con->flags & CON_BOOT)) {
>>> + con->write(con, text, text_len);
>>> + continue;
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void printk_emergency(char *buffer, int level, u64 ts_nsec, u16 cpu,
>>> + char *text, u16 text_len)
>>> +{
>>> + struct printk_log msg;
>>> + size_t prefix_len;
>>> +
>>> + if (!console_can_emergency(level))
>>> + return;
>>> +
>>> + msg.level = level;
>>> + msg.ts_nsec = ts_nsec;
>>> + msg.cpu = cpu;
>>> + msg.facility = 0;
>>> +
>>> + /* "text" must have PREFIX_MAX preceding bytes available */
>>> +
>>> + prefix_len = print_prefix(&msg,
>>> + console_msg_format & MSG_FORMAT_SYSLOG,
>>> + printk_time, buffer);
>>> + /* move the prefix forward to the beginning of the message text */
>>> + text -= prefix_len;
>>> + memmove(text, buffer, prefix_len);
>>> + text_len += prefix_len;
>>> +
>>> + text[text_len++] = '\n';
>>> +
>>> + call_emergency_console_drivers(level, text, text_len);
>>
>> So this iterates the console list and calls consoles' callbacks, but
>> what prevents console driver to be rmmod-ed under us?
>>
>> CPU0 CPU1
>>
>> printk_emergency() rmmod netcon
>> call_emergency_console_drivers()
>> con_foo->flags & CON_ENABLED == 1
>> unregister_console(con_foo)
>> con_foo->flags &= ~CON_ENABLED
>> __exit // con_foo gone ?
>> con_foo->write()
>>
>> We use console_lock()/console_trylock() in order to protect the list
>> and console drivers; but this brings scheduler to the picture, with
>> all its locks.
>
> Great catch!
Yes, thanks!
> I think that it is doable to guard the list using RCU.
I think it would be enough to take the prb_cpulock when modifying the
console linked list. That will keep printk_emergency() out until the
list has been updated. (registering/unregistering consoles is not
something that happens often.)
John Ogness
^ permalink raw reply
* Re: [RFC PATCH v1 00/25] printk: new implementation
From: Sergey Senozhatsky @ 2019-03-11 10:54 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, linux-kernel, Peter Zijlstra, Petr Mladek,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky
In-Reply-To: <87lg1rggcz.fsf@linutronix.de>
On (03/07/19 10:53), John Ogness wrote:
[..]
>
> No, I am not sure if we can convert all console drivers to atomic
> consoles. But I think if we don't have to fear disturbing the system,
> the possibilities for such an implementation are greater.
> > If there are setups which can be fully !atomic (in terms of console
> > output) then we, essentially, have a fully preemptible kthread printk
> > implementation.
>
> Correct. I've mentioned in another response[0] some ideas about what
> could be done to aid this.
>
> I understand that fully preemptible kthread printing is unacceptable for
> you.
Well, it's not like it's unacceptable for me. It's just we've been
there, we had preemptible printk(); and people were not happy with
it. Just to demonstrate that I'm not making this up:
> > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>:
> > [..]
> >
> > Using a reproducer [..] you will find that calling cond_resched()
> > (from console_unlock() from printk()) can cause a delay of nearly
> > one minute, and it can cause a delay of nearly 5 minutes to complete
> > one out_of_memory() call.
preemptible printk() and printk() do opposite things.
I can't really say that I care for fbcon; but fully preemptible
netcon is going to hurt.
> Since all current console drivers are already irq safe, I'm
> wondering if using irq_work to handle the emergency printing for console
> drivers without write_atomic() would help. (If the printk caller is in a
> context that write() supports, then write() could be called directly.)
> This would also demand that the irq-safe requirements for write() are
> not relaxed. The printk-kthread might still be faster than irq_work, but
> it might increase reliability if an irq_work is triggered as an extra
> precaution.
Hmm. OK. So one of the things with printk is that it's fully sequential.
We call console drivers one by one. Slow consoles can affect what appears
on the fast consoles; fast console have no impact on slow ones.
call_console_drivers()
for_each_console(c)
c->write(c, text, text_len);
So a list of (slow_serial serial netcon) console drivers is a camel train;
fast netcon is not fast anymore, and slow consoles sometimes are the reason
we have dropped messages. And if we drop messages we drop them for all
consoles, including fast netcon. Turning that sequential pipline into a
bunch of per-console kthreads/irq and letting fast consoles to be fast is
not a completely bad thing. Let's think more about this, I'd like to read
more opinions.
-ss
^ permalink raw reply
* Re: [RFC PATCH v1 08/25] printk: add ring buffer and kthread
From: John Ogness @ 2019-03-11 10:51 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, linux-kernel, Peter Zijlstra, Petr Mladek,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial
In-Reply-To: <20190307051530.GC4893@jagdpanzerIV>
On 2019-03-07, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>> The current printk implementation will do a better job of getting the
>> informational messages out, but at an enormous cost to all the tasks
>> on the system (including the realtime tasks). I am proposing a printk
>> implementation where the tasks are not affected by console printing
>> floods.
>
> In new printk design the tasks are still affected by printing floods.
> Tasks have to line up and (busy) wait for each other, regardless of
> contexts.
They only line up and busy wait is to add the informational message to
the ring buffer. The current printk implementation is the same in this
respect. And as you've noted, the logbuf spinlock is not a source of
latencies.
> One of the late patch sets which I had (I never ever published it) was
> a different kind of printk-kthread offloading. The idea was that
> whatever should be printed (suppress_message_printing()) should be
> printed. We obviously can't loop in console_unlock() for ever and
> there is only one way to figure out if we can print out more messages,
> that's why printk became RCU stall detector and watchdog aware; and
> printk would break out and wake up printk_kthread if it sees that
> watchdog is about to get angry on that particular CPU. printk_kthread
> would run with preemption disabled and do the same thing: if it spent
> watchdog_threshold / 2 printing - breakout, enable local IRQ,
> cond_resched(). IOW watchdogs determine how much time we can spend on
> printing.
I studied and experimented with this (from your git). It was an
interesting idea of keeping the current logic, but allowing to offload
to a separate kthread if things were getting too overloaded. (It is also
where I got term "emergency" from.)
But I was satisfied with neither the direct printing (winner takes all,
printk-safe defers) nor _relying_ on a kthread for important messages in
an offload situation. This is what convinced me that the kernel needs a
new interface so that it can communicate the _really_ important things
synchronously: write_atomic().
>> I want messages of the information category to cause no disturbance
>> to the system. Give the kernel the freedom to communicate to users
>> without destroying its own performance. This can only be achieved if
>> the messages are printed from a _fully_ preemptible context.
> [..]
>> And I want messages of the emergency category to be as reliable as
>> possible, regardless of the costs to the system. Give the kernel a
>> clear mechanism to _reliably_ communicate critical information. Such
>> messages should never appear on a correctly functioning system.
>
> I don't really understand the role of loglevel anymore.
>
> When I do ./a.out --loglevel=X I have a clear understanding that
> all messages which fall into [critical, X] range will be in the logs,
> because I told that application that those messages are important to
> me right now. And it used to be the same with the kernel loglevel.
The loglevel is not related to logging. It specifies the amount of
console printing. But I will assume you are referring to creating log
files by having an external device store the console printing.
> But now the kernel will do its own thing:
>
> - what the kernel considers important will go into the logs
> - what the kernel doesn't consider important _maybe_ will end up
> in the logs (preemptible printk kthread). And this is where
> loglevel now. After the _maybe_ part.
"what the kernel considers" is a configuration option of the
administrator. The administrator can increase the verbocity of the
console (loglevel) without having negative effects on the system
itself. Also, if the system were to suddenly crash, those crash messages
shouldn't be in jeopardy just because the verbocity of the console was
turned up.
You (and Petr) talk about that _all_ console printing is for
emergencies. That if an administrator sets the loglevel to 7 it is
because the pr_info messages are just as important as the pr_emerg. And
if that is indeed the intention of console printing and loglevel, then
why is asynchronous printk calls for console messages even allowed
today? IMO that isn't taking the importance of the message very
seriously.
> If I'm not mistaken, Tetsuo reported that on a box under heavy OOM
> pressure he saw preemptible printk dragging 5 minutes behind the
> logbuf head. Preemptible printk is good for nothing. It's beyond
> useless, it's something else.
The informational messages are correctly timestamped and can be sorted
offline. They are informational, so any loss is less tragic. And they
aren't affecting system performance because they are being printed in
preemptible contexts.
John Ogness
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox