[PATCH resend] serial_core: Fix pm imbalance on unbind

[PATCH resend] serial_core: Fix pm imbalance on unbind

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@linux-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@linux-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);
 		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);
+	}
 
 	/*
 	 * Free the port IO and memory resources, if any.
-- 
1.7.9.5

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

[Peter Hurley]

On 03/21/2014 05:08 AM, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@linux-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@linux-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.
>

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

[Geert Uytterhoeven]

Hi Peter,

On Fri, Mar 21, 2014 at 2:06 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> @@ -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 didn't see that actually happening.

> 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()).]

But indeed, for concistency/symmetry uport->state and uport->cons
should be resend, but that's something separate.

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

uart_close() won't get that far, so uart_change_pm() won't be called.

See also https://lkml.org/lkml/2014/3/10/651, and my workaround for the
crash https://lkml.org/lkml/2014/3/17/231.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Peter Hurley]

Hi Geert,

On 03/21/2014 09:23 AM, Geert Uytterhoeven wrote:
> Hi Peter,
>
> On Fri, Mar 21, 2014 at 2:06 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> @@ -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 didn't see that actually happening.

Ok, but I still think this isn't right. See below.

>> 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()).]
>
> But indeed, for concistency/symmetry uport->state and uport->cons
> should be resend, but that's something separate.

I don't see this as being a "looks good" problem; I see this as being
"what's the right way to teardown a uart device that's going away when
a tty console is running on it", and there are too many problems with
uart_remove_one_port() doing:

      state->uart_port = NULL

to keep with that solution.

>> Then, uart_close() will power off the port when all ttys using the port have
>> been closed.
>
> uart_close() won't get that far, so uart_change_pm() won't be called.

And this is the central problem: uart_close() must complete normally
if a tty console is still running on a device.

For example, if uart_shutdown() isn't getting called, then who's freeing
the ring buffer page?


> See also https://lkml.org/lkml/2014/3/10/651, and my workaround for the
> crash https://lkml.org/lkml/2014/3/17/231.

See my comments to the v2 patch there.

Regards,
Peter Hurley

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

[Geert Uytterhoeven]

On Fri, Mar 21, 2014 at 2:06 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> 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@linux-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.

Thanks, I'll send an updated patch handling the non-serial console case only.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

On Fri, Mar 21, 2014 at 2:06 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> 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@linux-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.

Thanks, I'll send an updated patch handling the non-serial console case only.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds