linux-kernel - Re: [PATCH] serial: 8250_omap: Fix idling for unloaded serdev drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 4 Oct 2019 14:29:35 -0500
From:   Adam Ford <aford173@...il.com>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Peter Hurley <peter@...leysoftware.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Ujfalusi <peter.ujfalusi@...com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh R <vigneshr@...com>, linux-serial@...r.kernel.org,
        Linux-OMAP <linux-omap@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH] serial: 8250_omap: Fix idling for unloaded serdev drivers

On Tue, Jul 23, 2019 at 5:21 PM Tony Lindgren <tony@...mide.com> wrote:
>
> For many years omap variants have been setting the runtime PM
> autosuspend delay to -1 to prevent unsafe policy with lossy first
> character on wake-up. The user must specifically enable the timeout
> for UARTs if desired.
>
> We must not enable the workaround for serdev devices though. It leads
> into UARTs not idling if no serdev devices are loaded and there is no
> sysfs entry to configure the UART in that case. And this means that
> my PM may not work unless the serdev modules are loaded.
>
> We can detect a serdev device being configured based on a dts child
> node, and we can simply skip the workround in that case. And the
> serdev driver can idle the port during runtime when suitable if an
> out-of-band wake-up GPIO line exists for example.
>
> Let's also add some comments to the workaround while at it.

This seems to help some of the stability issues I am seeing on the
DM3730 UART2 running Bluetooth at 3000000 baud.
Does it make sense to backport this to the stable kernels?

adam

>
> Cc: Johan Hovold <johan@...nel.org>
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -1234,7 +1234,16 @@ static int omap8250_probe(struct platform_device *pdev)
>
>         device_init_wakeup(&pdev->dev, true);
>         pm_runtime_use_autosuspend(&pdev->dev);
> -       pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
> +
> +       /*
> +        * Disable runtime PM until autosuspend delay unless specifically
> +        * enabled by the user via sysfs. This is the historic way to
> +        * prevent an unsafe default policy with lossy characters on wake-up.
> +        * For serdev devices this is not needed, the policy can be managed by
> +        * the serdev driver.
> +        */
> +       if (!of_get_available_child_count(pdev->dev.of_node))
> +               pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
>
>         pm_runtime_irq_safe(&pdev->dev);
>         pm_runtime_enable(&pdev->dev);
> --
> 2.21.0