linux-kernel - Re: [PATCH resend] serial_core: Fix pm imbalance on unbind

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, 21 Mar 2014 09:06:18 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>, linux-serial@...r.kernel.org,
	linux-sh@...r.kernel.org, linux-kernel@...r.kernel.org,
	Geert Uytterhoeven <geert+renesas@...ux-m68k.org>
Subject: Re: [PATCH resend] serial_core: Fix pm imbalance on unbind

On 03/21/2014 05:08 AM, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@...ux-m68k.org>
>
> When a serial port is closed, uart_close() takes care of shutting down the
> hardware, and powering it down.
>
> When a serial port is unbound while in use, uart_close() bypasses all of
> this, as this is supposed to be done through uart_hangup() (invoked via
> tty_vhangup() in uart_remove_one_port()).
>
> However, uart_hangup() does not set the hardware's power state, leaving it
> powered up.  This may also lead to unbounded nesting counts in clock and
> power management, depending on their internal implementation.
>
> Make sure to power down the port in uart_hangup(), except when the port is
> used as a serial console. For serial consoles, this must be postponed until
> after their deregistration in uart_remove_one_port() (symmetry with
> registration in uart_configure_port(), invoked from uart_add_one_port()).
>
> After this, the module clock used by the sh-sci driver is disabled on
> unbind while the serial port is in use.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...ux-m68k.org>
> ---
>   drivers/tty/serial/serial_core.c |    8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 2cf5649a6dc0..56dda84f82a5 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1452,6 +1452,8 @@ static void uart_hangup(struct tty_struct *tty)
>   		clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
>   		spin_unlock_irqrestore(&port->lock, flags);
>   		tty_port_tty_set(port, NULL);
> +		if (!uart_console(state->uart_port))
> +			uart_change_pm(state, UART_PM_STATE_OFF);

Ok.

>   		wake_up_interruptible(&port->open_wait);
>   		wake_up_interruptible(&port->delta_msr_wait);
>   	}
> @@ -2681,10 +2683,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>   	}
>
>   	/*
> -	 * If the port is used as a console, unregister it
> +	 * If the port is used as a console, unregister it, and power it down
>   	 */
> -	if (uart_console(uport))
> +	if (uart_console(uport)) {
>   		unregister_console(uport->cons);
> +		uart_change_pm(state, UART_PM_STATE_OFF);

Won't this power off the port while tty consoles may still be open?
I think the right thing here is to unregister_console then set uport->cons = NULL

[uport->cons is properly reassigned when/if a port is re-added via
uart_add_one_port()).]

Then, uart_close() will power off the port when all ttys using the port have
been closed.

> +	}
>
>   	/*
>   	 * Free the port IO and memory resources, if any.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/