linux-kernel - Re: [PATCH 1/2] serial: 8250: Fix runtime PM for start

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: <YlQXUpNGo8JSppF+@hovoldconsulting.com>
Date:   Mon, 11 Apr 2022 13:56:02 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Matwey V . Kornilov" <matwey@....msu.ru>,
        Steffen Trumtrar <s.trumtrar@...gutronix.de>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH 1/2] serial: 8250: Fix runtime PM for start_tx() for RS485

On Mon, Apr 11, 2022 at 01:32:58PM +0300, Tony Lindgren wrote:
> * Johan Hovold <johan@...nel.org> [220411 10:23]:
> > On Mon, Apr 11, 2022 at 01:10:34PM +0300, Tony Lindgren wrote:
> > > * Johan Hovold <johan@...nel.org> [220411 09:54]:
> > > > On Mon, Apr 11, 2022 at 12:48:04PM +0300, Tony Lindgren wrote:
> > > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > > > --- a/drivers/tty/serial/8250/8250_port.c
> > > > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > > > @@ -1681,8 +1681,10 @@ static void serial8250_start_tx(struct uart_port *port)
> > > > >  		return;
> > > > >  
> > > > >  	if (em485 &&
> > > > > -	    em485->active_timer == &em485->start_tx_timer)
> > > > > +	    em485->active_timer == &em485->start_tx_timer) {
> > > > > +		serial8250_rpm_put_tx(up);
> > > > >  		return;
> > > > > +	}

> > The problem is that that serial8250_rpm_put_tx() you're adding may
> > suspend the device unconditionally (i.e. regardless of any previous
> > calls to serial8250_rpm_get_tx()).
> > 
> > If rs485 tx is just being deferred you mustn't suspend the device before
> > it has had a chance to start transmitting.
> 
> Hmm I'm pretty sure rs485 has the runtime PM usage count is currently
> unbalanced. To me it seems em485->start_tx_timer calls start_tx()
> again from serial8250_em485_handle_start_tx().

It appears to call __start_tx(), but note that the only call to
serial8250_rpm_get_tx() is in serial8250_start_tx() which this patch
would have cancelled out.
 
Also note that the serial8250_rpm_get/set_tx() calls aren't supposed to
be balanced. get() can be called multiple times and will only increment
the PM usage counter once, while put() will decrement the counter
whenever get() has been called once (and hence potentially suspend the
device immediately).

Messy indeed.

Johan