linux-kernel - Re: [PATCH v12 1/1] serial: core: Start managing serial controllers to enable runtime PM

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]

Message-ID: <CAGXv+5HwL+R5QpO3pHGQd9qAxu2pCMDjYvdni1HjiC8eEE38mg@mail.gmail.com>
Date:   Mon, 5 Jun 2023 21:01:29 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     John Ogness <john.ogness@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Dhruva Gole <d-gole@...com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Johan Hovold <johan@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-omap@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v12 1/1] serial: core: Start managing serial controllers
 to enable runtime PM

On Mon, Jun 5, 2023 at 8:24 PM Tony Lindgren <tony@...mide.com> wrote:
>
> * Chen-Yu Tsai <wenst@...omium.org> [230605 11:34]:
> > On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <tony@...mide.com> wrote:
> > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > > --- a/drivers/tty/serial/8250/8250_mtk.c
> > > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > > @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > >  static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > >  {
> > >         struct mtk8250_data *data = dev_get_drvdata(dev);
> > > -       struct uart_8250_port *up = serial8250_get_port(data->line);
> > >
> > >         /* wait until UART in idle status */
> > >         while
> > > -               (serial_in(up, MTK_UART_DEBUG0));
> > > +               (mtk8250_read(data, MTK_UART_DEBUG0));
> >
> > I believe it still gets stuck here sometimes.
>
> Hmm so maybe you need to mtk8250_write(data, 0, MTK_UART_RATE_FIX) in
> probe before pm_runtime_resume_and_get() that enables the baud clock?
> That's something I changed, so maybe it messes up things.

I think it has something to do with the do_pm() function calling
the callbacks directly, then also calling runtime PM.

> Looking at the 8250_mtk git log, it's runtime PM functions seem to only
> currently manage the baud clock so register access should be doable
> without runtime PM resume?

Actually it only manages the bus clock. The baud clock is simply the system
XTAL which is not gateble.

> > With your earlier patch, it could get through registering the port, and
> > the console would show
> >
> >     11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
> > 1625000) is a ST16650V2
> >
> > for the console UART.
>
> OK
>
> > Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
> > in the MTK UART hardware.
> >
> > I tried reworking it into your patch here, but it causes issues with the
> > UART-based Bluetooth on one of my devices. After the UART runtime suspends
> > and resumes, something is off and causes the transfers during Bluetooth
> > init to become corrupt.
> >
> > I'll try some more stuff, but the existing code seems timing dependent.
> > If I add too many printk statements to the runtime suspend/resume
> > callbacks, things seem to work. One time I even ended up with broken
> > UARTs but otherwise booted up the system.
>
> Well another thing that now changes is that we now runtime suspend the
> port at the end of the probe. What the 8250_mtk probe was doing earlier
> it was leaving the port baud clock enabled, but runtime PM disabled
> until mtk8250_do_pm() I guess.

I guess that's the biggest difference? Since the *bus* clock gets disabled,
any access will hang. Is it enough to just support runtime PM? Or do I have
to also have UART_CAP_RPM?

ChenYu